GeezerVison is showing github.com — GH-114781 potentially breaks gevent: threading becomes pre-imported at startup · Issue #117983 · python/cpython
GH-114781 potentially breaks gevent: threading becomes pre-imported at startup #117983DescriptionBug 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
Reacted by Arcadiy Ivanov, Filipe Fernandes and Vitaliy Yelnik Activityjaraco commented on Apr 17, 2024 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 commented on Apr 17, 2024 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 commented on Apr 17, 2024 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 commented on Apr 17, 2024 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? Reacted by Arcadiy Ivanov and Filipe Fernandes vstinner commented on Apr 17, 2024
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. Reacted by Jason R. Coombs navytux commented on Apr 18, 2024 @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:
|
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 Reacted by Jason R. Coombs, Chris Markiewicz, Filipe Fernandes, Vitaliy Yelnik and Dmitry C navytux commented on Apr 18, 2024 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 commented on Apr 18, 2024 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 commented on Apr 23, 2024 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 commented on Apr 23, 2024 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 commented on Jun 7, 2024 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. Reacted by Kirill Smelkov, Filipe Fernandes, Vitaliy Yelnik, Craig Blaszczyk and John Lucas brettcannon commented on Jul 3, 2024 brettcannon commented on Jul 3, 2024 And thanks to @effigies for staying on top of this! navytux commented on Jul 4, 2024 @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 commented on Jul 4, 2024 Probably not, as versions 3.11 and earlier receive security fixes only. navytux commented on Jul 4, 2024 @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 commented on Jul 4, 2024 jaraco commented on Jul 4, 2024
@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 commented on Jul 4, 2024 @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.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|