-
Notifications
You must be signed in to change notification settings - Fork 651
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
tryShutdown
results in cancelled Unary GRPCs
#2091
Comments
It's definitely not supposed to do that, for unary or streaming RPCs. And the Have you tried listening for |
I will try |
Ok, confirmed with a timer. https://github.com/artificial-aidan/grpc/tree/aidan/call-cancel In one terminal run Edit Re-run the test. And you will get a successful message.
|
I debugged it to the level of when the stream events were being handled, but didn't go any deeper than that. With grpc debugging and http2 debugging turned on here is the client and server output: Server
Client
|
On my machine, your example works with earlier versions of Node. It looks like they broke something in Node 16.7. |
I was able to confirm the same. |
I have filed nodejs/node#42713 |
The linked issue was closed but graceful shutdown still doesn't appear to work even on newer NodeJS versions. |
I have not duplicated that problem in newer versions of Node. Can you share your test code that triggers this problem, so that we can provide more details in that Node issue about when it is still broken? |
Sorry, my minimal reproduction did not show any problem. I'm gonna research further. |
It seems that this is once again broken on new Node versions (>= 18.15.0 and >= 19.8.0) . They reverted the fix recently... : |
Had to upgrade some infra to node 18+, as I read @eplightning's comment, 18.14 should be a safe bet to not have this issue right? I traced through the release notes a bit and think I found that as well, but just double checking. |
This has been fixed in Node 20.10.0. |
Problem description
When using
tryShutdown
to gracefully shutdown a server, it doesn't let unary RPCs finish successfully.Reproduction steps
process.on
handler forSIGTERM
SIGTERM
to the server process.Environment
Additional context
The normal process for a Unary RPC (that I can deduce from debugging) is as follows:
Client
opens stream toServer
Client
sends request toServer
Server
sends response toClient
Client
closes stream.When using the
tryShutdown
method the process is different.Client
opens stream toServer
Client
sends request toServer
tryShutdown
is calledServer
callsclose
on http2 sessionServer
sends response toClient
Client
see's that stream has been cancelled, and errors.The cancellation appears to be coming from the session destruction happening in
tryShutdown
. This makes sense for streaming GRPC calls, as they need to be cancelled, but a Unary GRPC should be able to finish its response without the call being cancelled.Removing the
session.close()
fromtryShutdown
fixes the problem for Unary GRPCs, but obviously doesn't for Streaming GRPCs.The text was updated successfully, but these errors were encountered: