Author vstinner
Recipients BTaskaya, corona10, dino.viehland, eric.snow, lukasz.langa, pablogsal, petr.viktorin, shihai1991, vstinner
Date 2020-09-02.16:46:35
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1599065195.58.0.349425781811.issue41631@roundup.psfhosted.org>
In-reply-to
Content
At commit ac46eb4ad6662cf6d771b20d8963658b2186c48c ("bpo-38113: Update the Python-ast.c generator to PEP384 (gh-15957)"):
---------------
static struct PyModuleDef _astmodule = {
    PyModuleDef_HEAD_INIT,
    "_ast",
    NULL,
    sizeof(astmodulestate),
    NULL,
    NULL,
    astmodule_traverse,
    astmodule_clear,
    astmodule_free,
};

#define astmodulestate_global ((astmodulestate *)PyModule_GetState(PyState_FindModule(&_astmodule)))

PyMODINIT_FUNC
PyInit__ast(void)
{
    PyObject *m;
    if (!init_types()) return NULL;
    m = PyState_FindModule(&_astmodule);
    ...
    return m;
}
---------------

=> I investigated bpo-41194 crash again. It seems like the root issue is more than PyInit__ast() returns a *borrowed* reference, rather than a strong reference.

=> The _ast module can only be loaded once, astmodule_free() and astmodule_clear() are not called at Python exit.


At commit 91e1bc18bd467a13bceb62e16fbc435b33381c82 ("bpo-41194: The _ast module cannot be loaded more than once (GH-21290)"):
---------------
static astmodulestate global_ast_state;

static astmodulestate *
get_ast_state(PyObject *Py_UNUSED(module))
{
    return &global_ast_state;
}

#define get_global_ast_state() (&global_ast_state)

static struct PyModuleDef _astmodule = {
    PyModuleDef_HEAD_INIT,
    .m_name = "_ast",
    .m_size = -1,
    .m_traverse = astmodule_traverse,
    .m_clear = astmodule_clear,
    .m_free = astmodule_free,
};

PyMODINIT_FUNC
PyInit__ast(void)
{
    PyObject *m = PyModule_Create(&_astmodule);
    if (!m) {
        return NULL;
    }
    astmodulestate *state = get_ast_state(m);
    ...
}
---------------

* bpo-41194 => ok, PyInit__ast() is fine
* bpo-41631 => ok: PyAST_Check() doesn't import the module, it uses a global state.
* bpo-41261 => ERROR: This implementation allows to create two instances of the _ast module, but also to unload one instance (call astmodule_free), because _astmodule.m_size = -1.


At commit b1cc6ba73a51d5cc3aeb113b5e7378fb50a0e20a ("bpo-41194: Convert _ast extension to PEP 489 (GH-21293)"):
---------------
static astmodulestate*
get_global_ast_state(void)
{
    ...
    PyObject *module = PyImport_GetModule(name);
    if (module == NULL) {
        if (PyErr_Occurred()) {
            return NULL;
        }
        module = PyImport_Import(name);
        if (module == NULL) {
            return NULL;
        }
    }
    ...
    astmodulestate *state = get_ast_state(module);
    Py_DECREF(module);
    return state;
}

static struct PyModuleDef _astmodule = {
    PyModuleDef_HEAD_INIT,
    .m_name = "_ast",
    .m_size = sizeof(astmodulestate),
    .m_slots = astmodule_slots,
    .m_traverse = astmodule_traverse,
    .m_clear = astmodule_clear,
    .m_free = astmodule_free,
};

PyMODINIT_FUNC
PyInit__ast(void)
{
    return PyModuleDef_Init(&_astmodule);
}
---------------

* bpo-41631 => ERROR: PyAST_Check() imports "_ast" module
History
Date User Action Args
2020-09-02 16:46:35vstinnersetrecipients: + vstinner, dino.viehland, petr.viktorin, lukasz.langa, eric.snow, corona10, pablogsal, BTaskaya, shihai1991
2020-09-02 16:46:35vstinnersetmessageid: <1599065195.58.0.349425781811.issue41631@roundup.psfhosted.org>
2020-09-02 16:46:35vstinnerlinkissue41631 messages
2020-09-02 16:46:35vstinnercreate