gh-92408/gh-96026: SharedMemory changes #133227
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`multiprocessing.shared_memory.SharedMemory` currently tries to unlink a POSIX shared memory object if it fails to `mmap` it after opening/creating it. However, since it only catches `OSError`, it fails to do so when opening an existing zero-length shared memory object, as `mmap.mmap` raises a `ValueError` when given a 0 size. Fix this by catching all exceptions with a bare `except`, so the cleanup is run for all errors.
… handling `multiprocessing.shared_memory.SharedMemory` currently tries to unlink a POSIX shared memory object if it fails to mmap it after opening/creating it in the constructor. It does this by calling `cleanup()`, which unregisters the shared memory despite it not having been registered. This will trigger a separate exception in the resource tracker. Fix this by calling the low-level unlink directly.
`multiprocessing.shared_memory.SharedMemory` currently attempts to `unlink` a POSIX shared memory object it has opened or created if a subsequent exception is raised in its constructor. This is necessary to avoid it leaking if the shared memory object was created in the constructor, however it seems inappropriate if it is opening a pre-existing shared memory object. Fix this by only unlinking the shared memory object if it was created.
These are allowed by POSIX and it seems more consistent/cleaner to support them than forcing the user to treat them as a special case. Windows does not support creating a zero-length mapping, and neither does `mmap` on any platform, so skip those operations and assign a zero-length buffer instead. TODO: The way the POSIX/Windows code is interleaved inside if statements makes for difficult to follow and deeply nested logic. It might be better to refactor this code and keep the platform-specific logic in separate functions or possibly even implementation classes.
|
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 |
It appears MacOS has tight limits on name length which the new tests were exceeding.
|
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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is not for merging so much as discussion. It would probably be best split into several PRs: allowing zero-length shared memory objects at least should be separate, and probably should have its own issue.