Issue 1120 (Cleaning up tmpdir's) #6253
Conversation
… is registered (pytest-dev#1120) The cleanup of the numbered directory will now be executed after the locks of the current process got removed.
| @@ -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 | |||
There was a problem hiding this comment.
Instead of p.stat().st_mtime, this could just be time.time() (which you might need to import)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
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
|
Closing this as inactive / needs a different approach. |
Hi folks,
I have rebased the bugfix @bitmuster which closes #1120, including moving the at_exit registration outside a loop it didn't need to be in. The registration will be called once regardless of how the creation or exceptions in the area go.
Mostly prepared by @bitmuster, chatted in person with @Zac-HD.