Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixed rare race condition error when creating the basetemp directory #5525

Closed
wants to merge 1 commit into from
Closed

Fixed rare race condition error when creating the basetemp directory #5525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/5524.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed rare race condition error when creating the ``basetemp`` directory.
2 changes: 1 addition & 1 deletion src/_pytest/pathlib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def ensure_reset_dir(path):
"""
if path.exists():
rmtree(path, force=True)
path.mkdir()
Copy link
Member

Choose a reason for hiding this comment

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

it absolutely has to fail when doing a ensure_reset

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem as I understand, is a race condition between workers trying to create the same basedir at the same time; worker 1 and 2 (for example) manage to rmtree at the same time, but only one of them will succeed when they get to path.mkdir()

Copy link
Member

Choose a reason for hiding this comment

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

ensure_dir_reset is about completely resetting a folder, if the invariant does not hold, something went baldy wrong, and 2 processes that absolutely should not use the same folder will do so

pytest/src/_pytest/tmpdir.py

Lines 58 to 60 in 9136b3f

if self._given_basetemp is not None:
basetemp = self._given_basetemp
ensure_reset_dir(basetemp)
- if a exact basetemp is given

it absolutely never must pass if the observed error happens - basetemp invocation itself is not concurrency save ever

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, my bad, indeed basetemp is created by the master process so there's shouldn't be a race condition there.

I'm closing this and the related issue, I will investigate some more on our side as we do pass a basetemp around. 👍

Thanks for the quick review!

path.mkdir(exist_ok=True)


def rmtree(path, force=False):
Expand Down