Fix threading issues with register_post_import_hook

[eltoder]

1. Add a missing lock to ImportHookFinder.find_spec(). It should have the same lock as find_module() since it accesses _post_import_hooks.

2. Fix a deadlock between notify_module_loaded() and ImportHookFinder. This occurs because the former is calling import hooks under _post_import_hooks_lock. If a hook is making any imports itself while another thread is also making imports, the thread can grab the import lock and wait for _post_import_hooks_lock in ImportHookFinder, while the hook already has _post_import_hooks_lock and is waiting for the import lock. This results in a deadlock. The solution is to call the hooks outside of _post_import_hooks_lock.

Same for register_post_import_hook() and ImportHookFinder.

3. Also fix an issue if register_post_import_hook() was called after a module was already imported but was later removed from sys.modules. This also makes the logic simpler.

[GrahamDumpleton]

Thanks. I will have a look. I need to think through what implications are of moving the lock to the with statement rather than being on the function as a whole.

[eltoder]

Hi @GrahamDumpleton. Did you have a chance to look?

[GrahamDumpleton]

Sorry, been really caught up in work. I'll see if can set aside some time this weekend to look at it. I also need to get a mod_wsgi release out as well. :-(

[GrahamDumpleton]

I am not sure this is thread safe any more. A few comments below.

In:

def notify_module_loaded(module):
    name = getattr(module, '__name__', None)
    with _post_import_hooks_lock:
        hooks = _post_import_hooks.pop(name, ())

    # NOTE: Call hooks outside of the lock to avoid deadlocks.
    for hook in hooks:
        hook(module)

The with statement with lock here doesn't really serve any purpose as pop() should be guaranteed as atomic for builtin dictionary.

Similarly, where changed to:

        with _post_import_hooks_lock:
            if fullname not in _post_import_hooks:
                return None

The in keyword results in __contains__ being called, which for builtin dict should be atomic.

Removal of the lock on that whole function though means:

        if fullname in self.in_progress:
            return None

        self.in_progress[fullname] = True

is no longer protected. If it is technically possible for two separate imports of the same module to enter this at the same time there would be a race condition. The question though is whether the Python import lock would provide that protection or not, remembering that how the import lock has changed over time from being a single lock, to a lock per module (???) so it gets complicated.

Similar deal with both find_module and find_spec with that code.

So if it is reasonable to remove the lock from the whole function, the locking by the with statements is probably not needed, but then the in progress checks does need to be locked. For the latter it may make more sense to lock on the object instance rather than use the global scoped lock, although not essential.

        with synchronized(self):
            if fullname in self.in_progress:
                return None

            self.in_progress[fullname] = True

Overall am still worried about the changes as the locking was added at some point for a reason and am pretty sure it wasn't always this way. Problem there is that part of the code was based on work done in a separate package, but that other package since switched to wrapt, plus when they eventually made it open source they threw away all the history and started with only the most up to date copy, so the reasons for commits which added the locking is gone.

Anyway, that is first pass done. I will think about it some more and see if I can remember why locking was done the way it was.

[eltoder]

Hi @GrahamDumpleton,

Thank you for review. I make the following two claims:

1. Import finders are protected by the global import lock. You can see it in the code. I don't see this documented, but it's been like this since at least Python 3.1 where importlib._bootstrap was added. If I remember correctly, it was like this in Python 2 as well. Also, I've never seen any finder or loader implementation that used locks to protect its internal state. As examples here I'll cite the built-in zipimporter, the six's importer, and the pyximport from cython. So I claim that we don't need a lock to protect self.in_progress in ImportHookFinder. (And even if we did need a lock, we should not use the _post_import_hooks_lock for this.)
2. Thread safety with my change is strictly better than it was before. That is, it removes a very real deadlock that I found in a production system, and any race that exists with the change existed before it. (More on the races below.) If you can think of a counter-example, please let me know.

Also note that find_spec added in 53323ba4b92f88dab5a8ec3262a1c1f1e979a3e2 does not use any locks at all, so in every not ancient version of Python there's no locking on the importing side. But if you add a lock around the whole method like there was for find_module, you get the deadlock that I'm fixing in this change.

Two more notes:

1. While it is true that operations on built-in types are generally atomic in cpython, I don't know if this is the case in all other python implementations. As far as I know, this is not guaranteed by the language spec and relying on this is generally frowned upon. For example, see the Google's style guide. If you do want to rely on this, the lock is only required for adding ImportHookFinder into sys.meta_path. Everything else can be done with one method call.
2. There is indeed a race condition:
   • One thread is doing an import and already ran the if fullname not in _post_import_hooks check, which retuned False, but did not get to call the loader to create the module and put it into sys.modules.
   • Another thread is calling register_post_import_hook. The module is not in sys.modules so it adds the hook to _post_import_hooks, however the importing thread already did the check, so it is too late.

The problem is that there is a gap between the Finder and the Loader. No locks in the Finder can solve this. I think it's possible to solve by holding the per-module lock in register_post_import_hook using the _ModuleLockManager. Alas, _ModuleLockManager is a private API in importlib, but there is a dormant ticket to expose it.

Python 2 has a less granular import locking, but I think this race still exists. Even if the whole import sequence (finding and loading of the module) is protected by one uninterrupted lock, you still need to hold it in register_post_import_hook (i.e., you need to call imp.acquire_lock() and imp.release_lock()). The current code does not do this.

[GrahamDumpleton]

The dict.pop method has always been atomic.

• http://effbot.org/pyfaq/what-kinds-of-global-value-mutation-are-thread-safe.htm

The dict.setdefault method has been atomic since Python 3.2 and Python 2.7.3.

• https://hg.python.org/cpython/rev/5c52e7c6d868

These are fundamental guarantees and since CPython is the reference implementation other implementations should behave the same way.

Right now wrapt still works with Python 2.7, although there is no constraint as to what patch revision of Python 2.7 is required so if did make such a change to rely on atomic behaviour of dict.setdefault would need to change:

python_requires = >= 2.7, != 3.0.*, != 3.1.*, != 3.2.*, != 3.3.*, != 3.4.*

to:

python_requires = >= 2.7.4, != 3.0.*, != 3.1.*, != 3.2.*, != 3.3.*, != 3.4.*

Because another change queued up changes some behaviour of wrapt object proxy, next version would be 1.15.0 so I don't see an issue with constraining Python 2.7 requirement slightly, especially since people should be using an up to date patched version of Python 2.7 anyway.

[eltoder]

These are fundamental guarantees and since CPython is the reference implementation other implementations should behave the same way.

Raymond Hettinger disagrees :-)

http://effbot.org/pyfaq/what-kinds-of-global-value-mutation-are-thread-safe.htm

This clearly describes the cpython implementation, not the Python language, and even then it's not very accurate. A lot of things that are a single bytecode instruction call back into Python (because they allocate an object, call decref, call a python implementation of __hash__, __eq__, etc), which can switch threads. This takes an effort to handle and the result is not always atomic. For example, list.sort is not atomic.

But then again, it concludes with

When in doubt, use a mutex!

:-)

The dict.setdefault method has been atomic since Python 3.2 and Python 2.7.3.

I think the fact that people were relying on setdefault being atomic for 12 years before it actually became atomic is a good reason not to rely on undocumented behavior. But this is your project, so it is up to you. I'm happy to remove the lock.

[eltoder]

I think the fact that people were relying on setdefault being atomic for 12 years before it actually became atomic is a good reason not to rely on undocumented behavior. But this is your project, so it is up to you. I'm happy to remove the lock.

Actually, I think the old version of setdefault was already atomic for str keys, which is what we have. Only keys with python hash/eq had an issue. So if you want to rely on this, we don't even need to change python version in requirements.

[GrahamDumpleton]

I figured any literal values as key was fine, eg., string, bytes, int, float and maybe even tuple. Am still happy to go with the more conservative approach for now and keep the locking.

[eltoder]

Yes, exactly. Anything except python classes that implement __hash__ or __eq__ in python.

[GrahamDumpleton]

These changes have been merged into develop branch now. I did change the tests though to break them into separate tests and also avoid a test on Python 2.7 which would have still deadlocked due to the single Python module import lock that exists in that version, where as Python 3.X has module import locks per named module.