bpo-30891: importlib _find_and_load() try/except #2665

vstinner · 2017-07-11T08:03:33Z

importlib _find_and_load() now uses a "try/except KeyError" block to
check if the imported module is not sys.modules, rather than checking
for "name is sys.modules" and then get it with sys.modules[name].

This change should prevent a race condition.

mention-bot · 2017-07-11T08:03:35Z

@Haypo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @brettcannon, @ericsnowcurrently and @pitrou to be potential reviewers.

vstinner · 2017-07-11T08:06:09Z

I already proposed this change in my previous PR #2646, but @serhiy-storchaka complained that it would make import() slower.

But @ncoghlan also noticed that previously, the whole function was protected by the global imp lock, whereas now "module = sys.modules[name]" is not protected by any lock and so there is a risk of a race condition.

I agree, that's why I wrote this PR.

See also discussion at PR #2646.

vstinner · 2017-07-11T08:07:47Z

If @serhiy-storchaka still considers that performance is more important that atomicity in this function, we can just move "module = sys.modules[name]" into the "with _ModuleLockManager(name):" block.

serhiy-storchaka · 2017-07-11T08:16:15Z

In common case _find_and_load() is called when name not in sys.modules (this is checked in C code). This will cause raising KeyError. Catching an exception in Python code is slow. Therefore this change will make slower the common case.

You can use sys.modules.get(name, sentinel). This is atomic as well as sys.modules[name], but faster in common case (name not in sys.modules).

ncoghlan

If anything this will make the "already loaded" case a tiny bit faster (since it skips the double name lookup), while making initial loads marginally slower (but likely imperceptibly slower relative to the overheads of actually reading files and directories from disk).

So +1 from me for going with the conventional solution of switching to relying on exception handling to help ensure consistency of state.

serhiy-storchaka · 2017-07-11T08:22:22Z

The "already loaded" case already is made significantly faster by handling in C code. This code is executed in "still not loaded" case.

ncoghlan · 2017-07-11T08:26:40Z

@serhiy-storchaka The module lock means that in the presence of concurrent attempts at loading the module, the 2nd and subsequent threads will follow the "no KeyError" path.

It occurs to me that this could use a comment after the with statement explaining the need for that path, though.

There's also the micro-optimisation option you suggested on the original PR: creating a _NEEDS_LOADING sentinel, and using sys.modules.get(name, _NEEDS_LOADING) instead of the try/except.

serhiy-storchaka · 2017-07-11T08:37:47Z

A race condition is exceptional. It happens only when two or more threads simultaneously import the same module first time. Once the module is imported, all subsequent imports go by fast path in C code and don't have a race condition.

This PR fixes even more rare case. When at the same time one thread unloads a module while other thread imports it (this case is similar to bpo-30876). I agree that this case is worth to be fixed, but not at the cost of more common cases.

vstinner · 2017-07-11T09:15:39Z

You can use sys.modules.get(name, sentinel).

I don't want to add a sentinel object. I just moved "module = sys.modules[name]" in the "with _ModuleLockManager(name):" block.

ncoghlan · 2017-07-11T09:40:26Z

Lib/importlib/_bootstrap.py

        if name not in sys.modules:
            return _find_and_load_unlocked(name, import_)
+        module = sys.modules[name]


This is the branch that I think could use a comment to say "Another thread updated sys.modules while this one was waiting for the import lock"

ncoghlan

Added a note suggesting a clarifying comment around when the fallback path executes, but either way I'm fine with merging this approach.

vstinner · 2017-07-11T10:21:50Z

Sorry, I don't understand which solution should be implemented:

"try/except KeyError" is too slow according to @serhiy-storchaka
"name in" followed by "module = sys.modules[name"] is unsafe accordng to @ncoghlan
Is sys.modules.get(name, sentinel) the right fix for this issue? Would it be fast enough?

serhiy-storchaka · 2017-07-11T11:04:07Z

Both (1) and (3) are safe. (1) and (3) are equally safe if ignore some corner cases (non-string keys in sys.modules which raise KeyError on comparison). In normal case (3) is slightly faster than (1).

ncoghlan · 2017-07-11T12:26:32Z

The "name in" + "sys.modules[name]" case is fine if you're holding the module lock the entire time (hence why I switched my review to approved), but I prefer the sys.modules.get approach with a sentinel since it is more obviously self-consistent and avoids the overhead of a Python level exception handler.

vstinner · 2017-07-11T19:45:08Z

Ok, let's try sys.modules.get().

serhiy-storchaka · 2017-07-12T04:00:23Z

Lib/importlib/_bootstrap.py

@@ -957,13 +954,16 @@ def _find_and_load_unlocked(name, import_):
    return module


+_SENTINEL = object()


Nick suggested the name _NEEDS_LOADING and it looks better to me.

Use sys.modules.get() in the "with _ModuleLockManager(name):" block to protect the dictionary key with the module lock and use an atomic get to prevent race condition. Remove also _bootstrap._POPULATE since it was unused (_bootstrap_external now has its own _POPULATE object), add a new _SENTINEL object instead.

serhiy-storchaka · 2017-07-21T10:19:15Z

Since this fixes an observable behavior, I think it needs a NEWS entry.

vstinner · 2017-07-21T10:49:13Z

Since this fixes an observable behavior, I think it needs a NEWS entry.

Can you please elaborate? How replacing "name not in sys.modules" with "sys.modules.get(name, _NEEDS_LOADING)" changes the behaviour? sys.modules is a dummy dict, so there is no behaviour change, no?

The main change is that it fixes a race condition, but this commit is the 3rd one in http://bugs.python.org/issue30891 and the first commit was made by you and already contains a NEWS item:

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

It's enough to summarize the 3 commits, no?

ncoghlan · 2017-07-21T10:52:27Z

Right, I think the original NEWS entry is sufficient to cover all the changes - we're just iterating on the exact details of how that fix works.

bedevere-bot · 2017-07-21T11:05:08Z

GH-2801 is a backport of this pull request to the 3.6 branch.

Use sys.modules.get() in the "with _ModuleLockManager(name):" block to protect the dictionary key with the module lock and use an atomic get to prevent race condition. Remove also _bootstrap._POPULATE since it was unused (_bootstrap_external now has its own _POPULATE object), add a new _SENTINEL object instead. (cherry picked from commit e72b135)

serhiy-storchaka · 2017-07-21T11:32:26Z

It's enough to summarize the 3 commits, no?

Agree.

the-knights-who-say-ni added the CLA signed label Jul 11, 2017

vstinner requested review from ncoghlan and serhiy-storchaka July 11, 2017 08:03

vstinner mentioned this pull request Jul 11, 2017

bpo-30891: Fix importlib _find_and_load() race condition #2646

Merged

ncoghlan approved these changes Jul 11, 2017

View reviewed changes

ncoghlan reviewed Jul 11, 2017

View reviewed changes

ncoghlan approved these changes Jul 11, 2017

View reviewed changes

serhiy-storchaka approved these changes Jul 12, 2017

View reviewed changes

serhiy-storchaka approved these changes Jul 21, 2017

View reviewed changes

serhiy-storchaka added needs backport to 3.5 type-bug An unexpected behavior, bug, or error labels Jul 21, 2017

vstinner merged commit e72b135 into python:master Jul 21, 2017

vstinner deleted the importlib branch July 21, 2017 11:00

bedevere-bot removed the needs backport to 3.6 label Jul 21, 2017

		@@ -957,13 +954,16 @@ def _find_and_load_unlocked(name, import_):
		return module


		_SENTINEL = object()

Uh oh!

bpo-30891: importlib _find_and_load() try/except #2665

bpo-30891: importlib _find_and_load() try/except #2665

Uh oh!

Conversation

vstinner commented Jul 11, 2017

Uh oh!

mention-bot commented Jul 11, 2017

Uh oh!

vstinner commented Jul 11, 2017

Uh oh!

vstinner commented Jul 11, 2017

Uh oh!

serhiy-storchaka commented Jul 11, 2017

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Jul 11, 2017

Uh oh!

ncoghlan commented Jul 11, 2017

Uh oh!

serhiy-storchaka commented Jul 11, 2017

Uh oh!

vstinner commented Jul 11, 2017

Uh oh!

ncoghlan Jul 11, 2017

Choose a reason for hiding this comment

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jul 11, 2017

Uh oh!

serhiy-storchaka commented Jul 11, 2017

Uh oh!

ncoghlan commented Jul 11, 2017

Uh oh!

vstinner commented Jul 11, 2017

Uh oh!

serhiy-storchaka Jul 12, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner Jul 21, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jul 21, 2017

Uh oh!

ncoghlan commented Jul 21, 2017

Uh oh!

bedevere-bot commented Jul 21, 2017

Uh oh!

serhiy-storchaka commented Jul 21, 2017

Uh oh!

Uh oh!

serhiy-storchaka commented Jul 21, 2017 •

edited

Loading