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 Python epoll1 Fork Support #32196

Merged
merged 23 commits into from Feb 2, 2023
Merged

Conversation

gnossen
Copy link
Contributor

@gnossen gnossen commented Jan 25, 2023

This PR resolves several issues in core impacting fork support in Python. It also reenables and improves the existing Python-layer fork test.

Copy link
Member

@drfloob drfloob left a comment

Choose a reason for hiding this comment

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

This is great, Richard! Have you confirmed this iteration order fix yet with the external reproductions?

@Vignesh2208 we'll still need to do some work to decouple the PosixEventEngine from the iomgr fork handlers. But we'll soon have some tests that'll help us do that safely.

src/core/lib/gprpp/fork.cc Show resolved Hide resolved
src/core/lib/gprpp/fork.h Outdated Show resolved Hide resolved
src/core/lib/event_engine/posix_engine/ev_epoll1_linux.h Outdated Show resolved Hide resolved
src/core/lib/iomgr/fork_posix.cc Outdated Show resolved Hide resolved
@gnossen
Copy link
Contributor Author

gnossen commented Jan 27, 2023

Have you confirmed this iteration order fix yet with the external reproductions?

I have specifically confirmed that this fixes the issue for Ray on Mac OS.

Copy link
Member

@drfloob drfloob left a comment

Choose a reason for hiding this comment

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

Looks good!

@gnossen gnossen enabled auto-merge (squash) February 2, 2023 22:12
@gnossen gnossen merged commit 190d095 into grpc:master Feb 2, 2023
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Feb 3, 2023
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
* WIP. A seemingly properly failing test

* WIP. Pre-fork handlers now work

* Roughly working.

* Clean up

* Clean up more

* Add to CI

* Format

* Ugh. Remove swap file

* And another

* clean up

* Add copyright

* Format

* Remove another debug line

* Add stub forkable methods

* Remove use of 3.9+ function

* Remove unintentional double copyright

* drfloob review comments

* Only hold lock during Close once

* Create separate job for fork test

* Bump up gdb timeout

* Format
wanlin31 pushed a commit that referenced this pull request May 18, 2023
* WIP. A seemingly properly failing test

* WIP. Pre-fork handlers now work

* Roughly working.

* Clean up

* Clean up more

* Add to CI

* Format

* Ugh. Remove swap file

* And another

* clean up

* Add copyright

* Format

* Remove another debug line

* Add stub forkable methods

* Remove use of 3.9+ function

* Remove unintentional double copyright

* drfloob review comments

* Only hold lock during Close once

* Create separate job for fork test

* Bump up gdb timeout

* Format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test bloat/none imported Specifies if the PR has been imported to the internal repository kind/bug lang/core lang/Python per-call-memory/neutral per-channel-memory/neutral release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants