-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
bpo-30028: make test.support.temp_cwd() fork-safe #1066
Conversation
Improve the context manager test.support.temp_cwd() to not remove the temporary directory, if a forked child terminates.
@akruis, thanks for your PR! By analyzing the history of the files in this pull request, we identified @serhiy-storchaka, @Yhg1s and @ncoghlan to be potential reviewers. |
Lib/test/test_regrtest.py
Outdated
class TempCwdTestCase(unittest.TestCase): | ||
@unittest.skipUnless(hasattr(os, "fork"), "test requires os.fork") | ||
def test_forked_child(self): | ||
import sys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opps, a left over from a debug print to sys.stderr
Remove an unused "import sys".
Lib/test/support/__init__.py
Outdated
try: | ||
yield path | ||
finally: | ||
if dir_created: | ||
if dir_created and pid == os.getpid(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is non obvious, can you please add a short comment explaining why you test the pid?
I guess that it's test to avoid that two processes try to remove the directory if the parent uses fork()?
Add bpo-30028 in the comment.
Lib/test/test_regrtest.py
Outdated
@@ -821,5 +821,19 @@ def test_crashed(self): | |||
randomize=True) | |||
|
|||
|
|||
class TempCwdTestCase(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this test to test_support.py which tests test.support.
test_regrtest tests Lib/test/libregrtest/.
Added an explanatory comment and moved the test to test_support.py. I also modified the test to be in line with the other tests of temp_cwd.
Lib/test/test_support.py
Outdated
@unittest.skipUnless(hasattr(os, "fork"), "test requires os.fork") | ||
def test_temp_dir__forked_child(self): | ||
"""Test that a forked child process does not remove the directory.""" | ||
with support.temp_cwd() as temp_path: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, please don't use fork in a test process. Write a short script run with test.support.script_helper. Otherwise, I expect too many issues like bad handling of KeyboardInterrupt. Use textwrap.dedent() to include the script directly here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (1a0364d). Is the patch now OK?
Lib/test/test_support.py
Outdated
# parent process | ||
os.waitpid(pid, 0) # wait for the child to terminate | ||
# make sure that temp_path is still present | ||
assert os.path.isdir(temp_path), "Child removed temp_path." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid assert keyword which might be removed depending on the Python option, but use an explicit if + raise Exception(...). But maybe it's ok, I don't know :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a subprocess, explicit os.exit()
can be more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm currently on a business trip and can't access my Linux box. I'll update the pull request on Thursday. But could you please agree on either raising AssertionError or a print() and explicit os.exit(). In the end it makes no difference.
An assert might removed depending on Python options.
@Haypo and @serhiy-storchaka: Any chance to get this PR merged? |
Lib/test/test_support.py
Outdated
pid = os.fork() | ||
if pid != 0: | ||
# parent process | ||
os.waitpid(pid, 0) # wait for the child to terminate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please test that the child didn't fail.
Lib/test/test_support.py
Outdated
# make sure that temp_path is still present | ||
if not os.path.isdir(temp_path): | ||
raise AssertionError("Child removed temp_path.") | ||
""")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment explaining that the child exits without removing the temporary directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the comment above enough?
@@ -161,6 +163,23 @@ def test_temp_dir__existing_dir__quiet_true(self): | |||
f'temporary directory {path!r}: '), | |||
warn) | |||
|
|||
@unittest.skipUnless(hasattr(os, "fork"), "test requires os.fork") | |||
def test_temp_dir__forked_child(self): | |||
"""Test that a forked child process does not remove the directory.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a reference to the issue: bpo-30028
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No other doc string in this file contains a reference to bpo. Therefore I added the reference as a comment.
- better comments - make sure, that the child exits successfully
This commit merges the fix from C-Python pull request python#1066 (git commit 22e032a) for bpo-30028. Without this change $ ./python -m test.regrtest test_multiprocessing_fork fails. ($ ./python -m test.test_multiprocessing_fork is OK.) https://bitbucket.org/stackless-dev/stackless/issues/112
This commit merges the fix from C-Python pull request python#1066 (git commit 22e032a) for bpo-30028. Without this change $ ./python -m test.regrtest test_multiprocessing_fork fails. ($ ./python -m test.test_multiprocessing_fork is OK.) https://bitbucket.org/stackless-dev/stackless/issues/112 (grafted from bda8a9d487da7cfbe4357d5c4c7635e0de19c6af)
To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request. If/when the requested changes have been made, please leave a comment that says, |
I think requests by @vstinner were satisfied. |
A news entry is not required since this function is for internal use, but it would be worth to add your name @akruis into Misc/ACKS. |
Proposed by Serhiy Storchaka.
Is there anything, I can do to get this PR merged? |
Make test.support.temp_cwd() fork-safe. The context manager test.support.temp_cwd() no longer removes the temporary directory when executing in a process other than the parent it entered from. If a forked child exits the context manager it won't do the cleanup. (cherry picked from commit 33dddac) Co-authored-by: Anselm Kruis <a.kruis@science-computing.de>
GH-5824 is a backport of this pull request to the 3.7 branch. |
Sorry, @akruis and @gpshead, I could not cleanly backport this to |
Sorry, @akruis and @gpshead, I could not cleanly backport this to |
I'm not sure it is actually worth backporting this to 3.6 or 2.7 for this internal test support library issue but if you want to do so, just create PRs using cherry_picker and tag me on them for review/merge. :) |
Make test.support.temp_cwd() fork-safe. The context manager test.support.temp_cwd() no longer removes the temporary directory when executing in a process other than the parent it entered from. If a forked child exits the context manager it won't do the cleanup. (cherry picked from commit 33dddac) Co-authored-by: Anselm Kruis <a.kruis@science-computing.de>
Make test.support.temp_cwd() fork-safe. The context manager test.support.temp_cwd() no longer removes the temporary directory when executing in a process other than the parent it entered from. If a forked child exits the context manager it won't do the cleanup.. (cherry picked from commit 33dddac) Co-authored-by: Anselm Kruis <a.kruis@science-computing.de>
GH-5825 is a backport of this pull request to the 2.7 branch. |
Make test.support.temp_cwd() fork-safe. The context manager test.support.temp_cwd() no longer removes the temporary directory when executing in a process other than the parent it entered from. If a forked child exits the context manager it won't do the cleanup.. (cherry picked from commit 33dddac) Co-authored-by: Anselm Kruis <a.kruis@science-computing.de>
GH-5826 is a backport of this pull request to the 3.6 branch. |
…-5825) Make test.support.temp_cwd() fork-safe. The context manager test.support.temp_cwd() no longer removes the temporary directory when executing in a process other than the parent it entered from. If a forked child exits the context manager it won't do the cleanup.. (cherry picked from commit 33dddac) Co-authored-by: Anselm Kruis <a.kruis@science-computing.de>
…-5826) Make test.support.temp_cwd() fork-safe. The context manager test.support.temp_cwd() no longer removes the temporary directory when executing in a process other than the parent it entered from. If a forked child exits the context manager it won't do the cleanup.. (cherry picked from commit 33dddac) Co-authored-by: Anselm Kruis <a.kruis@science-computing.de>
Improve the context manager test.support.temp_cwd() to not remove the temporary
directory, if a forked child terminates.
https://bugs.python.org/issue30028