[event_engine] Implement fork support in Posix Event Engine#38980
[event_engine] Implement fork support in Posix Event Engine#38980eugeneo wants to merge 17 commits intogrpc:masterfrom
Conversation
40106d9 to
d5d2b1b
Compare
src/core/lib/event_engine/posix_engine/file_descriptor_collection.h
Outdated
Show resolved
Hide resolved
drfloob
left a comment
There was a problem hiding this comment.
My comments are fairly minor. I'm glad to see the fork tests passing! Can you confirm that the fork tests pass say 100k times each with opt and asan builds, just to be sure? You can run them with 2000 RBE jobs, it should take a few minutes.
eugeneo
left a comment
There was a problem hiding this comment.
Thanks for the review. Addressed the comments.
markdroth
left a comment
There was a problem hiding this comment.
This is getting close!
Please let me know if you have any questions. Thanks!
src/core/lib/event_engine/posix_engine/posix_interface_posix.cc
Outdated
Show resolved
Hide resolved
eugeneo
left a comment
There was a problem hiding this comment.
Thank you for the review! I addressed the comments. Please take another look.
src/core/lib/event_engine/posix_engine/posix_interface_posix.cc
Outdated
Show resolved
Hide resolved
markdroth
left a comment
There was a problem hiding this comment.
Other than a couple of minor nits, this looks great to me!
markdroth
left a comment
There was a problem hiding this comment.
This looks great!
Let's make sure everyone's happy with the rollback story before we merge this.
- Moves fork handling from behind the event_engine_fork experiment
- No experiment (parity with original fork implementation):
- Thread pool and timer manager will go through the fork as expected
- Epoll poller fd will be recreated in the child
- EventHandles are closed in the child process.
- Behind experiment (new functionality):
- File descriptors are tracked in the collection
- FD generation is tracked and checked on every Posix call.
- AresResolver is reset on fork
- Fixes to address race between PosixEngine destructor and fork
handling.
- Thread pool will properly handle fork even if the parent PosixEngine
instance entered the destructor before the fork call
- Thread pool fork handler will wait for the parent event engine
destructor to finish before shutting down threads.
markdroth
left a comment
There was a problem hiding this comment.
Only a couple of minor nits here, otherwise looks great!
| absl::StrCat("getsockname:", grpc_core::StrError(errno))); | ||
| } | ||
| return EventEngine::ResolvedAddress(addr.address(), len); | ||
| return grpc_event_engine::experimental::LocalAddress(fd.fd()); |
There was a problem hiding this comment.
I don't think we need the grpc_event_engine::experimental:: prefix, since we're already inside of that namespace.
There was a problem hiding this comment.
I was able to inline that function now, I only had it separate for internal code. Internal code now uses EventEnginePosixInterface too.
| for (const auto& engine : LockEventEngines()) { | ||
| engine->BeforeFork(); | ||
| for (const auto& [event_engine, executor] : LockForkHandlers()) { | ||
| if (event_engine) { |
There was a problem hiding this comment.
For pointer types, please prefer to say != nullptr rather than evaluating them as booleans.
Same thing on throughout.
Co-authored-by: eugeneo <287917+eugeneo@users.noreply.github.com>
…k/11-can-tests-pass
No description provided.