GH-114781 potentially breaks gevent: threading becomes pre-imported at startup

[navytux]

Bug report

Bug description:

Hello up there.

I've discovered that starting from CPython >= 3.11.10 gevent-based applications
become potentially broken because, in a venv with gevent installed, threading
module becomes pre-imported by python stdlib itself. As gevent injects its own
implementation of threading primitives, it helps to know that there are no
instances of C-implemented locks at the time of the monkey-patching: if there
are no such instances(*) the program is known to have only instances of
gevent-provided locks and it will not run into deadlock scenarios
described at e.g. gevent/gevent#1865 where the code
tries to take a lock, that lock turns to be standard thread lock, but there are
only greenlets running and there is no other real thread to release that lock.

So not having threading pre-imported at startup is very desirable property in
the context of gevent. For this reason gpython from pygolang actually verifies
that invariant and it is through which I've discovered the issue when testing
pygolang on py3.11.10:

(py311-venv) kirr@deca:~/src/tools/go/pygolang-master$ gpython
Traceback (most recent call last):
  File "/home/kirr/src/tools/go/py311-venv/bin/gpython", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/home/kirr/src/tools/go/pygolang-master/gpython/__init__.py", line 368, in main
    raise RuntimeError('gpython: internal error: the following modules are pre-imported, but must be not:'
RuntimeError: gpython: internal error: the following modules are pre-imported, but must be not:

        ['threading']

sys.modules:

        ['__editable___pygolang_0_1_finder', '__future__', '__main__', '_abc', '_codecs', '_collections', '_collections_abc', '_distutils_hack', '_frozen_importlib', '_frozen_importlib_external', '_functools', '_imp', '_io', '_operator', '_signal', '_sitebuiltins', '_sre', '_stat', '_thread', '_warnings', '_weakref', '_weakrefset', 'abc', 'builtins', 'codecs', 'collections', 'contextlib', 'copyreg', 'encodings', 'encodings.aliases', 'encodings.utf_8', 'enum', 'errno', 'fnmatch', 'functools', 'genericpath', 'gpython', 'importlib', 'importlib._abc', 'importlib._bootstrap', 'importlib._bootstrap_external', 'importlib.machinery', 'importlib.util', 'io', 'ipaddress', 'itertools', 'keyword', 'marshal', 'ntpath', 'operator', 'os', 'os.path', 'pathlib', 'posix', 'posixpath', 're', 're._casefix', 're._compiler', 're._constants', 're._parser', 'reprlib', 'site', 'stat', 'sys', 'threading', 'time', 'types', 'urllib', 'urllib.parse', 'warnings', 'zipimport', 'zope']

The problem is there because importlib.util started to import threading after
46f821d62b5a, and because setuptools
emits importlib.util usage in installed *-nspkg.pth and in generated finders for packages installed in editable mode:

https://github.com/pypa/setuptools/blob/92b45e9817ae829a5ca5a5962313a56b943cad91/setuptools/namespaces.py#L46-L61
https://github.com/pypa/setuptools/blob/92b45e98/setuptools/command/editable_wheel.py#L786-L790

So for example the following breaks because e.g. zope.event is installed via nspkg way:

kirr@deca:~/tmp/trashme/X$ python3.12 -m venv 1.venv
kirr@deca:~/tmp/trashme/X$ . 1.venv/bin/activate
(1.venv) kirr@deca:~/tmp/trashme/X$ pip list
Package Version
------- -------
pip     24.0

(1.venv) kirr@deca:~/tmp/trashme/X$ pip install zope.event
Collecting zope.event
  Using cached zope.event-5.0-py3-none-any.whl.metadata (4.4 kB)
Collecting setuptools (from zope.event)
  Using cached setuptools-69.5.1-py3-none-any.whl.metadata (6.2 kB)
Using cached zope.event-5.0-py3-none-any.whl (6.8 kB)
Using cached setuptools-69.5.1-py3-none-any.whl (894 kB)
Installing collected packages: setuptools, zope.event
Successfully installed setuptools-69.5.1 zope.event-5.0

(1.venv) kirr@deca:~/tmp/trashme/X$ python -c 'import sys; assert "threading" not in sys.modules, sys.modules'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
AssertionError: {'threading': <module 'threading' ...>, 'importlib.util': <module 'importlib.util' (frozen)>, ...}

So would you please consider applying the following suggested patch to fix this problem:

--- a/Lib/importlib/util.py
+++ b/Lib/importlib/util.py
@@ -15,7 +15,7 @@
 import _imp
 import functools
 import sys
-import threading
+#import threading   delayed to avoid pre-importing threading early at startup
 import types
 import warnings
 
@@ -316,7 +316,7 @@ def exec_module(self, module):
         loader_state = {}
         loader_state['__dict__'] = module.__dict__.copy()
         loader_state['__class__'] = module.__class__
-        loader_state['lock'] = threading.RLock()
+        loader_state['lock'] = __import__('threading').RLock()
         loader_state['is_loading'] = False
         module.__spec__.loader_state = loader_state
         module.__class__ = _LazyModule

?

Thanks beforehand,
Kirill

/cc @effigies, @vstinner, @ericsnowcurrently, @doko42, @brettcannon
/cc @jamadden, @arcivanov, @maciejp-ro
/cc @eduardo-elizondo, @wilson3q, @vsajip

(*) or the list of C-implemented lock instances is limited and well defined -
for example gevent reinstantiates thrading._active_limbo_lock on the
monkey-patching.

CPython versions tested on:

3.11, 3.12, CPython main branch

Operating systems tested on:

Linux

Linked PRs

• gh-117983: Defer import of threading for lazy module loading #120233
• [3.13] gh-117983: Defer import of threading for lazy module loading (GH-120233) #121349
• [3.12] gh-117983: Defer import of threading for lazy module loading (GH-120233) #121350

[jaraco]

I note that zope is attempting to get away from pkg_resources-based namespace packages, which would bypass this issue, but only until another package came along that imported importlib.util early.

It seems not unreasonable to me to defer the import of threading to facilitate gevent.

[effigies]

Just a quick note that I had imported threading locally in the first implementation, but committed 57fe084 in response to #114781 (comment).

Trivially revertible, and it does still seem cleaner to me not to import threading unless lazy loading is actually used.

[vstinner]

Python doesn't support monkey patching on the standard library. This change has an impact on a core feature of Python, imports. I don't think that it's reasonable.

gevent should adapt its monkey patching mecanism for latest importlib changes.

[jaraco]

Is it a negative impact on a core feature? Doesn't deferring the import of 'threading' actually improve import times of importlib.util? We've been doing a lot of deferred imports for performance reasons. What is the downside to deferring the threading import?

[vstinner]

What is the downside to deferring the threading import?

It can slow down exec_module(). Also, it's kind of weird that executing a module during an import will import indirectly another import :-( The threading module is special and is injected as part of the importlib bootstrap process.

[navytux]

@jaraco, @effigies, @vstinner, thanks for feedback.

@vstinner, I can understand your arguments, and that was the reason I was hesitant to open this issue in the first place.

But also: here we are talking about LazyLoader - the thing that is intentionally deferring loading something:

cpython/Lib/importlib/util.py

Lines 236 to 272 in 17ed54b

	class LazyLoader(Loader):
	"""A loader that creates a module which defers loading until attribute access."""
	@staticmethod
	def __check_eager_loader(loader):
	if not hasattr(loader, 'exec_module'):
	raise TypeError('loader must define exec_module()')
	@classmethod
	def factory(cls, loader):
	"""Construct a callable which returns the eager loader made lazy."""
	cls.__check_eager_loader(loader)
	return lambda *args, **kwargs: cls(loader(*args, **kwargs))
	def __init__(self, loader):
	self.__check_eager_loader(loader)
	self.loader = loader
	def create_module(self, spec):
	return self.loader.create_module(spec)
	def exec_module(self, module):
	"""Make the module load lazily."""
	module.__spec__.loader = self.loader
	module.__loader__ = self.loader
	# Don't need to worry about deep-copying as trying to set an attribute
	# on an object would have triggered the load,
	# e.g. ``module.__spec__.loader = None`` would trigger a load from
	# trying to access module.__spec__.
	loader_state = {}
	loader_state['__dict__'] = module.__dict__.copy()
	loader_state['__class__'] = module.__class__
	loader_state['lock'] = threading.RLock()
	loader_state['is_loading'] = False
	module.__spec__.loader_state = loader_state
	module.__class__ = _LazyModule

From this point of view to me it is ok to say that the functionality, that only that LazyLoader uses, is reasonable to import only when it is actually needed the first time. No?

Sure, gevent needs can be smashed very easily with the argument "python does not care about gevent monkey patching at all". If the level of required cooperation from the python side would be significant, that argument would indeed make sense, but here we are talking about insignificant amount of changes and it is really the attitude, not technical bits, which becomes decision driver.

Even though "python way" is all asyncio this days, there are still many users of gevent out there. It would be good if the needs of those python+gevent users are also heard.

Kirill

[navytux]

P.S. I have no problem with running v3.11.9-4-g3bc0d2b851b with my suggested patch locally: make test passes fully and every other application that I use also work ok.

[ericsnowcurrently]

I might have suggested messing around with .pth files, but adding a new one won't necessarily help since they are loaded in the order they are identified by os.listdir(). Perhaps that could work via the "user site dir", but probably not. (I don't recall how the ordering works out in Lib/site.py, especially for venv.)

[serhiy-storchaka]

Threading support is only needed in multithreaded program. What if make importlib.util checking whether threading has been imported and only use locks if it was imported? importlib.util could use already imported threading if it was imported and not import it itself.

[effigies]

I believe this example would fail if you did that:

import lazy_loader as lazy  # Assume `lazy_loader` does not import threading itself

np = lazy.load('numpy')

import threading

def trigger():
    return np.__version__

def race():
    for _ in range(10):
        threading.Thread(target=trigger).start()

race()

(The most recent version of lazy_loader does in fact import threading, but there are many examples of ad hoc implementations based on the importlib.utils docs that do not import threading.)

The realistic scenario I see that would make conditionally using locks a problem would be a project that internally uses lazy loading to make its full API available with a single top-level import. If that gets imported before threading, we would be reintroducing potential races that are currently guarded against.

[effigies]

With another round of releases coming out, I feel like this needs resolution.

I understand that monkey-patching the stdlib is unsupported, but this feels like a weird hill to die on, given that deferred imports are accepted practice elsewhere (e.g., #114664):

My criterion for delayed imports is that they're only worth it if the majority of users of the module would benefit from it, otherwise you're just moving latency around unpredictably.

As the person who added the threading import, I can confidently assert that this is an unnecessary import in all cases that do not involve lazy loading. Given that lazy loading is not used in the stdlib, and there are many cases where a user cannot preempt importlib.util from loading, the majority of users will benefit from not adding an obligatory threading import.

On the other hand, lazy loading is used when there is a performance gain to deferring the third-party module load, and slightly adding latency to the first lazy-load simply adjusts the calculation of whether a module is worth loading eagerly or lazily. If the difference of an extra import threading shifts that balance, the difference probably doesn't matter.

I will open a PR that I hope will be backported, and I would ask the core developers to close this issue along with that PR if there is no intention of accepting a patch.

[brettcannon]

All merged!

[brettcannon]

And thanks to @effigies for staying on top of this!

[navytux]

@brettcannon, thanks a lot for merging the fixes, and @effigies, thanks, once again, for improving things here.

I see that the patch landed to 3.12, 3.13 and main, but, as original description of the issue explains, the bug was introduced in 3.11.10, so is there a possibility to backport to 3.11 as well?

[vsajip]

Probably not, as versions 3.11 and earlier receive security fixes only.

[navytux]

@vsajip, thanks for feedback. I understand your reasoning, but it was a regression introduced in 3.11.10. From this point of view this patch is not a new addition, but reverts that regression back. In other words it does not add something new, but removes what was added in a recent point release and found to cause problems. For me it goes as common sense to apply the fix to 3.11.x as well, because else 3.11 leaves the whole gevent ecosystem in broken state.

[effigies]

#120233 (comment)

[jaraco]

@vsajip, thanks for feedback. I understand your reasoning, but it was a regression introduced in 3.11.10. From this point of view this patch is not a new addition, but reverts that regression back. In other words it does not add something new, but removes what was added in a recent point release and found to cause problems. For me it goes as common sense to apply the fix to 3.11.x as well, because else 3.11 leaves the whole gevent ecosystem in broken state.

@pablogsal Would you consider this request to backport to 3.11 one more time in light of this additional information (that the downstream regression was introduced in a security-fix release)? (apologies if that was already a factor in your consideration)

[brettcannon]

@jaraco the change was actually made while 3.11 was open to bugfixes: 46f821d was committed on Feb 26 and Python 3.11 entered security-only on April 2 w/ the 3.11.9 release.