Skip to content

Issue 1120 (Cleaning up tmpdir's) #6253

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

Closed
wants to merge 8 commits into from
Closed

Issue 1120 (Cleaning up tmpdir's) #6253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f9d5127
Register cleanup_numbered_dir for atexit before the cleanup for locks…
bitmuster Jul 16, 2019
3d5d0ac
Add comments to clarify directory cleanup
bitmuster Jul 16, 2019
0213740
Remove obsolete cleanup at program start
bitmuster Jul 16, 2019
9439e07
Linting autoformat
bitmuster Jul 17, 2019
7dee7fc
Add changelog entry
bitmuster Jul 17, 2019
3e07134
Update AUTHORS file
bitmuster Jul 17, 2019
5eb46e3
Move at_exit registration outside numbered dir loop
caitelatte Nov 21, 2019
137a2a7
Add caitlin to the AUTHORS
caitelatte Nov 21, 2019
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
2 changes: 2 additions & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ Brian Maissy
Brian Okken
Brianna Laugher
Bruno Oliveira
Caitlin Macleod
Cal Leeming
Carl Friedrich Bolz
Carlos Jenkins
Expand Down Expand Up @@ -177,6 +178,7 @@ Matt Williams
Matthias Hafner
Maxim Filipenko
mbyt
Michael Abel
Michael Aquilina
Michael Birtwell
Michael Droettboom
Expand Down
2 changes: 2 additions & 0 deletions changelog/1120.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix issue where directories from ``tmpdir`` are not removed properly when
multiple instances of pytest are running in parallel.
16 changes: 9 additions & 7 deletions src/_pytest/pathlib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,15 @@ def make_numbered_dir_with_cleanup(
) -> Path:
"""creates a numbered dir with a cleanup lock and removes old ones"""
e = None
# Register a cleanup for program exit
consider_lock_dead_if_created_before = p.stat().st_mtime - lock_timeout
Copy link
Member

Choose a reason for hiding this comment

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

Instead of p.stat().st_mtime, this could just be time.time() (which you might need to import)

Copy link
Member

Choose a reason for hiding this comment

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

the code as it is hoere cant hope to work (as p is not defined up there)

ensuring the lock time-out is considered relative to the created folder is critical (as it ensures relative timing correctness)

as far as i can tell the atexit registration can only be done sanely in the else clasuse where we did the cal lbeffore

Copy link
Member

Choose a reason for hiding this comment

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

something we MUST check is if based on the atexit order this allows the folder of the current process to be cleaned up alongside other folders

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review folks. I didn't notice the relationship to the p folder at the time so I'm sorry for missing that!

@RonnyPfannschmidt could you please expand on the relative timing correctness using the successful folder's creation time results in? I thought that using the time at the beginning of the function would be good enough.

If we move the register back into the else: clause of the try/except block, I suspect that it may not always be called if folder creation didn't succeed at all, so wouldn't be registered to clean up older folders. What if it goes into a finally: clause or at the end of the for loop, so that it always gets called regardless of success or failure to create a folder? This would still need to remove the reliance on p.stat().st_mtime as it may not have been created.

Regarding atexit order, the register_cleanup_lock_removal(lock_path) line registers its own atexit cleanup on a lock on p using register_cleanup_lock_removal(p). We should move the cleanup_numbered_dir registration after the register_cleanup_lock_removal in that case, so that it can run after the lock should have been released.

There's some complex relationships between lock files here so further conversation and feedback would be appreciated 😸

This comment was marked as outdated.

Sign in to view

Copy link
Member

Choose a reason for hiding this comment

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

wrt ensuring the timing is relative to the process, this simply accounts for the possibility of process suspension between setting up the atexit handler and the creation.
it is a rather uncommon/unlikely occurrence

as for not running clean-up if a folder couldn't be made, that seems acceptable to me

the relationship between the lock in p and the folder clean-up is easy to miss, as its not explicit in code, but implicit in context, i just barely noted the detail after a gut feeling and reading the code as well as surrounding code a number of times before it clicked

atexit.register(
cleanup_numbered_dir,
root,
prefix,
keep,
consider_lock_dead_if_created_before,
)
for i in range(10):
try:
p = make_numbered_dir(root, prefix)
Expand All @@ -304,13 +313,6 @@ def make_numbered_dir_with_cleanup(
except Exception as exc:
e = exc
else:
consider_lock_dead_if_created_before = p.stat().st_mtime - lock_timeout
cleanup_numbered_dir(
root=root,
prefix=prefix,
keep=keep,
consider_lock_dead_if_created_before=consider_lock_dead_if_created_before,
)
return p
assert e is not None
raise e
Expand Down