Skip to content

gh-82300: Add track parameter to shared memory #110778

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

Merged
merged 28 commits into from
Dec 5, 2023
Merged

Conversation

pan324
Copy link
Contributor

@pan324 pan324 commented Oct 12, 2023

@pan324 pan324 requested a review from tiran as a code owner October 12, 2023 18:12
@ghost
Copy link

ghost commented Oct 12, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Oct 12, 2023

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@pan324
Copy link
Contributor Author

pan324 commented Oct 12, 2023

Whoops I messed up the documentation part. The tracking part should be on unlink, not on close.

@gvanrossum gvanrossum removed the request for review from tiran October 15, 2023 07:37
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Thanks. This looks reasonable, I have mostly questions about the phrasing of the docs.

Do I understand correctly that the whole tracking mechanism only applies on POSIX? Maybe the docs should mention that?

@gvanrossum
Copy link
Member

@serhiy-storchaka Do you have any experience with this code? Could you help me with the review?

@gvanrossum
Copy link
Member

The new parameter needs tests.

@serhiy-storchaka
Copy link
Member

No, I have no experience with this code. I can help with technical review, but I can't say whether the proposed feature is the right way to solve the problem.

It looks like track=False only affects POSIX. What about Windows? Should it raise an exception for track=False? Or for track=True?

If in most cases track should be False if create is false, should not track be set to create by default?

@gvanrossum
Copy link
Member

Thanks, @serhiy-storchaka -- let's see what the PR author says.

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thanks @pan324 for submitting this! The proposal looks good on the principle, but please make sure you adress the review comments (including the request for additional unit tests).

@bedevere-app
Copy link

bedevere-app bot commented Oct 15, 2023

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@pan324
Copy link
Contributor Author

pan324 commented Oct 15, 2023

Thanks everyone!

Yes, the tracking only happens on POSIX because Windows has its own reference counting. I was cautious about mentioning this because users shouldn't rely on such internal behavior. But I can see that Windows users might that need it as a word of caution to thoroughly test their shared memory deallocation on other systems.

Outside of multiprocessing, each Python process comes with its own resource tracker which (assuming track is True) cleans up on termination. If some process shares memory with some short-lived subprocess, then the subprocess process tracker will warn that the user failed to clean up and delete the shared memory. The main process can then either unlink to get a FileNotFoundError or do nothing and be warned by its own resource tracker that the memory wasn't cleaned up properly (right after, the resource tracker will warn about its own FileNotFoundError).

Surely the resource tracker itself would need some deeper changes to be more robust but it's not my area of expertise either which is why I have opted for these minimal changes.

Windows must not raise an exception for either track value! That would make the code nonportable with no gains.

As far as I understand there is a clash between multiprocessing and subprocess. Users of subprocess always want to disable tracking when not creating. For multiprocessing I use the higher level constructs and therefore never had the need to access the SharedMemory module directly. So while users of SharedMemory typically want track and create to have the same value, the internals of multiprocessing may depend on track always being True.

I will look into adding suitable tests now.

pan324 and others added 3 commits October 15, 2023 20:57
…-O38.rst

Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
@gvanrossum
Copy link
Member

@pitrou Can you complete this review? I really don't know enough about this topic to review it properly.

@buzmeg
Copy link

buzmeg commented Nov 9, 2023

@pitrou It looks like this is being held on your request from 3 weeks ago. I believe that @pan324 made the changes you requested.

Could you look at this and either release it or confirm that you still don't think the unit tests are sufficient?

Thanks.

@gpshead gpshead self-assigned this Nov 30, 2023
@gpshead gpshead added type-feature A feature request or enhancement 3.13 bugs and security fixes and removed awaiting changes labels Dec 2, 2023
@gpshead gpshead merged commit 81ee026 into python:main Dec 5, 2023
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…ython#110778)

Add a track parameter to shared memory to allow resource tracking via the side-launched resource tracker process to be disabled on platforms that use it (POSIX).

This allows people who do not want automated cleanup at process exit because they are using the shared memory with processes not participating in Python's resource tracking to use the shared_memory API.

Co-authored-by: Łukasz Langa <lukasz@langa.pl>
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
…ython#110778)

Add a track parameter to shared memory to allow resource tracking via the side-launched resource tracker process to be disabled on platforms that use it (POSIX).

This allows people who do not want automated cleanup at process exit because they are using the shared memory with processes not participating in Python's resource tracking to use the shared_memory API.

Co-authored-by: Łukasz Langa <lukasz@langa.pl>
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants