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

Created on 2019-02-08 17:13 by pitrou, last changed 2020-02-20 20:22 by gjb1002. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 15057 merged nanjekyejoannah, 2019-07-31 15:51
Messages (13)
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.
History
Date User Action Args
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