classification
Title: make test.support.temp_cwd() fork-safe
Type: behavior Stage: commit review
Components: Tests Versions: Python 3.8, Python 3.7, Python 3.6, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: anselm.kruis, christian.heimes, gregory.p.smith, inada.naoki, miss-islington, serhiy.storchaka, vinay.sajip, vstinner
Priority: normal Keywords: patch

Created on 2017-04-09 20:00 by anselm.kruis, last changed 2018-02-23 16:28 by gregory.p.smith. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 1066 merged anselm.kruis, 2017-04-09 20:12
PR 1074 closed vstinner, 2017-04-10 08:02
PR 5824 merged miss-islington, 2018-02-23 01:38
PR 5825 merged anselm.kruis, 2018-02-23 10:57
PR 5826 merged anselm.kruis, 2018-02-23 11:09
Messages (14)
msg291396 - (view) Author: Anselm Kruis (anselm.kruis) * Date: 2017-04-09 20:00
The context manager test.support.temp_cwd() creates a temporary directory and removes it on exit. The test runner test.regrtest uses this context manager.

I observed an annoying behaviour of test.support.temp_cwd() on Linux/UNIX: if the code, that runs in the temp_cwd() context forks and if the forked child terminates (without calling exec), then the temporary directory will be removed twice: by the child and by the parent. This can cause errors in the parent, if the parent tries to access the no longer existing directory.

I discovered this problem, when a test in test_multiprocessing_fork failed and the test directory for the complete test.regrtest-run got removed. Of course all other tests failed too.

I propose to modify test.support.temp_cwd() to remove the created directory only, if the process id (os.getpid()) is unchanged. I'll create a pull request.
msg291413 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-04-10 05:40
Is os.getpid always available? I have found hasattr(os, 'getpid') in the logging module.
msg291415 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-04-10 06:02
The line is from 2006. Some parts of the logging module are much older, maybe even from the time Python had DOS support.
msg291417 - (view) Author: Anselm Kruis (anselm.kruis) * Date: 2017-04-10 06:46
I had the same concerns about os.getpid(), but test.support uses it unconditionally in various places. See https://github.com/python/cpython/blob/master/Lib/test/support/__init__.py#L803 for an example.
msg291418 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-04-10 06:59
Then LGTM.
msg291420 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-04-10 08:03
I proposed PR 1074 to remove the hasattr(os, 'getpid') from logging.LogRecord constructor.
msg291422 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2017-04-10 08:40
#28156 added HAVE_GETPID check.
Maybe, we should have dummy getpid() for CloudABI?
msg291423 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-04-10 08:43
Naoki:
> #28156 added HAVE_GETPID check.
> Maybe, we should have dummy getpid() for CloudABI?

Oh. If os.getpid() is not always available, the documentation should be updated.
msg291424 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2017-04-10 08:59
> Oh. If os.getpid() is not always available, the documentation should be updated.

Yes, but CloudABI is not officially supported platform.
They have many patches to run Python on CloudABI. https://github.com/NuxiNL/cloudabi-ports/tree/master/packages/python

How can we document about it?  I hadn't used it.

os.getlogin document says `Availability: Unix, Windows.` [1]
Is it proper way to indicate "there may be some minor platforms which doesn't provide this."?
[1] https://docs.python.org/3.6/library/os.html#os.getlogin
msg291426 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-04-10 09:22
> os.getlogin document says `Availability: Unix, Windows.`

Originally this meant "not in Mac OS, DOS, OS/2, VMS". But now the support of all these platforms is dropped and the note is outdated.
msg312609 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-02-23 01:37
New changeset 33dddac00ba8d9b72cf21b8698504077eb3c23ad by Gregory P. Smith (Anselm Kruis) in branch 'master':
bpo-30028: make test.support.temp_cwd() fork-safe (GH-1066)
https://github.com/python/cpython/commit/33dddac00ba8d9b72cf21b8698504077eb3c23ad
msg312618 - (view) Author: miss-islington (miss-islington) Date: 2018-02-23 05:39
New changeset 694c5e0e1f7f23595fab314f26b89e241477ff18 by Miss Islington (bot) in branch '3.7':
bpo-30028: make test.support.temp_cwd() fork-safe (GH-1066)
https://github.com/python/cpython/commit/694c5e0e1f7f23595fab314f26b89e241477ff18
msg312641 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-02-23 16:27
New changeset 61bd4d2e6319a3c5c3b9ce5f807b44a45cc1d4a1 by Gregory P. Smith (Anselm Kruis) in branch '2.7':
[2.7] bpo-30028: make test.support.temp_cwd() fork-safe (GH-1066) (GH-5825)
https://github.com/python/cpython/commit/61bd4d2e6319a3c5c3b9ce5f807b44a45cc1d4a1
msg312642 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-02-23 16:27
New changeset 9c819a6a7d34594779fea3a25fd69ec5745e185e by Gregory P. Smith (Anselm Kruis) in branch '3.6':
[3.6] bpo-30028: make test.support.temp_cwd() fork-safe (GH-1066) (GH-5826)
https://github.com/python/cpython/commit/9c819a6a7d34594779fea3a25fd69ec5745e185e
History
Date User Action Args
2018-02-23 16:28:33gregory.p.smithsetstatus: open -> closed
stage: patch review -> commit review
resolution: fixed
versions: + Python 2.7, Python 3.8, - Python 3.5
2018-02-23 16:27:56gregory.p.smithsetmessages: + msg312642
2018-02-23 16:27:30gregory.p.smithsetmessages: + msg312641
2018-02-23 11:09:18anselm.kruissetpull_requests: + pull_request5604
2018-02-23 10:57:25anselm.kruissetpull_requests: + pull_request5603
2018-02-23 05:39:10miss-islingtonsetnosy: + miss-islington
messages: + msg312618
2018-02-23 01:38:51miss-islingtonsetkeywords: + patch
pull_requests: + pull_request5601
2018-02-23 01:37:44gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg312609
2017-04-10 09:22:27serhiy.storchakasetmessages: + msg291426
2017-04-10 08:59:00inada.naokisetmessages: + msg291424
2017-04-10 08:43:19vstinnersetmessages: + msg291423
2017-04-10 08:40:13inada.naokisetnosy: + inada.naoki
messages: + msg291422
2017-04-10 08:03:37vstinnersetmessages: + msg291420
2017-04-10 08:02:33vstinnersetpull_requests: + pull_request1217
2017-04-10 06:59:24serhiy.storchakasetmessages: + msg291418
2017-04-10 06:46:21anselm.kruissetmessages: + msg291417
2017-04-10 06:02:56christian.heimessetnosy: + christian.heimes
messages: + msg291415
2017-04-10 05:40:14serhiy.storchakasetnosy: + vinay.sajip, vstinner, serhiy.storchaka

messages: + msg291413
stage: patch review
2017-04-09 20:12:59anselm.kruissetpull_requests: + pull_request1212
2017-04-09 20:00:25anselm.kruiscreate