Don't preimport threading early #1897

navytux · 2020-07-14T17:22:14Z

Gevent-based applications need to be able to import threading, socket,
ssl, etc... modules early to be able to monkey-patch them to use
lightweight greenlets instead of OS threads. This patching has to be
done as early as possible in the lifetime of the process

http://www.gevent.org/intro.html#monkey-patching

However starting from caae48b (use import hooks to patch
distutils/setuptools (#1688)) threading became always preimported which,
for example, rendered gpython[1] unusable:

(py38.virtualenv) kirr@deco:~/tmp/trashme$ gpython
Traceback (most recent call last):
  File "/home/kirr/tmp/trashme/py38.virtualenv/bin/gpython", line 8, in <module>
    sys.exit(main())
  File "/home/kirr/tmp/trashme/py38.virtualenv/lib/python3.8/site-packages/gpython/__init__.py", line 151, 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:

        ['__future__', '__main__', '_abc', '_bootlocale', '_codecs', '_collections', '_collections_abc', '_frozen_importlib', '_frozen_importlib_external', '_functools', '_heapq', '_imp', '_io', '_locale', '_operator', '_signal', '_sitebuiltins', '_sre', '_stat', '_thread', '_virtualenv', '_warnings', '_weakref', '_weakrefset', 'abc', 'builtins', 'codecs', 'collections', 'contextlib', 'copyreg', 'encodings', 'encodings.aliases', 'encodings.latin_1', 'encodings.utf_8', 'enum', 'functools', 'genericpath', 'gpython', 'heapq', 'importlib', 'importlib._bootstrap', 'importlib._bootstrap_external', 'importlib.abc', 'importlib.machinery', 'importlib.util', 'io', 'itertools', 'keyword', 'marshal', 'operator', 'os', 'os.path', 'posix', 'posixpath', 're', 'reprlib', 'site', 'sitecustomize', 'sre_compile', 'sre_constants', 'sre_parse', 'stat', 'sys', 'threading', 'time', 'types', 'warnings', 'zipimport', 'zope']

Fix it by importing threading lazily.

Fixes: #1895

[1] https://pypi.org/project/pygolang/#gpython

Thanks for contributing, make sure you address all the checklists (for details on how see

development documentation)!

ran the linter to address style issues (tox -e fix_lint)
wrote descriptive pull request text
ensured there are test(s) validating the fix
added news fragment in docs/changelog folder
updated/extended the documentation (no need for this PR)

Gevent-based applications need to be able to import threading, socket, ssl, etc... modules early to be able to monkey-patch them to use lightweight greenlets instead of OS threads. This patching has to be done as early as possible in the lifetime of the process http://www.gevent.org/intro.html#monkey-patching However starting from caae48b (use import hooks to patch distutils/setuptools (pypa#1688)) threading became always preimported which, for example, rendered gpython[1] unusable: (py38.virtualenv) kirr@deco:~/tmp/trashme$ gpython Traceback (most recent call last): File "/home/kirr/tmp/trashme/py38.virtualenv/bin/gpython", line 8, in <module> sys.exit(main()) File "/home/kirr/tmp/trashme/py38.virtualenv/lib/python3.8/site-packages/gpython/__init__.py", line 151, 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: ['__future__', '__main__', '_abc', '_bootlocale', '_codecs', '_collections', '_collections_abc', '_frozen_importlib', '_frozen_importlib_external', '_functools', '_heapq', '_imp', '_io', '_locale', '_operator', '_signal', '_sitebuiltins', '_sre', '_stat', '_thread', '_virtualenv', '_warnings', '_weakref', '_weakrefset', 'abc', 'builtins', 'codecs', 'collections', 'contextlib', 'copyreg', 'encodings', 'encodings.aliases', 'encodings.latin_1', 'encodings.utf_8', 'enum', 'functools', 'genericpath', 'gpython', 'heapq', 'importlib', 'importlib._bootstrap', 'importlib._bootstrap_external', 'importlib.abc', 'importlib.machinery', 'importlib.util', 'io', 'itertools', 'keyword', 'marshal', 'operator', 'os', 'os.path', 'posix', 'posixpath', 're', 'reprlib', 'site', 'sitecustomize', 'sre_compile', 'sre_constants', 'sre_parse', 'stat', 'sys', 'threading', 'time', 'types', 'warnings', 'zipimport', 'zope'] Fix it by importing threading lazily. Fixes: pypa#1895 [1] https://pypi.org/project/pygolang/#gpython

AttributeError("'PosixPath' object has no attribute 'rfind'",)

src/virtualenv/create/via_global_ref/_virtualenv.py

tests/unit/create/test_creator.py

docs/changelog/1897.bugfix.rst

Address review feedback: - wrap comments at 120 characters; - use "imported" instead of "preimport" in tests and leverage python syntax sugar to raise signal/noise; - inline links in changelog Try to fix test failures: - try to disable coverage for spawned subrocess. However in the presence of coverage-enable-subprocess coverge is still unconditioanlly imported, and even plain `import coverage` leads to `import threading`. Probably need to fix coverage as well.

navytux · 2020-07-14T19:42:39Z

@gaborbernat thanks for feedback. I've amended the change according to your review comments.

There are also test failures - locally it works for me if I run the test via just py.test, but it does not work if I run the tests via tox. Tox runs runs py.test via coverage. I've traced this to the fact that even plain import coverage currently leads to import threading despite the fact that coverage cares not to import threading unneccessarily for exactly the same reason as this patch:

https://github.com/nedbat/coveragepy/blob/40df2bf9d0996844185d96500739cd7ecebd2505/coverage/collector.py#L123-L137

Probably coverage and/or coverage-enable-subprocess will need to be fixed as well.

gaborbernat · 2020-07-14T19:52:01Z

Probably coverage and/or coverage-enable-subprocess will need to be fixed as well.

Let's just disable coverage tracking for this one test, you can do this here https://github.com/pypa/virtualenv/blob/master/tests/conftest.py#L191, probably the easiest is to add a no_coverage marker onto the test, and do not enable coverage tracking if the request param of the fixture has that marker.

@gaborbernat

Disable coverage for the test as suggested by @gaborbernat.

navytux · 2020-07-14T21:10:20Z

Thanks. I've amended the change accordingly.

codecov · 2020-07-14T21:13:48Z

Codecov Report

Merging #1897 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1897      +/-   ##
==========================================
- Coverage   94.00%   93.95%   -0.05%     
==========================================
  Files          86       86              
  Lines        4220     4220              
==========================================
- Hits         3967     3965       -2     
- Misses        253      255       +2

Flag	Coverage Δ
#tests	`93.95% <ø> (-0.05%)`	⬇️

Impacted Files	Coverage Δ
.../create/via_global_ref/builtin/cpython/cpython3.py	`95.65% <0.00%> (-2.18%)`	⬇️
src/virtualenv/seed/embed/base_embed.py	`96.22% <0.00%> (-1.89%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16f80ac...1398a9d. Read the comment docs.

navytux · 2020-07-14T21:43:08Z

Most test pass except pypy3 on macos. I'll try to look into it next time I have time.

gaborbernat · 2020-07-15T09:19:21Z

Most test pass except pypy3 on macos. I'll try to look into it next time I have time.

Most likely pypy3 itself imports those automatically at startup (check it's site.py).

jamadden · 2020-07-15T10:14:48Z

Most likely pypy3 itself imports those automatically at startup (check it's site.py).

It does, albeit not directly.

$ pypy3 -I
Python 3.6.9 (2ad108f17bdb, Apr 07 2020, 02:59:26)
[PyPy 7.3.1 with GCC 4.2.1 Compatible Apple LLVM 11.0.3 (clang-1103.0.32.29)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>>> import sys
>>>> 'threading' in sys.modules
True

There's no mention of threading in site.py, so I tweaked threading.py to print when it is imported and then ran pypy3 -I again. That gives the chain site -> sysconfig -> _sysconfigdata -> platform -> subprocess -> threading.

  File "<frozen importlib._bootstrap>", line 1083, in __import__
  ...
  File "//pypy3/lib-python/3/site.py", line 564, in <module>
    main()
  File "//pypy3/lib-python/3/site.py", line 550, in main
    known_paths = addusersitepackages(known_paths)
  File "//pypy3/lib-python/3/site.py", line 284, in addusersitepackages
    user_site = getusersitepackages()
  File "//pypy3/lib-python/3/site.py", line 260, in getusersitepackages
    user_base = getuserbase() # this will also set USER_BASE
  File "//pypy3/lib-python/3/site.py", line 250, in getuserbase
    USER_BASE = get_config_var('userbase')
  File "//pypy3/lib-python/3/sysconfig.py", line 629, in get_config_var
    return get_config_vars().get(name)
  File "//pypy3/lib-python/3/sysconfig.py", line 575, in get_config_vars
    _init_posix(_CONFIG_VARS)
  File "//pypy3/lib-python/3/sysconfig.py", line 439, in _init_posix
    _temp = __import__(name, globals(), locals(), ['build_time_vars'], 0)
  ...            
  File "//pypy3/lib_pypy/_sysconfigdata.py", line 42, in <module>
    import platform
  ...
  File "//pypy3/lib-python/3/platform.py", line 116, in <module>
    import sys, os, re, subprocess
  ...
  File "//pypy3/lib-python/3/subprocess.py", line 140, in <module>
    import threading

gaborbernat · 2020-07-15T12:02:42Z

Can you open an issue to report this to https://foss.heptapod.net/pypy/pypy/-/issues, and skip this test for pypy otherwise?

navytux · 2020-07-15T12:55:08Z

@gaborbernat, @jamadden, thanks. I've filed https://foss.heptapod.net/pypy/pypy/-/issues/3269. Let me try to add corresponding xfail.

Xfail on pypy3/darwin. See reason link for details.

navytux · 2020-07-15T14:39:59Z

Thank you, @gaborbernat.

gaborbernat · 2020-07-15T15:49:52Z

released via https://pypi.org/project/virtualenv/20.0.27/

navytux · 2020-07-15T16:57:24Z

Thanks

navytux mentioned this pull request Jul 14, 2020

virtualenv generates python that pre-imports threading (16.x -> 20.x regression) #1895

Closed

fixup! Don't preimport threading early

74e8e4d

AttributeError("'PosixPath' object has no attribute 'rfind'",)

gaborbernat requested changes Jul 14, 2020

View reviewed changes

src/virtualenv/create/via_global_ref/_virtualenv.py Outdated Show resolved Hide resolved

tests/unit/create/test_creator.py Outdated Show resolved Hide resolved

docs/changelog/1897.bugfix.rst Outdated Show resolved Hide resolved

fixup! Don't preimport threading early

44ace84

Disable coverage for the test as suggested by @gaborbernat.

fixup! Don't preimport threading early

1398a9d

Xfail on pypy3/darwin. See reason link for details.

gaborbernat approved these changes Jul 15, 2020

View reviewed changes

gaborbernat merged commit cd17f30 into pypa:master Jul 15, 2020

navytux deleted the y/no-threading branch July 15, 2020 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't preimport threading early #1897

Don't preimport threading early #1897

navytux commented Jul 14, 2020

navytux commented Jul 14, 2020

gaborbernat commented Jul 14, 2020

navytux commented Jul 14, 2020

codecov bot commented Jul 14, 2020 •

edited

Loading

navytux commented Jul 14, 2020

gaborbernat commented Jul 15, 2020

jamadden commented Jul 15, 2020

gaborbernat commented Jul 15, 2020

navytux commented Jul 15, 2020

navytux commented Jul 15, 2020

gaborbernat commented Jul 15, 2020

navytux commented Jul 15, 2020

Don't preimport threading early #1897

Don't preimport threading early #1897

Conversation

navytux commented Jul 14, 2020

Thanks for contributing, make sure you address all the checklists (for details on how see

navytux commented Jul 14, 2020

gaborbernat commented Jul 14, 2020

navytux commented Jul 14, 2020

codecov bot commented Jul 14, 2020 • edited Loading

Codecov Report

navytux commented Jul 14, 2020

gaborbernat commented Jul 15, 2020

jamadden commented Jul 15, 2020

gaborbernat commented Jul 15, 2020

navytux commented Jul 15, 2020

navytux commented Jul 15, 2020

gaborbernat commented Jul 15, 2020

navytux commented Jul 15, 2020

codecov bot commented Jul 14, 2020 •

edited

Loading