Skip to content

bpo-30814: Fixed a race condition when import a submodule from a package. #2580

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions Lib/importlib/_bootstrap.py
Original file line number Diff line number Diff line change
Expand Up @@ -956,9 +956,19 @@ def _find_and_load_unlocked(name, import_):


def _find_and_load(name, import_):
"""Find and load the module, and release the import lock."""
with _ModuleLockManager(name):
return _find_and_load_unlocked(name, import_)
"""Find and load the module."""
_imp.acquire_lock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This consolidation seems a little confusing to me, but I like the idea of simplifying the C code by moving the ModuleNotFoundError generation into Python.

How about adding a dedicated _raise_for_halted_import(name, import_): function that just handles the module is None case?

if name not in sys.modules:
with _ModuleLockManager(name):
return _find_and_load_unlocked(name, import_)
module = sys.modules[name]
if module is None:
_imp.release_lock()
message = ('import of {} halted; '
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would call _raise_for_halted_import(name, import_).

'None in sys.modules'.format(name))
raise ModuleNotFoundError(message, name=name)
_lock_unlock_module(name)
return module


def _gcd_import(name, package=None, level=0):
Expand All @@ -973,17 +983,7 @@ def _gcd_import(name, package=None, level=0):
_sanity_check(name, package, level)
if level > 0:
name = _resolve_name(name, package, level)
_imp.acquire_lock()
if name not in sys.modules:
return _find_and_load(name, _gcd_import)
module = sys.modules[name]
if module is None:
_imp.release_lock()
message = ('import of {} halted; '
'None in sys.modules'.format(name))
raise ModuleNotFoundError(message, name=name)
_lock_unlock_module(name)
return module
return _find_and_load(name, _gcd_import)


def _handle_fromlist(module, fromlist, import_):
Expand Down
28 changes: 28 additions & 0 deletions Lib/test/test_import/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import random
import stat
import sys
import threading
import time
import unittest
import unittest.mock as mock
import textwrap
Expand Down Expand Up @@ -380,6 +382,32 @@ def __getattr__(self, _):
self.assertEqual(str(cm.exception),
"cannot import name 'does_not_exist' from '<unknown module name>' (unknown location)")

def test_concurrency(self):
sys.path.insert(0, os.path.join(os.path.dirname(__file__), 'data'))
try:
exc = None
def run():
event.wait()
try:
import package
except BaseException as e:
nonlocal exc
exc = e

for i in range(10):
event = threading.Event()
threads = [threading.Thread(target=run) for x in range(2)]
try:
with test.support.start_threads(threads, event.set):
time.sleep(0)
finally:
sys.modules.pop('package', None)
sys.modules.pop('package.submodule', None)
if exc is not None:
raise exc
finally:
del sys.path[0]


@skip_if_dont_write_bytecode
class FilePermissionTests(unittest.TestCase):
Expand Down
2 changes: 2 additions & 0 deletions Lib/test/test_import/data/package/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import package.submodule
package.submodule
Empty file.
2 changes: 2 additions & 0 deletions Misc/NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ What's New in Python 3.7.0 alpha 1?
Core and Builtins
-----------------

- bpo-30814: Fixed a race condition when import a submodule from a package.

- bpo-30736: The internal unicodedata database has been upgraded to Unicode
10.0.

Expand Down
17 changes: 1 addition & 16 deletions Python/import.c
Original file line number Diff line number Diff line change
Expand Up @@ -1532,18 +1532,7 @@ PyImport_ImportModuleLevelObject(PyObject *name, PyObject *globals,
}

mod = PyDict_GetItem(interp->modules, abs_name);
if (mod == Py_None) {
PyObject *msg = PyUnicode_FromFormat("import of %R halted; "
"None in sys.modules", abs_name);
if (msg != NULL) {
PyErr_SetImportErrorSubclass(PyExc_ModuleNotFoundError, msg,
abs_name, NULL);
Py_DECREF(msg);
}
mod = NULL;
goto error;
}
else if (mod != NULL) {
if (mod != NULL && mod != Py_None) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the separate helper function, we'd keep the dedicated mod == Py_None branch, but it would be simplified to just calling the new _raise_for_halted_import helper rather than constructing the exception directly in C.

_Py_IDENTIFIER(__spec__);
_Py_IDENTIFIER(_initializing);
_Py_IDENTIFIER(_lock_unlock_module);
Expand Down Expand Up @@ -1584,10 +1573,6 @@ PyImport_ImportModuleLevelObject(PyObject *name, PyObject *globals,
}
}
else {
#ifdef WITH_THREAD
_PyImport_AcquireLock();
#endif
/* _bootstrap._find_and_load() releases the import lock */
mod = _PyObject_CallMethodIdObjArgs(interp->importlib,
&PyId__find_and_load, abs_name,
interp->import_func, NULL);
Expand Down
Loading