Skip to content

bpo-29694: race condition in pathlib mkdir with flags parents=True #1089

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

Merged
merged 4 commits into from
Apr 13, 2017
Merged

bpo-29694: race condition in pathlib mkdir with flags parents=True #1089

merged 4 commits into from
Apr 13, 2017

Conversation

arigo
Copy link
Contributor

@arigo arigo commented Apr 12, 2017

No description provided.

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

@arigo arigo changed the title Issue #29694: race condition in pathlib mkdir with flags parents=True bpo-29694: race condition in pathlib mkdir with flags parents=True Apr 12, 2017
got_exception = False
except FileExistsError:
got_exception = True
self.assertEqual(str(p12) in concurrently_created, got_exception)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertIn()/assertNotIn()? This will give more information in the case of failure.

try:
    ...
except FileExistsError:
    self.assertIn(str(p12), concurrently_created)
else:
    self.assertNotIn(str(p12), concurrently_created)

os.mkdir(path, mode) # from another process
concurrently_created.add(path)
os.mkdir(path, mode) # our real call
org_mkdir = pathlib._normal_accessor.mkdir
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this is not used.

Misc/NEWS Outdated
@@ -306,6 +306,9 @@ Extension Modules

Library
-------

- bpo-29694: race condition in pathlib mkdir with flags parents=True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add "Fixed" at the start, add a period at the end.

@serhiy-storchaka
Copy link
Member

In general LGTM, just few minor comments.

@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.5 labels Apr 12, 2017
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

# just before we try to create it ourselves. We do it
# in all possible pattern combinations, assuming that this
# function is called at most 5 times (dirCPC/dir1/dir2,
# dirCPC/dir1, dirCPC, dirCPC/dir1, cirCPC/dir1/dir2).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cirCPC looks like a typo here.

Misc/NEWS Outdated
@@ -306,6 +306,10 @@ Extension Modules

Library
-------

- bpo-29694: Fixed race condition in pathlib mkdir with flags
parents=True.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also add "Patch by Armin Rigo.".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Armin is a core developer and will merge the PR himself this is not necessary.

@arigo
Copy link
Contributor Author

arigo commented Apr 13, 2017

I may be a core developer but nowadays I'd call this "historical reasons". I didn't commit anything for ages. Please merge this for me.

@Mariatta
Copy link
Member

I updated Misc/NEWS. I will merge and backport once travis CI is completed.
Thanks :)

@Mariatta Mariatta self-assigned this Apr 13, 2017
@Mariatta Mariatta merged commit 22a594a into python:master Apr 13, 2017
Mariatta pushed a commit to Mariatta/cpython that referenced this pull request Apr 14, 2017
Mariatta pushed a commit to Mariatta/cpython that referenced this pull request Apr 14, 2017
Mariatta added a commit that referenced this pull request Apr 14, 2017
Mariatta added a commit that referenced this pull request Apr 14, 2017
@Mariatta Mariatta removed their assignment Jun 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants