python / cpython

The Python programming language
https://www.python.org
Other
63.18k stars 30.25k forks source link

MSVCRT's spawnve/spawnvpe are not thread safe #50725

Open f748c18b-c532-4573-b247-db46997613cd opened 15 years ago

f748c18b-c532-4573-b247-db46997613cd commented 15 years ago
BPO 6476
Nosy @amauryfa, @pitrou
Files
  • spawnve.py: Failing sample with workaround
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['interpreter-core', 'type-bug'] title = "MSVCRT's spawnve/spawnvpe are not thread safe" updated_at = user = 'https://bugs.python.org/jfonseca' ``` bugs.python.org fields: ```python activity = actor = 'pitrou' assignee = 'none' closed = False closed_date = None closer = None components = ['Interpreter Core'] creation = creator = 'jfonseca' dependencies = [] files = ['14495'] hgrepos = [] issue_num = 6476 keywords = [] message_count = 6.0 messages = ['90495', '90498', '90503', '140574', '140632', '140634'] nosy_count = 5.0 nosy_names = ['amaury.forgeotdarc', 'pitrou', 'jfonseca', 'python-dev', 'steve_hill'] pr_nums = [] priority = 'normal' resolution = 'wont fix' stage = None status = 'open' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue6476' versions = ['Python 2.7', 'Python 3.2', 'Python 3.3'] ```

    f748c18b-c532-4573-b247-db46997613cd commented 15 years ago

    MSVCRT's implementation of _spawnve, _spawnvpe, or any version that takes an environ as paramater is not thread-safe, because it stores a temporary environment string into a global variable.

    _spawnve, _spawnvpe, and friends call a function named _cenvarg which concatenate the environment strings into a global variable called _aenvptr, which gets free'd and zero'd after each invocation.

    This was causing random build failures in scons when parallel build (-j) was enabled.

    The sample application evidences this problem. It also includes a simple workaround in python, by acquiring a global lock around os.spawnve, and simulating P_WAIT with P_NOWAIT to avoid holding the global lock while the child process is running. I believe something along these lines should be done for CPython on Windows.

    amauryfa commented 15 years ago

    Indeed. But shouldn't you use the subprocess module instead?

    f748c18b-c532-4573-b247-db46997613cd commented 15 years ago

    Perhaps. I'm not a scons developer -- just an user -- and I don't know what versions of python far back in time they want support, but it appears it would make sense to use subprocess where available indeed. I already I've filled an issue with scons at http://scons.tigris.org/issues/show_bug.cgi?id=2449 and linked back to this issue and I trust the developers to do whatever they see fit to address this problem.

    Instead of just marking this issue as won't fix, shouldn't at least some documentation be added, or something sensible like that? In http://docs.python.org/library/os.html#process-management os.spawn* are not marked as deprecated -- just says that "the subprocess module provides more powerful facilities" and is "preferable" --, and it certainly doesn't say these functions are not thread safe. It would be a pity if other users would have to invest as much time as I did to find this race condition (it was rare, and happened in a build farm so we couldn't see the windows access violation dialog), and multithreading is getting more and more common.

    Also, if the only reason not to fix this is the lack of a patch I don't mind in producing one FWIW.

    676131d1-1d86-4c78-ab02-82782749c1e3 commented 13 years ago

    Why has this bug been resolved as "won't fix"? It seems to me that this is a valid issue with something that has not been deprecated, yet it has been decided neither to fix it (despite there being an offer by the originator to submit a patch) nor even to document the deficiency.

    We've been using SCons for the last 3-4 years, during which time we have been plagued by this issue - more so as multi-core machines have become more prevalent. There was a thread on the SCons Users mailing list in March '09, which stopped short of diagnosing the problem and we've lived with it ever since - putting it down to Python being "a bit flaky". I now discover that it is an issue that has been diagnosed two years ago and deliberately left in the implementation of the language. Simply saying "you should use subprocess" is not helpful; SCons at that time was supporting all Python versions back to 2.0 (possibly earlier) so couldn't rely on the subprocess module being present.

    Ideally, it should be worked-around so that these functions can safely be used on all platforms without requiring mutual exclusion in the application. However, it seems to me that, at a bare minimum, it should be documented that these functions are not thread safe under Windows.

    1762cc99-3127-4a62-9baf-30c3d0f51ef7 commented 13 years ago

    New changeset 3fa7581f6d46 by Antoine Pitrou in branch '2.7': Issue bpo-6476: Document that os.spawnle and os.spawnve are not thread-safe under Windows. http://hg.python.org/cpython/rev/3fa7581f6d46

    New changeset a01ba3c32a4b by Antoine Pitrou in branch '3.2': Issue bpo-6476: Document that os.spawnle and os.spawnve are not thread-safe under Windows. http://hg.python.org/cpython/rev/a01ba3c32a4b

    New changeset aee7c27ea7df by Antoine Pitrou in branch 'default': Issue bpo-6476: Document that os.spawnle and os.spawnve are not thread-safe under Windows. http://hg.python.org/cpython/rev/aee7c27ea7df

    pitrou commented 13 years ago

    I've made the necessary doc changes. I leave it open because I'm not sure what to do with the bugfix request (I agree with the general suggestion to use subprocess instead, though).