-
-
Notifications
You must be signed in to change notification settings - Fork 32k
bpo-36867: Make semaphore_tracker track other system resources. #13222
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
Conversation
You may want to take a look at the CI failures. |
Yes: turns out we don't want a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks mostly good to me. I have just one comment and a question.
Some more comment about OS availability: On windows, it looks that even with no |
Indeed, it seems that shared memory on Windows is more cleverly implemented than on Unix. |
cc @applio just in case he wants to confirm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple more nits :-)
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be poked with soft cushions! |
I have made the requested changes; please review again. |
Thanks for making the requested changes! @pitrou: please review the changes made to this pull request. |
Also @pitrou, before potentially merging, any opinion on pierreglaser#3? It adds shared-memory leaking tests for all platforms, independently of whether the |
Note also that there is still work to do to properly release shared memory segments created with a |
Thanks @pierreglaser ! |
Thanks @pitrou for the infallible reviews ;) |
Primary Purpose
This PR proposes to extend the
semaphore_tracker
range of action to other system resources.Currently in
python3.8
, if a process that created ashared_memory
object gets violently terminated (withkill
for example), the memory segment will not be properly released until the next machine reboot. This will be problematic for long-running clusters used by many different users.The primary reason of this PR is thus to extend the
semaphore_tracker
to track alsoshared_memory
segments. Thesemaphore_tracker
module gets consequently renamedresource_tracker
.Extension to a potentially public API
Additionally, the design of the
resource_tracker
is more generic, and possibly suggests a new public API to enable the tracking other named-resources. For now, the private_CLEANUP_FUNCS
dict references all the types that can possibly be tracked.To start tracking a new type, the
resource_tracker
only needs to know which cleanup routine to apply to each leaked instance of this type after the python session ends. The public API would thus consist of a function likeresource_tracker.register_resource_type(resource_type, cleanup_func)
, where:string
cleanup_func
must be a callable taking a single string as an argument.Under the hood,
register_resource_type
would populate theCLEANUP_FUNCS
with the new (type, cleanup_func
) record.Operating System Availability
To ease the review process of this PR. the resource_tracker is now POSIX only. I however have a functional windows implementation that I will provide upon demand :)
pinging @pitrou @ogrisel @tomMoral
@pablogsal this may interest you as well :)
https://bugs.python.org/issue36867