bpo-30028: make test.support.temp_cwd() fork-safe by akruis · Pull Request #1066 · python/cpython · GitHub

Improve the context manager test.support.temp_cwd() to not remove the temporary
directory, if a forked child terminates.

https://bugs.python.org/issue30028

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.

Is it used?

Opps, a left over from a debug print to sys.stderr

Remove an unused "import sys".

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.

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.

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.

Done (1a0364d). Is the patch now OK?

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 :-)

Since this is a subprocess, explicit os.exit() can be more appropriate.

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?

Please test that the child didn't fail.

Please add a comment explaining that the child exits without removing the temporary directory.

Isn't the comment above enough?

Please add a reference to the issue: bpo-30028

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 have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

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?

Thanks @akruis for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
🐍🍒⛏🤖

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 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 33dddac00ba8d9b72cf21b8698504077eb3c23ad 2.7

Sorry, @akruis and @gpshead, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 33dddac00ba8d9b72cf21b8698504077eb3c23ad 3.6

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>

bpo-30028: make test.support.temp_cwd() fork-safe

22e032a

Improve the context manager test.support.temp_cwd() to not remove the temporary directory, if a forked child terminates.

the-knights-who-say-ni added the CLA signed label Apr 9, 2017

serhiy-storchaka reviewed Apr 10, 2017

View reviewed changes

Update test_regrtest.py

7b96cf7

Remove an unused "import sys".

vstinner requested changes Apr 10, 2017

View reviewed changes

vstinner reviewed Apr 10, 2017

View reviewed changes

bpo-30028: Add comments, move the test to test_support.py

9ca9665

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.

vstinner reviewed Apr 10, 2017

View reviewed changes

bpo-30028: Run the fork test as an external script.

1a0364d

serhiy-storchaka approved these changes Apr 10, 2017

View reviewed changes

vstinner reviewed Apr 10, 2017

View reviewed changes

bpo-30028: replace assert by if ...: raise AssertionError

0f480d9

An assert might removed depending on Python options.

vstinner reviewed Apr 21, 2017

View reviewed changes

vstinner requested changes Apr 21, 2017

View reviewed changes

bpo-30028: improve the test case

8cebac8

- better comments - make sure, that the child exits successfully

Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017

gpshead self-assigned this Jan 30, 2018

brettcannon added the awaiting changes label Feb 21, 2018

Merge branch 'master' into tempcwd_fork_fix

1b78826

serhiy-storchaka added needs backport to 3.6 tests Tests in the Lib/test dir skip news labels Feb 21, 2018

serhiy-storchaka added needs backport to 3.7 and removed awaiting changes labels Feb 21, 2018

bpo-30028: Add name to Misc/ACKS

ee34a5b

Proposed by Serhiy Storchaka.

gpshead merged commit 33dddac into python:master Feb 23, 2018

bedevere-bot removed the needs backport to 3.7 label Feb 23, 2018

bedevere-bot removed the needs backport to 2.7 label Feb 23, 2018

bedevere-bot removed the needs backport to 3.6 label Feb 23, 2018

akruis deleted the tempcwd_fork_fix branch February 23, 2018 11:45

+              class TempCwdTestCase(unittest.TestCase):
+                  @unittest.skipUnless(hasattr(os, "fork"), "test requires os.fork")
+                  def test_forked_child(self):
+                      import sys

                       yield path
                   finally:
-                      if dir_created:
+                      if dir_created and pid == os.getpid():

+                  @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:

+                                  # 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."

+                              pid = os.fork()
+                              if pid != 0:
+                                  # parent process
+                                  os.waitpid(pid, 0)  # wait for the child to terminate

+                                  # make sure that temp_path is still present
+                                  if not os.path.isdir(temp_path):
+                                      raise AssertionError("Child removed temp_path.")
+                      """))

bpo-30028: make test.support.temp_cwd() fork-safe by akruis · Pull Request #1066 · python/cpython · GitHub

Repository navigation

bpo-30028: make test.support.temp_cwd() fork-safe - #1066

bpo-30028: make test.support.temp_cwd() fork-safe#1066gpshead merged 8 commits intopython:masterpython/cpython:masterfrom akruis:tempcwd_fork_fixakruis/cpython:tempcwd_fork_fixCopy head branch name to clipboard

Conversation

akruis commented Apr 9, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Apr 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akruis commented Apr 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettcannon commented Feb 21, 2018

Uh oh!

serhiy-storchaka commented Feb 21, 2018

Uh oh!

serhiy-storchaka commented Feb 21, 2018

Uh oh!

akruis commented Feb 22, 2018

Uh oh!

miss-islington commented Feb 23, 2018

Uh oh!

bedevere-bot commented Feb 23, 2018

Uh oh!

miss-islington commented Feb 23, 2018

Uh oh!

miss-islington commented Feb 23, 2018

Uh oh!

gpshead commented Feb 23, 2018

Uh oh!

bedevere-bot commented Feb 23, 2018

Uh oh!

bedevere-bot commented Feb 23, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

bpo-30028: make test.support.temp_cwd() fork-safe#1066
gpshead merged 8 commits into
python:masterfrom
akruis:tempcwd_fork_fix

akruis commented Apr 9, 2017 •
edited by bedevere-bot
Loading