GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-30028: make test.support.temp_cwd() fork-safe by akruis · Pull Request #1066 · python/cpython

Conversation

@akruis

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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

https://bugs.python.org/issue30028

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Anselm Kruis
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 the-knights-who-say-ni added the CLA signed label Apr 9, 2017
@mention-bot

mention-bot commented Apr 9, 2017

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka reviewed Apr 10, 2017
View reviewed changes
Comment thread 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

@serhiy-storchaka serhiy-storchaka Apr 10, 2017

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@akruis akruis Apr 10, 2017

Copy link
Copy Markdown
Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Anselm Kruis
Update test_regrtest.py
7b96cf7 
Remove an unused "import sys".
vstinner
vstinner requested changes Apr 10, 2017
View reviewed changes
Comment thread Lib/test/support/__init__.py
yield path
finally:
if dir_created:
if dir_created and pid == os.getpid():

@vstinner vstinner Apr 10, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed Apr 10, 2017
View reviewed changes
Comment thread Lib/test/test_regrtest.py Outdated
randomize=True)


class TempCwdTestCase(unittest.TestCase):

@vstinner vstinner Apr 10, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Anselm Kruis
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
vstinner reviewed Apr 10, 2017
View reviewed changes
Comment thread 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:

@vstinner vstinner Apr 10, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@akruis akruis Apr 10, 2017

Copy link
Copy Markdown
Author

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Anselm Kruis
bpo-30028: Run the fork test as an external script.
1a0364d
serhiy-storchaka
serhiy-storchaka approved these changes Apr 10, 2017
View reviewed changes
vstinner
vstinner reviewed Apr 10, 2017
View reviewed changes
Comment thread 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."

@vstinner vstinner Apr 10, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Apr 10, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@akruis akruis Apr 11, 2017

Copy link
Copy Markdown
Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Anselm Kruis
bpo-30028: replace assert by if ...: raise AssertionError
0f480d9 
An assert might removed depending on Python options.
@akruis

akruis commented Apr 17, 2017

Copy link
Copy Markdown
Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Apr 21, 2017
View reviewed changes
Comment thread Lib/test/test_support.py Outdated
pid = os.fork()
if pid != 0:
# parent process
os.waitpid(pid, 0) # wait for the child to terminate

@vstinner vstinner Apr 21, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner requested changes Apr 21, 2017
View reviewed changes
Comment thread 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.")
"""))

@vstinner vstinner Apr 21, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Apr 22, 2017

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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."""

@vstinner vstinner Apr 21, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@akruis akruis May 1, 2017

Copy link
Copy Markdown
Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Anselm Kruis
bpo-30028: improve the test case
8cebac8 
- better comments
- make sure, that the child exits successfully
akruis pushed a commit to stackless-dev/stackless that referenced this pull request Sep 5, 2017
Anselm Kruis
Issue #112: Prepare Stackless 3.5, fix a C-Python test suite bug
70d307e 
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
akruis pushed a commit to akruis/cpython that referenced this pull request Sep 9, 2017
Anselm Kruis
Issue python#112: Prepare Stackless 3.5, fix a C-Python test suite bug
ca52f72 
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)
@Mariatta Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017
@gpshead gpshead self-assigned this Jan 30, 2018
@brettcannon

brettcannon commented Feb 21, 2018

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brettcannon brettcannon added the awaiting changes label Feb 21, 2018
@serhiy-storchaka
Merge branch 'master' into tempcwd_fork_fix
1b78826
@serhiy-storchaka

serhiy-storchaka commented Feb 21, 2018

Copy link
Copy Markdown
Member

I think requests by @vstinner were satisfied.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka serhiy-storchaka added needs backport to 3.6 tests Tests in the Lib/test dir skip news labels Feb 21, 2018
@serhiy-storchaka serhiy-storchaka added needs backport to 3.7 and removed awaiting changes labels Feb 21, 2018
@serhiy-storchaka

serhiy-storchaka commented Feb 21, 2018

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@akruis
bpo-30028: Add name to Misc/ACKS
ee34a5b 
Proposed by Serhiy Storchaka.
@akruis

akruis commented Feb 22, 2018

Copy link
Copy Markdown
Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
gpshead merged commit 33dddac into python:master Feb 23, 2018
@miss-islington

miss-islington commented Feb 23, 2018

Copy link
Copy Markdown
Contributor

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.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 23, 2018
Anselm Kruis @miss-islington
bpo-30028: make test.support.temp_cwd() fork-safe (pythonGH-1066)
357ce94 
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

bedevere-bot commented Feb 23, 2018

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.7 label Feb 23, 2018
@miss-islington

miss-islington commented Feb 23, 2018

Copy link
Copy Markdown
Contributor

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington

miss-islington commented Feb 23, 2018

Copy link
Copy Markdown
Contributor

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead

gpshead commented Feb 23, 2018

Copy link
Copy Markdown
Member

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. :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington added a commit that referenced this pull request Feb 23, 2018
@miss-islington Anselm Kruis
bpo-30028: make test.support.temp_cwd() fork-safe (GH-1066)
694c5e0 
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]>
akruis pushed a commit to akruis/cpython that referenced this pull request Feb 23, 2018
Anselm Kruis @akruis
[2.7] bpo-30028: make test.support.temp_cwd() fork-safe (pythonGH-1066)
95ec069 
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

bedevere-bot commented Feb 23, 2018

Copy link
Copy Markdown

GH-5825 is a backport of this pull request to the 2.7 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 2.7 label Feb 23, 2018
akruis pushed a commit to akruis/cpython that referenced this pull request Feb 23, 2018
Anselm Kruis @akruis
[3.6] bpo-30028: make test.support.temp_cwd() fork-safe (pythonGH-1066)
8a3dae7 
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 bedevere-bot removed the needs backport to 3.6 label Feb 23, 2018
@bedevere-bot

bedevere-bot commented Feb 23, 2018

Copy link
Copy Markdown

GH-5826 is a backport of this pull request to the 3.6 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@akruis
akruis deleted the tempcwd_fork_fix branch February 23, 2018 11:45
gpshead pushed a commit that referenced this pull request Feb 23, 2018
Anselm Kruis @gpshead
[2.7] bpo-30028: make test.support.temp_cwd() fork-safe (GH-1066) (GH…
61bd4d2 
…-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]>
gpshead pushed a commit that referenced this pull request Feb 23, 2018
Anselm Kruis @gpshead
[3.6] bpo-30028: make test.support.temp_cwd() fork-safe (GH-1066) (GH…
9c819a6 
…-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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@vstinner vstinner vstinner requested changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gpshead gpshead

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@akruis @mention-bot @brettcannon @serhiy-storchaka @miss-islington @bedevere-bot @gpshead @vstinner @Mariatta @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.