-
Notifications
You must be signed in to change notification settings - Fork 10.6k
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
ruby: Raise instead of hanging if grpc is used before and after fork #16332
Conversation
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
|
|
|
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.
Overall I think this looks good. One nit comment so far and I'll come back for another look.
One hesitation is that this is a sort of "best effort" debugging aid, and so it's possible that gRPC can still be "used" without running in to this fork guard - but I think the tradeoff there is ok.
Yes, it is possible that grpc can be used without running into the fork guard, but not every use is resulting in ruby hanging, so I focused on adding the guard to methods that would hang when used after forking with grpc initialized before forking. I've added a couple more fork guards to handle server usage and making rpc calls using the Raising an exception instead of crashing is meant to be a debugging aid, but it wasn't meant to be "best effort" in preventing the issue. Is there any specific usage in mind that you think should be protected by a fork guard? |
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.
Looks good, can we please squash down these commits into one?
src/ruby/ext/grpc/rb_grpc.c
Outdated
grpc_init(); | ||
atexit(grpc_rb_shutdown); | ||
} | ||
|
||
void grpc_ruby_fork_guard() { | ||
if (grpc_ruby_forked_after_init()) |
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.
nit, can we use braces around the body of this if?
f9c5cc7
to
7185432
Compare
commits have been squashed |
7185432
to
78f3f44
Compare
|
Fixes #7951
Problem
If grpc core is initialized before forking, then used after forking then it can cause the process to hang or crash. Since grpc core doesn't support using grpc before and after forking, we should make this unsupported usage clear so that it is easy to debug this invalid usage.
Solution
Store the pid for the process that initializes grpc, then check it before creating a new channel or using it in a way that could hang and raise. If the pid changes, then these channel methods that would hang will raise a runtime exception.
These checks aren't needed on windows where it doesn't support forking, so they are stubbed out using conditional compilation.
Regression tests are also included that hang when run without the changes to the code under test.