How to handle server-side session close (sporadic GOAWAY)

[gunters63]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the issue has not already been raised

Issue

In our project we are using fastify and @fastify/http-proxy as a reverse proxy to forward requests to an internal GraphQL service..

Sometimes, very sporadically, we have the effect that the forwarding gets stuck with fastify logging a GOAWAY error.

In the logs the exception looks like this

Error [ERR_HTTP2_GOAWAY_SESSION]: New streams cannot be created after receiving a GOAWAY
              at ClientHttp2Session.request (node:internal/http2/core:1757:13)
              at y (file:///data/asc/application/services/web-server/web-server.mjs:616:12700)

If we look in the Node source code, we can see the error is thrown here:

It seems the GraphQL server somehow closed the connection server-side.
It is currently not clear why he does this. We are still investigating.

Anyway, if we look in the source of fastify-reply-from, we can see the request which threw the exception here (in handleHttp2Req):

This the same source location than our stack trace (file:///data/asc/application/services/web-server/web-server.mjs:616:12700) in our bundled web server.

Strangely, to recover from this error, it doesn´t help to restart the GraphQL server, we have to restart the fastify proxy.

We will still try to find out why the GraphQL server closed the connection from his side in the first place, but beside that:

• Is there a way to recover cleanly from this situation?
• Nodes http2Client.request immediately throws exceptions when the session was destroyed or closed. Wouldn't it be better if fastify-reply-from re-created the httpclient in this case and retried?

[gunters63]

So, if I look at this code in fastify-reply-from:

and then this in Node internal/http2/core.js/ClientHttp2Session:

I am thinking, if the connection for the cached http2Client in fastify-reply-from was closed from the server-side, doesn´t that mean that this connection is stuck forever?

A new httpclient is never created because there is only a check for destroyed. New requests coming into fastify-reply-from now always run into the ERR_HTTP2_GOAWAY_SESSION() exception.

Shouldn´t there be a check for closed too, like:

 if (!http2Client || http2Client.destroyed || http2Client.closed) {

or maybe better

 if (!http2Client || http2Client.destroyed || http2Client.closed) {
   if (http2Client && !http2Client.destroyed) http2Client.destroy()

[Eomm]

Thanks for reporting!

I think we should handle the goaway event:

https://nodejs.org/api/http2.html#event-goaway

Probably like the error one:

fastify-reply-from/lib/request.js

Line 187 in d4bb4f6

http2Client.once('error', done)

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

[gunters63]

Yes, I´d like to make a PR for this.

But I am not really sure about the correct way to handle this.

There is already a GOAWAY handler in Node which also emits the session "goaway" event:

// Receiving a GOAWAY frame from the connected peer is a signal that no
// new streams should be created. If the code === NGHTTP2_NO_ERROR, we
// are going to send our close, but allow existing frames to close
// normally. If code !== NGHTTP2_NO_ERROR, we are going to send our own
// close using the same code then destroy the session with an error.
// The goaway event will be emitted on next tick.
function onGoawayData(code, lastStreamID, buf) {
  const session = this[kOwner];
  if (session.destroyed)
    return;
  debugSessionObj(session, 'goaway %d received [last stream id: %d]',
                  code, lastStreamID);

  const state = session[kState];
  state.goawayCode = code;
  state.goawayLastStreamID = lastStreamID;

  session.emit('goaway', code, lastStreamID, buf);
  if (code === NGHTTP2_NO_ERROR) {
    // If this is a no error goaway, begin shutting down.
    // No new streams permitted, but existing streams may
    // close naturally on their own.
    session.close();
  } else {
    // However, if the code is not NGHTTP_NO_ERROR, destroy the
    // session immediately. We destroy with an error but send a
    // goaway using NGHTTP2_NO_ERROR because there was no error
    // condition on this side of the session that caused the
    // shutdown.
    session.destroy(new ERR_HTTP2_SESSION_ERROR(code), NGHTTP2_NO_ERROR);
  }
}

So, after the "goaway" event was emitted, the session is already either closed (which I think triggers the problem with the stuck proxy) or destroyed.

I think always destroying the session is not the right way to handle this, because we want the existing frames to finish cleanly. If i understand correctly, a server sending GOAWAY won't accept new sessions, but will try to finish existing streams.

Ideally, the "goaway" handler should probably wait for the existing frames to finish and then destroy the session (or does that happen automatically?). I have to research how to do this.

Meanwhile, probably via a flag, the reply-from request handler (handleHttp2Req) should create a new session for incoming new requests.

[mcollina]

yes exactly, that's how it should be handled.

Another option is to implement this in Undici, given that it can handle HTTP/2, and remove all the custom http and http2
code here, massively simplifying the codebase.

[gunters63]

I had a good look again at the code in http2/core.js.

In Http2Stream.._destroy at the end there is this call:

    // This notifies the session that this stream has been destroyed and
    // gives the session the opportunity to clean itself up. The session
    // will destroy if it has been closed and there are no other open or
    // pending streams. Delay with setImmediate so we don't do it on the
    // nghttp2 stack.
    setImmediate(() => {
      session[kMaybeDestroy]();
    });

So it seems whenever a stream finishes, the corresponding session gets destroyed (when it was already closed) when there are no more open streams.

It is not really clear for me how _destroy gets called. I saw no reference anywhere and at the start of the method there is a comment "Called by this.destroy()", but there is no class method called destroyed either. I can only assume that this gets called when a stream closes.

The interesting thing in my use case is, that the client using the fastify proxy has indefinitely running GraphQL subscriptions. So there will be always open streams and that could be the reason this cleanup logic is not be triggered.

The goaway error gets propagated to the proxy client (I saw GOAWAY errors in the browser where the client lives). I would assume that the subscription gets cancelled and retried in my client when a GOAWAY is received (which will in turn trigger the cleanup of the proxies session), but maybe this is not the case. I will check that.

Maybe the only thing needed to change is the line

    if (!http2Client || http2Client.destroyed) {

to

    if (!http2Client || http2Client.destroyed || http2Client.closed) {

so the proxy never gets stuck on a session in closing state.

The session cleanup should be triggered by the client reacting to the propagated GOAWAY errors.

[mcollina]

If that works, that'd be fantastic. Can you send a PR with a test?

[gunters63]

I will try. I am scared a little bit of writing the test for this 😄

[gunters63]

I didn´t have much time lately to work on this issue.

This pnpm patch did solve our issue for now in production:

diff --git a/lib/request.js b/lib/request.js
index 882d19500b4ef8e3f24d26aa8224de9c4a6d0e42..c371bc32238d460b363b58084e3772b987d2a3c3 100644
--- a/lib/request.js
+++ b/lib/request.js
@@ -182,7 +182,8 @@ function buildRequest (opts) {
     let cancelRequest
     let sessionTimedOut = false
 
-    if (!http2Client || http2Client.destroyed) {
+    if (!http2Client || http2Client.destroyed || http2Client.closed) {
+      if (http2Client && !http2Client.destroyed) http2Client.destroy()
       http2Client = http2.connect(baseUrl, http2Opts.sessionOptions)
       http2Client.once('error', done)
       // we might enqueue a large number of requests in this connection

but this is of course only a work-around.

I am not really sure how to handle this problem.

The main reason is that fastify-reply-from has a retry handler (which I still not really understand) and I don't really know how server-side streaming calls should transparently retry after a GOAWAY without breaking the stream consumer.

I started a test to reproduce my problem, but it is not finished now. The branch is here, the new test is http2-goaway-test.js

I try to work on this more, but it will take a while.

[LConstantouris]

Hi, is there any update on fixing this issue?

[gunters63]

I am still using the pnpm patch. The two line fix seems to work for me. I didn´t create a PR because I was not able to write a test to cleanly reproduce the error in a reasonable time. At least not yet.