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

msvccompiler spawn is not threadsafe #5

Closed
jaraco opened this issue Jul 12, 2020 · 1 comment
Closed

msvccompiler spawn is not threadsafe #5

jaraco opened this issue Jul 12, 2020 · 1 comment

Comments

@jaraco
Copy link
Member

jaraco commented Jul 12, 2020

In pypa/setuptools#2212, we learned that in the spawn operation, distutils will mutate global state, a concurrency-unsafe operation leading to a race condition.

@jaraco
Copy link
Member Author

jaraco commented Jul 12, 2020

I started working on a test for this issue, but discovered that the module can only be imported on Windows, so I'm giving up on writing a test. Here's what I'd drafted:

diff --git a/distutils/tests/test_msvccompiler.py b/distutils/tests/test_msvccompiler.py
index b518d6a78b..6c07812342 100644
--- a/distutils/tests/test_msvccompiler.py
+++ b/distutils/tests/test_msvccompiler.py
@@ -2,6 +2,7 @@
 import sys
 import unittest
 import os
+import threading
 
 from distutils.errors import DistutilsPlatformError
 from distutils.tests import support
@@ -74,6 +75,27 @@ class msvccompilerTestCase(support.TempdirManager,
         else:
             raise unittest.SkipTest("VS 2015 is not installed")
 
+
+class TestSpawn(unittest.TestCase):
+    def test_concurrent_safe(self):
+        """
+        Concurrent calls to spawn should have consistent results.
+        """
+        import distutils._msvccompiler as _msvccompiler
+        compiler = _msvccompiler.MSVCCompiler()
+        compiler._paths = "expected"
+        inner_cmd = 'import os; assert os.environ["PATH"] == "expected"'
+        command = ['python', '-c', inner_cmd]
+        threads = [
+            threading.Thread(target=compiler.spawn, args=[command])
+            for n in range(100)
+        ]
+        for thread in threads:
+            thread.start()
+        for thread in threads:
+            thread.join()
+
+
 def test_suite():
     return unittest.makeSuite(msvccompilerTestCase)
 

@jaraco jaraco closed this as completed in 360aadc Jul 12, 2020
clrpackages pushed a commit to clearlinux-pkgs/openstack-setuptools that referenced this issue Jul 14, 2020
…0 to version 49.2.0

Alex Henrie (1):
      Change exec_module to load_module

Hai Shi (2):
      bpo-40275: Use new test.support helper submodules in tests (GH-21151)
      bpo-40275: Use new test.support helper submodules in tests (GH-21317)

Hugo (1):
      Test Python 3.9-dev

Hugo van Kemenade (1):
      Test Python 3.9-dev

Jason R. Coombs (26):
      Amend changelog for 48.0 to include more detail about usage expectations. Ref #2230.
      Use lowercase 't' for consistency in branding.
      Rename logo assets to remove project name and 'logo', which are implied by the context.
      Render logo in the readme.
      Add banner to main docs page
      Remove stale description of packaging.
      Add test for spawn when exe is missing. Ref pypa/distutils#3.
      Replace OSError with DistutilsExecError. Fixes pypa/distutils#3 and pypa/setuptools#2228 and bpo-41207.
      Update changelog. Ref #2228.
      Bump version: 49.0.0 → 49.0.1
      Move assert outside the context so it actually has its effect.
      Remove py2_warn, no longer needed as a SyntaxError is encountered before the warning can be issueed.
      bpo-41207 In distutils.spawn, rewrite FileNotFound (GH-21359)
      Update changelog.
      Bump version: 49.1.0 → 49.1.1
      Provide escape hatch for distutils adoption.
      Allow opt-in and opt-out of distutils adoption at run time with SETUPTOOLS_USE_DISTUTILS environment variable.
      Bump version: 49.1.1 → 49.1.2
      Add a simple blank issue so it gets a green button.
      Update changelog.
      Allow spawn to accept environment. Avoid monkey-patching global state. Closes pypa/setuptools#2212 and closes pypa/distutils#5.
      Update changelog.
      Bump version: 49.1.2 → 49.1.3
      Warn the user when distutils is present to discourage this usage and direct users to the recommended usage. Closes #2230.
      Programmatically disable coverage when running on PyPy.
      Bump version: 49.1.3 → 49.2.0

Serhiy Storchaka (1):
      bpo-41043: Escape literal part of the path for glob(). (GH-20994)

Victor Stinner (1):
      bpo-41003: Fix test_copyreg when numpy is installed (GH-20935)

cajhne (1):
      Add logo resources
clrpackages pushed a commit to clearlinux-pkgs/setuptools that referenced this issue Jul 14, 2020
…on 49.2.0

Alex Henrie (1):
      Change exec_module to load_module

Hugo (1):
      Test Python 3.9-dev

Hugo van Kemenade (1):
      Test Python 3.9-dev

Jason R. Coombs (11):
      Provide escape hatch for distutils adoption.
      Allow opt-in and opt-out of distutils adoption at run time with SETUPTOOLS_USE_DISTUTILS environment variable.
      Bump version: 49.1.1 → 49.1.2
      Add a simple blank issue so it gets a green button.
      Update changelog.
      Allow spawn to accept environment. Avoid monkey-patching global state. Closes pypa/setuptools#2212 and closes pypa/distutils#5.
      Update changelog.
      Bump version: 49.1.2 → 49.1.3
      Warn the user when distutils is present to discourage this usage and direct users to the recommended usage. Closes #2230.
      Programmatically disable coverage when running on PyPy.
      Bump version: 49.1.3 → 49.2.0
clrpackages pushed a commit to clearlinux-pkgs/setuptools that referenced this issue Aug 5, 2020
…on 49.2.1

Hugo van Kemenade (1):
      The PyPA has adopted the PSF code of conduct

Jason R. Coombs (12):
      Add docs on porting from distutils.
      Revert "Render logo in the readme."
      Revert "Rename logo assets to remove project name and 'logo', which are implied by the context."
      Revert "Merge pull request #2229 from cajhne/logo001"
      Revert "Add banner to main docs page"
      Suppress ImportError for winreg as the module is only available on some platforms. Allows unit testing of module on non-Windows platforms.
      Add a unit test for testing spawn. Ref pypa/distutils#5.
      In TestSpawn.test_concurrent_safe, use CheckThread to ensure that the spawn call does not simply fail to execute. Ref pypa/setuptools#2257.
      In CCompiler, allow keyword arguments to be passed to spawn calls. Ref pypa/setuptools#2257 and pypa/distutils#5.
      In _msvccompiler.MSVCCompiler.spawn, use correct capitalization for PATH environment variable. Fixes failing test and fixes pypa/setuptools#2257.
      Add changelog. Ref #2257.
      Bump version: 49.2.0 → 49.2.1

Paul Ganssle (1):
      Remove issue templates
clrpackages pushed a commit to clearlinux-pkgs/openstack-setuptools that referenced this issue Aug 5, 2020
…0 to version 49.2.1

Hugo van Kemenade (1):
      The PyPA has adopted the PSF code of conduct

Jason R. Coombs (12):
      Add docs on porting from distutils.
      Revert "Render logo in the readme."
      Revert "Rename logo assets to remove project name and 'logo', which are implied by the context."
      Revert "Merge pull request #2229 from cajhne/logo001"
      Revert "Add banner to main docs page"
      Suppress ImportError for winreg as the module is only available on some platforms. Allows unit testing of module on non-Windows platforms.
      Add a unit test for testing spawn. Ref pypa/distutils#5.
      In TestSpawn.test_concurrent_safe, use CheckThread to ensure that the spawn call does not simply fail to execute. Ref pypa/setuptools#2257.
      In CCompiler, allow keyword arguments to be passed to spawn calls. Ref pypa/setuptools#2257 and pypa/distutils#5.
      In _msvccompiler.MSVCCompiler.spawn, use correct capitalization for PATH environment variable. Fixes failing test and fixes pypa/setuptools#2257.
      Add changelog. Ref #2257.
      Bump version: 49.2.0 → 49.2.1

Paul Ganssle (1):
      Remove issue templates
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

No branches or pull requests

1 participant