bpo-30814: Fixed a race condition when import a submodule from a package.#2580
Conversation
serhiy-storchaka
added
needs backport to 3.6
type-bug
|
@serhiy-storchaka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @brettcannon, @pitrou and @ericsnowcurrently to be potential reviewers. |
ncoghlan
left a comment
ncoghlan
left a comment
There was a problem hiding this comment.
The general approach looks good to me, but I think a dedicated helper function will be clearer when handling the mod == Py_None case in C, rather than relying on the second module is None check in _find_and_load.
| with _ModuleLockManager(name): | ||
| return _find_and_load_unlocked(name, import_) | ||
| """Find and load the module.""" | ||
| _imp.acquire_lock() |
There was a problem hiding this comment.
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?
| goto error; | ||
| } | ||
| else if (mod != NULL) { | ||
| if (mod != NULL && mod != Py_None) { |
There was a problem hiding this comment.
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.
| module = sys.modules[name] | ||
| if module is None: | ||
| _imp.release_lock() | ||
| message = ('import of {} halted; ' |
There was a problem hiding this comment.
This would call _raise_for_halted_import(name, import_).
|
Calling Python method from the C code is not much simpler than generating an exception. The Note that the new If you prefer I can withdraw changes in I also can implement the second check in C and keep Python code unchanged. But I think that it is better to implement performancy non-critical parts in Python. |
|
@serhiy-storchaka I think it's specifically the paired negations in |
|
Do you want to swap also other branches for |
|
@serhiy-storchaka I switched my review, as coming back to it after a couple of hours, I now suspect my initial confusion was specifically due to reviewing the diff, rather than reading the updated code standalone. That realisation came mainly from comparing it to the following |
…a package. (pythonGH-2580). (cherry picked from commit b4baace)
|
GH-2598 is a backport of this pull request to the 3.6 branch. |
|
Thank you for your review @ncoghlan. |
…a package. (pythonGH-2580). (cherry picked from commit b4baace)
5 participants
No description provided.