Handle GOAWAY frames with NO_ERROR in Node.js clients #681
Conversation
| if (errorCode === http2.constants.NGHTTP2_NO_ERROR) { | ||
| receivedGoAwayNoError = true; | ||
| const nodeMajor = parseInt(process.versions.node.split(".")[0], 10); | ||
| // Node.js v16 closes a connection on its own when it receives a GOAWAY | ||
| // frame and there are no open streams (emitting a "close" event and | ||
| // destroying the session), but more recent versions do not. | ||
| if (streamCount == 0 && nodeMajor >= 18) { | ||
| conn.destroy( | ||
| new ConnectError( | ||
| "received GOAWAY without any open streams", | ||
| Code.Canceled | ||
| ), | ||
| http2.constants.NGHTTP2_NO_ERROR | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
In case it's relevant:
- grpc.js always close the connection https://github.com/grpc/grpc-node/blob/186718cb70af708c75584909b21169289d7363b2/packages/grpc-js/src/transport.ts#L198-L212
- the server will also do the same, close the connection after sending
goawayhttps://github.com/grpc/proposal/blob/master/A8-client-side-keepalive.md#server-enforcement
There was a problem hiding this comment.
// Node.js v16 closes a connection on its own when it receives a GOAWAY // frame and there are no open streams (emitting a "close" event and // destroying the session), but more recent versions do not.
Does it mean the bug shouldn't happen in node 16? I'm using node 16 and I can confirm it does happen
There was a problem hiding this comment.
I'm not sure if @grpc/grpc-js is really always closing the connection when receiving a GOAWAY. I've updated the PR description with details about the graceful shutdown. I would expect an HTTP/2 client to drain streams first. It is possible that @grpc/grpc-js does not do so for historical reasons.
Note that there is a distinction between error code NO_ERROR and actual error codes, and also the special behavior in case of ENHANCE_YOUR_CALM (last paragraph in basic keepalive).
Regarding the comment: The http2 module implements graceful shutdown. It rejects new streams, and it automatically closes the connection after all streams have finished. But if a connection without open streams receives a GOAWAY with NO_ERROR, behavior differs between versions. Seeing #680 in v16 is expected.
In v0.10.0, we added support for basic keepalive for Node.js HTTP/2 clients (implemented in #673).
We made a mistake and expected all GOAWAY frames to close a client session, but that is not the case for the HTTP/2 error code NO_ERROR:
The graceful shutdown is further specified:
The Node.js
http2module implements this behavior, and raises an error when new request is issued on a session. This is the issue we are seeing with #680.This PR fixes #680 by opening a new connection if the current one has received a GOAWAY with NO_ERROR. Connections that are gracefully shutting down are maintained until all streams have finished.