classification
Title: PyImport_GetModule() can return partially-initialized module
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.10, Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: nanjekyejoannah Nosy List: Big Stone, Valentyn Tymofieiev, brett.cannon, cebtenzzre, eric.snow, gjb1002, ishimoto, nanjekyejoannah, ncoghlan, p-ganssle, pablogsal, pitrou, vstinner
Priority: normal Keywords: patch

Created on 2019-02-08 17:13 by pitrou, last changed 2021-03-16 16:17 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
importerror-sample.tgz ishimoto, 2020-10-23 15:18
Pull Requests
URL Status Linked Edit
PR 15057 merged nanjekyejoannah, 2019-07-31 15:51
Messages (19)
msg335097 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2019-02-08 17:13
PyImport_GetModule() returns whatever is in sys.modules, even if the module is still importing and therefore only partially initialized.

One possibility is to reuse the optimization already done in PyImport_ImportModuleLevelObject():

        /* Optimization: only call _bootstrap._lock_unlock_module() if
           __spec__._initializing is true.
           NOTE: because of this, initializing must be set *before*
           stuffing the new module in sys.modules.
         */
        spec = _PyObject_GetAttrId(mod, &PyId___spec__);
        if (_PyModuleSpec_IsInitializing(spec)) {
            PyObject *value = _PyObject_CallMethodIdObjArgs(interp->importlib,
                                            &PyId__lock_unlock_module, abs_name,
                                            NULL);
            if (value == NULL) {
                Py_DECREF(spec);
                goto error;
            }
            Py_DECREF(value);
        }
        Py_XDECREF(spec);

Issue originally mentioned in issue34572.
msg335100 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-02-08 17:18
Yeah, that makes sense.
msg351847 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-09-11 12:47
New changeset 37c22206981f52ae35c28b39f7530f8438afbfdb by Brett Cannon (Joannah Nanjekye) in branch 'master':
bpo-35943: Prevent PyImport_GetModule() from returning a partially-initialized module (GH-15057)
https://github.com/python/cpython/commit/37c22206981f52ae35c28b39f7530f8438afbfdb
msg356920 - (view) Author: Valentyn Tymofieiev (Valentyn Tymofieiev) Date: 2019-11-18 22:11
Do we plan to backport the change by nanjekyejoannah to 3.7 branch?
msg356980 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-11-19 17:37
I've assigned this to Joannah to decide if she wants to backport this.
msg357201 - (view) Author: Valentyn Tymofieiev (Valentyn Tymofieiev) Date: 2019-11-21 19:30
Thanks. Is it possible that this issue and  https://bugs.python.org/issue38884 are duplicates?
msg360056 - (view) Author: Joannah Nanjekye (nanjekyejoannah) * (Python committer) Date: 2020-01-15 13:59
The changes required to successfully do this backport are many and affect critical areas. I am not in a hurry to do this. If anyone else wants to take this up quickly, please do.
msg360075 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-15 21:27
> The changes required to successfully do this backport are many and affect critical areas. I am not in a hurry to do this. If anyone else wants to take this up quickly, please do.

Do you mean that there is a risk that the backport introduces a regression in another part of the code? If yes, I would suggest to not backport the change to *stable* branches.

People survived with bug. Do you really *have to* backport the fix?

Note: this issue is closed. If you consider to backport it, I suggest to reopen the issue.
msg360399 - (view) Author: Joannah Nanjekye (nanjekyejoannah) * (Python committer) Date: 2020-01-21 14:14
> Do you mean that there is a risk that the backport introduces a regression in another part of the code? If yes, I would suggest to not backport the change to *stable* branches.

My worry are the many changes that are required to ceval to make this back port work. Not that I think we can not successfully backport things. we can.
msg362300 - (view) Author: Geoffrey Bache (gjb1002) Date: 2020-02-20 07:39
I have been experiencing what I thought was this issue in my embedded Python code. We have been using Python 3.7, so I thought upgrading to 3.8.1 would fix it, but it doesn't seem to have made any difference.

My C++ code essentially can call PyImport_GetModule() from two threads simultaneously on the same module A. The symptoms I see are that one of them then gets a stacktrace in module B (imported by A), saying that some symbol defined near the end of B does not exist.

I've also noticed that this happens far more often on deployed code (where Python modules end up in a zip file) than when run directly in development (where the modules are just normal files). I can't see any difference in the frequency between 3.7.5 and 3.8.1.

Any ideas? Should I reopen this?
msg362303 - (view) Author: Geoffrey Bache (gjb1002) Date: 2020-02-20 08:00
Oops, I mean we call PyImport_ImportModule and get these issues when the files are zipped. Unless that calls PyImport_GetModule internally I guess it's not related to this then.
msg362329 - (view) Author: Valentyn Tymofieiev (Valentyn Tymofieiev) Date: 2020-02-20 16:17
@gjb1002: see also https://bugs.python.org/issue38884, which demonstrates that concurrent imports are not thread-safe on Python 3.
msg362347 - (view) Author: Geoffrey Bache (gjb1002) Date: 2020-02-20 20:22
@Valentyn Tymofieiev - true, and thanks for the tip, though the symptoms described there are somewhat different from what I'm observing. Also, my problem seems to be dependent on zipping the Python code, which that one isn't.
msg379441 - (view) Author: Atsuo Ishimoto (ishimoto) * Date: 2020-10-23 15:18
After this fix, some functions like multiprocessing.Pool cannot be used in threaded code(https://bugs.python.org/issue41567).

importerror-sample.tgz contains simplified code to reproduce the same error without multiprocessing module. Is this an expected behaviour of this change?

Tested with Python 3.9.0/macOS 10.15.5.
msg383433 - (view) Author: Big Stone (Big Stone) Date: 2020-12-20 16:22
Is this bug causing the Dask-Jupyterlab failure ? 
https://github.com/dask/distributed/issues/4168
msg388846 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2021-03-16 15:22
Note the conjunction of this change + issue32596 produces import fragility:
https://bugs.python.org/issue43515
msg388847 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2021-03-16 15:27
Ok, going through other open issues including on third-party projects, I think these changes should unfortunately be reverted.  The regressions produced are far from trivial and most developers seem at a loss how to fix them.
msg388849 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2021-03-16 15:44
After analysis, it may not need reversal.  There is a simple logic error it seems.  Will check.
msg388857 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2021-03-16 16:17
Created a new issue + fix in issue43517.
History
Date User Action Args
2021-03-16 16:17:49pitrousetstatus: open -> closed
resolution: fixed
messages: + msg388857

stage: needs patch -> resolved
2021-03-16 15:44:10pitrousetmessages: + msg388849
2021-03-16 15:43:44pitrousetstatus: closed -> open
stage: resolved -> needs patch
resolution: fixed -> (no value)
versions: + Python 3.9, Python 3.10, - Python 3.7, Python 3.8
2021-03-16 15:27:34pitrousetmessages: + msg388847
2021-03-16 15:22:29pitrousetmessages: + msg388846
2020-12-28 22:49:32cebtenzzresetnosy: + cebtenzzre
2020-12-20 16:22:46Big Stonesetnosy: + Big Stone
messages: + msg383433
2020-10-23 15:18:57ishimotosetfiles: + importerror-sample.tgz
nosy: + ishimoto
messages: + msg379441

2020-02-20 20:22:19gjb1002setmessages: + msg362347
2020-02-20 16:17:39Valentyn Tymofieievsetmessages: + msg362329
2020-02-20 08:00:19gjb1002setmessages: + msg362303
2020-02-20 07:39:32gjb1002setnosy: + gjb1002
messages: + msg362300
2020-01-21 14:14:27nanjekyejoannahsetmessages: + msg360399
2020-01-15 21:27:52vstinnersetmessages: + msg360075
2020-01-15 13:59:48nanjekyejoannahsetmessages: + msg360056
2019-11-21 19:30:35Valentyn Tymofieievsetmessages: + msg357201
2019-11-19 17:37:29brett.cannonsetassignee: nanjekyejoannah

messages: + msg356980
nosy: + nanjekyejoannah
2019-11-18 22:11:12Valentyn Tymofieievsetnosy: + Valentyn Tymofieiev
messages: + msg356920
2019-09-11 12:48:02brett.cannonsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-09-11 12:47:42brett.cannonsetmessages: + msg351847
2019-07-31 15:51:07nanjekyejoannahsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request14806
2019-02-22 17:27:35pitrousetnosy: + vstinner
2019-02-08 18:56:13p-gansslesetnosy: + p-ganssle
2019-02-08 17:19:40pitrousetnosy: + pablogsal
2019-02-08 17:18:15eric.snowsetmessages: + msg335100
2019-02-08 17:13:37pitroucreate