GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-77377: Ensure multiprocessing SemLock is valid for spawn-based Process before serializing it by albanD · Pull Request #107275 · python/cpython

Conversation

@albanD

@albanD albanD commented Jul 25, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This fixes both the example in the initial report #77377 and other reports on the PyTorch repo.

Note that as far as I can tell, all objects that lead to segfault with mixed context are all caused by SemLock created with fork sent over a Spawn process. That's why it is the only object updated here and the only pair of ctx covered. Let me know if I'm mistaken and I can find a way to update other objects as well.
Also windows behavior is not changed here (and thus not tested).

Please let me know if the error message and/or the test are not appropriate and I can work on improving them.

  • Issue: multiprocessing: serialization must ensure that contexts are compatible (the same) #77377

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot mentioned this pull request Jun 6, 2022
multiprocessing: serialization must ensure that contexts are compatible (the same) #77377
Closed
@bedevere-bot bedevere-bot added the awaiting review label Jul 25, 2023
@bedevere-bot

bedevere-bot commented Jul 25, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@albanD albanD mentioned this pull request Jul 26, 2023
Dataloader with multiprocessing_context="spawn" fails with multiprocessing.Value pytorch/pytorch#105514
Closed
pitrou
pitrou reviewed Aug 16, 2023
View reviewed changes
Comment thread Lib/test/_test_multiprocessing.py Outdated
s = SemLock(1, 0, 10, name, False)
_multiprocessing.sem_unlink(name)

def test_semlock_mixed_context(self):

@pitrou pitrou Aug 16, 2023

Copy link
Copy Markdown
Member

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.

Does this test actually need to be Linux-only? (see SemLockTests definition above)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@albanD albanD Aug 23, 2023

Copy link
Copy Markdown
Contributor Author

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 is indeed also relevant on macos. Moved the test to be able to test it there.
On windows, there is only spawn, so there is no way to get "mixed" context.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/_test_multiprocessing.py Outdated

def test_semlock_mixed_context(self):
queue = multiprocessing.get_context("fork").Queue()
p = multiprocessing.get_context("spawn").Process(target=close_queue, args=(queue,))

@pitrou pitrou Aug 16, 2023

Copy link
Copy Markdown
Member

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.

Do we also want to test with "forkserver" method?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@albanD albanD Aug 23, 2023

Copy link
Copy Markdown
Contributor Author

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.

Good catch! Added it.
It now ensure that all cases work. Let me know if that properly covers all the cases you had in mind.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
albanD and others added 3 commits August 23, 2023 14:51
@albanD
Ensure multiprocessing lock is valid for spawn Process before seriali…
d9aeedf
…zing it
@blurb-it @albanD
📜🤖 Added by blurb_it.
df69347
@albanD
Improve test to run on macos and cover forkserver
152800c
@albanD
albanD force-pushed the fix-issue-77377 branch from b6b25ff to 152800c Compare August 23, 2023 18:52
@pitrou
Fix typos and wording
29d37e1
pitrou
pitrou reviewed Aug 23, 2023
View reviewed changes
Comment thread Lib/test/_test_multiprocessing.py Outdated
print(err)
self.fail("failed spawning forkserver or grandchild")

@unittest.skipIf(sys.platform == "win32", "Only Spawn on windows so no risk of mixing")

@pitrou pitrou Aug 23, 2023

Copy link
Copy Markdown
Member

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.

Style nit, but can you try to wrap the lines below around ~80 characters max?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

albanD reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@albanD albanD Aug 23, 2023

Copy link
Copy Markdown
Contributor Author

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.

Updated!
Thanks for the update on the blurb, I wasn't sure what to put there

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@albanD
lint
e5f4e32
@pitrou pitrou added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Aug 23, 2023
@pitrou
pitrou enabled auto-merge (squash) August 23, 2023 20:26
@pitrou
pitrou merged commit 1700d34 into python:main Aug 23, 2023
@miss-islington

miss-islington commented Aug 23, 2023

Copy link
Copy Markdown
Contributor

Thanks @albanD for the PR, and @pitrou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

albanD reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the awaiting review label Aug 23, 2023
@bedevere-bot

bedevere-bot commented Aug 23, 2023

Copy link
Copy Markdown

GH-108377 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Aug 23, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 23, 2023
@albanD @blurb-it
@pitrou @miss-islington
pythongh-77377: Ensure multiprocessing SemLock is valid for spawn-bas…
53476dd
…ed Process before serializing it (pythonGH-107275)

Ensure multiprocessing SemLock is valid for spawn Process before serializing it.

Creating a multiprocessing SemLock with a fork context, and then trying to pass it to a spawn-created Process, would segfault if not detected early.

---------

(cherry picked from commit 1700d34)

Co-authored-by: albanD <[email protected]>
Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Co-authored-by: Antoine Pitrou <[email protected]>
@bedevere-bot

bedevere-bot commented Aug 23, 2023

Copy link
Copy Markdown

GH-108378 is a backport of this pull request to the 3.11 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Aug 23, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 23, 2023
@albanD @blurb-it
@pitrou @miss-islington
pythongh-77377: Ensure multiprocessing SemLock is valid for spawn-bas…
79f745b
…ed Process before serializing it (pythonGH-107275)

Ensure multiprocessing SemLock is valid for spawn Process before serializing it.

Creating a multiprocessing SemLock with a fork context, and then trying to pass it to a spawn-created Process, would segfault if not detected early.

---------

(cherry picked from commit 1700d34)

Co-authored-by: albanD <[email protected]>
Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Co-authored-by: Antoine Pitrou <[email protected]>
@albanD
albanD deleted the fix-issue-77377 branch August 23, 2023 21:03
pitrou added a commit that referenced this pull request Aug 23, 2023
@miss-islington @albanD
@blurb-it @pitrou
[3.11] gh-77377: Ensure multiprocessing SemLock is valid for spawn-ba…
34ef75d
…sed Process before serializing it (GH-107275) (#108378)

gh-77377: Ensure multiprocessing SemLock is valid for spawn-based Process before serializing it (GH-107275)

Ensure multiprocessing SemLock is valid for spawn Process before serializing it.

Creating a multiprocessing SemLock with a fork context, and then trying to pass it to a spawn-created Process, would segfault if not detected early.

---------

(cherry picked from commit 1700d34)

Co-authored-by: albanD <[email protected]>
Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Co-authored-by: Antoine Pitrou <[email protected]>
Yhg1s pushed a commit that referenced this pull request Aug 23, 2023
@miss-islington @albanD
@blurb-it @pitrou
[3.12] gh-77377: Ensure multiprocessing SemLock is valid for spawn-ba…
9cf49aa
…sed Process before serializing it (GH-107275) (#108377)

gh-77377: Ensure multiprocessing SemLock is valid for spawn-based Process before serializing it (GH-107275)

Ensure multiprocessing SemLock is valid for spawn Process before serializing it.

Creating a multiprocessing SemLock with a fork context, and then trying to pass it to a spawn-created Process, would segfault if not detected early.

---------

(cherry picked from commit 1700d34)

Co-authored-by: albanD <[email protected]>
Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Co-authored-by: Antoine Pitrou <[email protected]>
@pitrou pitrou mentioned this pull request Aug 30, 2023
gh-108520: Fix bad fork detection in nested multiprocessing use case #108568
Merged
pitrou added a commit that referenced this pull request Aug 30, 2023
@albanD @blurb-it @pitrou
gh-108520: Fix bad fork detection in nested multiprocessing use case (#…
add8d45
…108568)

gh-107275 introduced a regression where a SemLock would fail being passed along nested child processes, as the `is_fork_ctx` attribute would be left missing after the first deserialization.

---------

Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Co-authored-by: Antoine Pitrou <[email protected]>
@miss-islington miss-islington mentioned this pull request Aug 30, 2023
[3.12] gh-108520: Fix bad fork detection in nested multiprocessing use case (GH-108568) #108691
Merged
@miss-islington miss-islington mentioned this pull request Aug 30, 2023
[3.11] gh-108520: Fix bad fork detection in nested multiprocessing use case (GH-108568) #108692
Merged
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 30, 2023
@albanD @blurb-it
@pitrou @miss-islington
pythongh-108520: Fix bad fork detection in nested multiprocessing use…
c62507f
… case (pythonGH-108568)

pythongh-107275 introduced a regression where a SemLock would fail being passed along nested child processes, as the `is_fork_ctx` attribute would be left missing after the first deserialization.

---------

(cherry picked from commit add8d45)

Co-authored-by: albanD <[email protected]>
Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Co-authored-by: Antoine Pitrou <[email protected]>
pitrou added a commit that referenced this pull request Aug 30, 2023
@miss-islington @albanD
@blurb-it @pitrou
[3.11] gh-108520: Fix bad fork detection in nested multiprocessing us…
385b195
…e case (GH-108568) (#108692)

gh-107275 introduced a regression where a SemLock would fail being passed along nested child processes, as the `is_fork_ctx` attribute would be left missing after the first deserialization.

---------

(cherry picked from commit add8d45)

Co-authored-by: albanD <[email protected]>
Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Co-authored-by: Antoine Pitrou <[email protected]>
Yhg1s pushed a commit that referenced this pull request Aug 30, 2023
@miss-islington @albanD
@blurb-it @pitrou
[3.12] gh-108520: Fix bad fork detection in nested multiprocessing us…
320d398
…e case (GH-108568) (#108691)

gh-108520: Fix bad fork detection in nested multiprocessing use case (GH-108568)

gh-107275 introduced a regression where a SemLock would fail being passed along nested child processes, as the `is_fork_ctx` attribute would be left missing after the first deserialization.

---------

(cherry picked from commit add8d45)

Co-authored-by: albanD <[email protected]>
Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Co-authored-by: Antoine Pitrou <[email protected]>
@Eclips4 Eclips4 mentioned this pull request May 22, 2024
multiprocessing child segfault when passing lock as an argument #119413
Closed
@evnchn evnchn mentioned this pull request May 8, 2026
Runtime Error when trying to run native=True zauberzeug/nicegui#1841
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitrou pitrou pitrou left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@albanD @bedevere-bot @miss-islington @pitrou
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.