Skip to content
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

Fix a regression in handling processes that fork #645

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

godlygeek
Copy link
Contributor

We switched from pthread_atfork handlers to os.register_at_fork ones
in order to support Python 3.13, where a lock that our child fork
handler needs to acquire will not have been reinitialized in the child
process at the time when pthread_atfork handlers run but will be
reinitialized by the time os.register_at_fork handlers run.

Unfortunately, this means that if a process forks without calling the
before handler registered with os.register_at_fork (as happens when
using the "spawn" start method for multiprocessing, the default on
macOS), then our hooks are unexpectedly still active from the time the
process forks until the time that it execs another process image.

We can work around this by using a mix of both pthread_atfork and
os.register_at_fork handlers: we suspend tracking in
a pthread_atfork prepare handler, resume it in a pthread_atfork
handler in the parent process, and (if --follow-fork mode is used) we
reinitialize it in the child process from an os.register_at_fork child
handler, after the interpreter's locks have been reinitialized.

Closes: #642

godlygeek added 2 commits July 2, 2024 20:53
We switched from `pthread_atfork` handlers to `os.register_at_fork` ones
in order to support Python 3.13, where a lock that our child fork
handler needs to acquire will not have been reinitialized in the child
process at the time when `pthread_atfork` handlers run but will be
reinitialized by the time `os.register_at_fork` handlers run.

Unfortunately, this means that if a process forks without calling the
`before` handler registered with `os.register_at_fork` (as happens when
using the "spawn" start method for multiprocessing, the default on
macOS), then our hooks are unexpectedly still active from the time the
process forks until the time that it execs another process image.

We can work around this by using a mix of both `pthread_atfork` and
`os.register_at_fork` handlers: we suspend tracking in
a `pthread_atfork` prepare handler, resume it in a `pthread_atfork`
handler in the parent process, and (if `--follow-fork` mode is used) we
reinitialize it in the child process from an `os.register_at_fork` child
handler, after the interpreter's locks have been reinitialized.

Signed-off-by: Matt Wozniski <[email protected]>
See changelog for more details.

Signed-off-by: Matt Wozniski <[email protected]>
@godlygeek godlygeek self-assigned this Jul 3, 2024
@godlygeek godlygeek requested a review from pablogsal July 3, 2024 00:59
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.06%. Comparing base (41248ed) to head (41ce62f).
Report is 103 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #645      +/-   ##
==========================================
+ Coverage   92.55%   93.06%   +0.51%     
==========================================
  Files          91       94       +3     
  Lines       11304    11415     +111     
  Branches     1581     2092     +511     
==========================================
+ Hits        10462    10623     +161     
+ Misses        837      792      -45     
+ Partials        5        0       -5     
Flag Coverage Δ
cpp 93.06% <100.00%> (+7.12%) ⬆️
python_and_cython 93.06% <100.00%> (-2.66%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pablogsal pablogsal merged commit 3b15301 into bloomberg:main Jul 3, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memray ERROR: Invalid record type when creating reports in v1.13.2
3 participants