Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't preimport threading early #1897

Merged
merged 5 commits into from Jul 15, 2020
Merged

Conversation

navytux
Copy link
Contributor

@navytux navytux commented Jul 14, 2020

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 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
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
Copy link
Contributor Author

navytux commented Jul 14, 2020

@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
Copy link
Contributor

gaborbernat commented Jul 14, 2020

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.

Disable coverage for the test as suggested by @gaborbernat.
@navytux
Copy link
Contributor Author

navytux commented Jul 14, 2020

Thanks. I've amended the change accordingly.

@codecov
Copy link

codecov bot commented Jul 14, 2020

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor Author

navytux commented Jul 14, 2020

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

@gaborbernat
Copy link
Contributor

gaborbernat commented Jul 15, 2020

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
Copy link
Member

jamadden commented Jul 15, 2020

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
Copy link
Contributor

gaborbernat commented Jul 15, 2020

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

@navytux
Copy link
Contributor Author

navytux commented Jul 15, 2020

@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.
@gaborbernat gaborbernat merged commit cd17f30 into pypa:master Jul 15, 2020
36 of 37 checks passed
@navytux
Copy link
Contributor Author

navytux commented Jul 15, 2020

Thank you, @gaborbernat.

@navytux navytux deleted the y/no-threading branch Jul 15, 2020
@gaborbernat
Copy link
Contributor

gaborbernat commented Jul 15, 2020

@navytux
Copy link
Contributor Author

navytux commented Jul 15, 2020

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants