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

akruis · 2017-04-09T20:12:58Z

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.

mention-bot · 2017-04-09T20:13:02Z

@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.

serhiy-storchaka · 2017-04-10T06:53:21Z

Lib/test/test_regrtest.py

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


Is it used?

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

Remove an unused "import sys".

vstinner · 2017-04-10T08:05:18Z

Lib/test/support/__init__.py

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


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.

vstinner · 2017-04-10T08:06:17Z

Lib/test/test_regrtest.py

@@ -821,5 +821,19 @@ def test_crashed(self):
                                  randomize=True)


+class TempCwdTestCase(unittest.TestCase):


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.

vstinner · 2017-04-10T09:04:27Z

Lib/test/test_support.py

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


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?

vstinner · 2017-04-10T21:42:41Z

Lib/test/test_support.py

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


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.

akruis · 2017-04-17T20:05:10Z

@Haypo and @serhiy-storchaka: Any chance to get this PR merged?

vstinner · 2017-04-21T23:09:00Z

Lib/test/test_support.py

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


Please test that the child didn't fail.

vstinner · 2017-04-21T23:10:28Z

Lib/test/test_support.py

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


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

Isn't the comment above enough?

vstinner · 2017-04-21T23:11:01Z

Lib/test/test_support.py

@@ -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."""


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)

brettcannon · 2018-02-21T01:51:37Z

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.

serhiy-storchaka · 2018-02-21T06:56:56Z

I think requests by @vstinner were satisfied.

serhiy-storchaka · 2018-02-21T07:00:34Z

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.

akruis · 2018-02-22T19:49:23Z

Is there anything, I can do to get this PR merged?

miss-islington · 2018-02-23T01:37:41Z

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 <[email protected]>

bedevere-bot · 2018-02-23T01:38:51Z

GH-5824 is a backport of this pull request to the 3.7 branch.

miss-islington · 2018-02-23T01:39:44Z

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

miss-islington · 2018-02-23T01:40:43Z

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

gpshead · 2018-02-23T01:47:45Z

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 <[email protected]>

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 <[email protected]>

bedevere-bot · 2018-02-23T10:57:26Z

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 <[email protected]>

bedevere-bot · 2018-02-23T11:09:19Z

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 <[email protected]>

…-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 <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

akruis commented Apr 9, 2017 •

edited by bedevere-bot

Loading

mention-bot commented Apr 9, 2017

serhiy-storchaka Apr 10, 2017

akruis Apr 10, 2017

vstinner Apr 10, 2017

vstinner Apr 10, 2017

vstinner Apr 10, 2017

akruis Apr 10, 2017

vstinner Apr 10, 2017

serhiy-storchaka Apr 10, 2017

akruis Apr 11, 2017

akruis commented Apr 17, 2017

vstinner Apr 21, 2017

vstinner Apr 21, 2017

serhiy-storchaka Apr 22, 2017

vstinner Apr 21, 2017

akruis May 1, 2017

brettcannon commented Feb 21, 2018

serhiy-storchaka commented Feb 21, 2018

serhiy-storchaka commented Feb 21, 2018

akruis commented Feb 22, 2018

miss-islington commented Feb 23, 2018

bedevere-bot commented Feb 23, 2018

miss-islington commented Feb 23, 2018

miss-islington commented Feb 23, 2018

gpshead commented Feb 23, 2018

bedevere-bot commented Feb 23, 2018

bedevere-bot commented Feb 23, 2018

		@@ -821,5 +821,19 @@ def test_crashed(self):
		randomize=True)


		class TempCwdTestCase(unittest.TestCase):

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

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

Conversation

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

mention-bot commented Apr 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akruis commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettcannon commented Feb 21, 2018

serhiy-storchaka commented Feb 21, 2018

serhiy-storchaka commented Feb 21, 2018

akruis commented Feb 22, 2018

miss-islington commented Feb 23, 2018

bedevere-bot commented Feb 23, 2018

miss-islington commented Feb 23, 2018

miss-islington commented Feb 23, 2018

gpshead commented Feb 23, 2018

bedevere-bot commented Feb 23, 2018

bedevere-bot commented Feb 23, 2018

akruis commented Apr 9, 2017 •

edited by bedevere-bot

Loading