-
Notifications
You must be signed in to change notification settings - Fork 523
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
Apply shutdown timeout to http server to limit reload delay #14339
Conversation
This pull request does not have a backport label. Could you fix it @carsonip? 🙏
|
|
// httpServer should stop before grpcServer to avoid a panic caused by placing a new connection into | ||
// a closed grpc connection channel during shutdown. | ||
// See https://github.com/elastic/gmux/issues/13 | ||
s.httpServer.stop() | ||
s.grpcServer.GracefulStop() |
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.
[to reviewer] Moving grpcServer.GracefulStop
ahead of httpServer.stop
, as this was how the code was originally written before elastic/gmux#13 was discovered. Now that it is fixed, we can revert it, so that it sends GOAWAY to grpc clients.
This pull request is now in conflicts. Could you fix it @carsonip? 🙏
|
httpServer.stop may block indefinitely, potentially due to misbehaving connections. Apply shutdown timeout to http server so that in a hot reload, the old and new server overlapping time is bounded. (cherry picked from commit 24329c8)
…14362) httpServer.stop may block indefinitely, potentially due to misbehaving connections. Apply shutdown timeout to http server so that in a hot reload, the old and new server overlapping time is bounded. (cherry picked from commit 24329c8) Co-authored-by: Carson Ip <[email protected]>
Motivation/summary
httpServer.stop may block indefinitely, potentially due to misbehaving connections.
Apply shutdown timeout to http server so that in a hot reload, the old and new server overlapping time is bounded.
Checklist
For functional changes, consider:
How to test these changes
May need to change the code to hang the http server to observe the shutdown timeout. Alternatively there may be a way to mock a misbehaving http client without code change.
Related issues
Part of #14337