Skip to content

gh-110395: invalidate open kqueues after fork #110517

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 3 commits into from
Nov 4, 2023
Merged

Conversation

sorcio
Copy link
Contributor

@sorcio sorcio commented Oct 8, 2023

Track select.kqueue objects with an open fd in a linked list, so they can be invalidated after fork. The invalidation happens through posix.register_at_fork() for lack of a better/safer idea.

This PR does some extra allocations, but the number of kqueues you'll open in a program is normally very small, and otherwise it's very lightweight (tracking is only involved at open/close/fork).

I thought of different approaches but I'm not convinced:

  • we could check the process pid. But it effectively requires to check at every operation on the kqueue. Performance impact would not be major, but still requires all code to pay for a corner case.
  • add some sort of .detach() method to invalidate the object without closing the fd. Leaves the burden of correct cleanup to users.
  • don't call close() on object destructor. Might make sense if combined with a check on pid at creation/destruction time. But still leaves some burden to users.

@sorcio sorcio requested a review from rhettinger as a code owner October 8, 2023 12:20
@rhettinger rhettinger removed their request for review October 10, 2023 05:39
@corona10 corona10 requested review from a team and vstinner October 22, 2023 14:00
Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

Overall I think this PR makes sense. Additional eyeballs from others such as @vstinner on what looks like module C API modernization with the tp_dealloc removal in favor of a tp_finalize would be helpful.

@gpshead gpshead self-assigned this Oct 31, 2023
@gpshead gpshead added the needs backport to 3.12 only security fixes label Oct 31, 2023
Make the child process _always_ exit even on test failure.
@gpshead
Copy link
Member

gpshead commented Nov 3, 2023

I double checked the tp_dealloc and tp_finalize stuff and gave this another look. it's good, thanks!

@gpshead gpshead enabled auto-merge (squash) November 3, 2023 21:16
@sorcio
Copy link
Contributor Author

sorcio commented Nov 4, 2023

Thanks for the review! CI failure looks spurious and unrelated to this change. Anything else I have to do?

@vstinner
Copy link
Member

vstinner commented Nov 4, 2023

test_asyncio test_unhandled_exceptions() failed on Windows x86: it's a known bug, fixed in the main branch. I re-run the job. If it fails too often, you can consider to rebase your PR on the main branch to get my test_unhandled_exceptions() fix.

@gpshead gpshead merged commit a6c1c04 into python:main Nov 4, 2023
@miss-islington-app
Copy link

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

@miss-islington-app
Copy link

Sorry, @sorcio and @gpshead, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker a6c1c04d4d2339f0094422974ae3f26f8c7c8565 3.12

gpshead pushed a commit to gpshead/cpython that referenced this pull request Nov 4, 2023
…110517)

Invalidate open select.kqueue instances after fork as the fd will be invalid in the child.
(cherry picked from commit a6c1c04)

Co-authored-by: Davide Rizzo <[email protected]>
gpshead pushed a commit to gpshead/cpython that referenced this pull request Nov 4, 2023
…110517)

Invalidate open select.kqueue instances after fork as the fd will be invalid in the child.
(cherry picked from commit a6c1c04)

Co-authored-by: Davide Rizzo <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Nov 4, 2023

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

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Nov 4, 2023
gpshead added a commit that referenced this pull request Nov 11, 2023
…1745)

* [3.12] gh-110395: invalidate open kqueues after fork (GH-110517)

Invalidate open select.kqueue instances after fork as the fd will be invalid in the child.
(cherry picked from commit a6c1c04)

Co-authored-by: Davide Rizzo <[email protected]>

* move assert to after the child dying

this is in `main` via https://github.com/python/cpython/pull/111816/files
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Invalidate open select.kqueue instances after fork as the fd will be invalid in the child.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Invalidate open select.kqueue instances after fork as the fd will be invalid in the child.
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.

3 participants