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

Improve safe_concurrent_creation contextmanager. #4690

Merged
merged 5 commits into from Jun 22, 2017

Conversation

Projects
None yet
2 participants
@kwlzn
Copy link
Member

kwlzn commented Jun 22, 2017

Fixes #4640

Problem

Currently, the safe_concurrent_creation contextmgr does the final directory rename in a finally block, which can lead to empty directory creation in the face of error handling.

Solution

Move the rename into a try/except/else block.

Result

No longer able to repro #4640.

@kwlzn kwlzn requested a review from benjyw Jun 22, 2017

@benjyw

benjyw approved these changes Jun 22, 2017

Copy link
Contributor

benjyw left a comment

Is this the right behavior for other callers of safe_concurrent_creation (if any)?

@kwlzn kwlzn force-pushed the twitter:kwlzn/4640 branch from 3146b0f to d4e2570 Jun 22, 2017

@kwlzn

This comment has been minimized.

Copy link
Member

kwlzn commented Jun 22, 2017

Is this the right behavior for other callers of safe_concurrent_creation (if any)?

yep.

@kwlzn kwlzn merged commit 91e3a93 into pantsbuild:master Jun 22, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

stuhood added a commit that referenced this pull request Jun 27, 2017

Improve safe_concurrent_creation contextmanager. (#4690)
Fixes #4640

Problem

Currently, the safe_concurrent_creation contextmgr does the final directory rename in a finally block, which can lead to empty directory creation in the face of error handling.

Solution

Move the rename into a try/except/else block.

Result

No longer able to repro #4640.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment