Skip to content

gh-92408/gh-96026: SharedMemory changes #133227

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

duaneg
Copy link

@duaneg duaneg commented Apr 30, 2025

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.

duaneg added 4 commits May 1, 2025 11:12
`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.
@bedevere-app
Copy link

bedevere-app bot commented Apr 30, 2025

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.

It appears MacOS has tight limits on name length which the new tests were
exceeding.
@bedevere-app
Copy link

bedevere-app bot commented May 1, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant