http2: allow streams to complete gracefully after goaway

A detailed analysis of the cause of this bug is in my linked comment on
the corresponding issue. The primary fix is the new setImmediate call in
Http2Stream#_destroy, which prevents a re-entrant call into
Http2Session::SendPendingData when sending trailers after the
Http2Session has been shut down, allowing the trailer data to be flushed
properly before the socket is closed.

As a result of this change, writes can be initiated later in the
lifetime of the Http2Session. So, when a JSStreamSocket is used as the
underlying socket reference for an Http2Session, it needs to be able to
accept write calls after it is closed.

In addition, now that outgoing data can be flushed differently after a
session is closed, in two tests clients receive errors that they
previously did not receive. I believe the new errors are more correct,
so I changed the tests to match.

Fixes: #42713
Refs: #42713 (comment)
PR-URL: #50202
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>

Author: murgatroid99

lib/internal/http2/core.js

@@ -2358,8 +2358,11 @@ class Http2Stream extends Duplex {
     // 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.
-    session[kMaybeDestroy]();
+    // pending streams. Delay with setImmediate so we don't do it on the
+    // nghttp2 stack.
+    setImmediate(() => {
+      session[kMaybeDestroy]();
+    });
     callback(err);
   }
   // The Http2Stream can be destroyed if it has closed and if the readable

lib/internal/js_stream_socket.js

@@ -179,10 +179,13 @@ class JSStreamSocket extends Socket {
       // anything. doClose will call finishWrite with ECANCELED for us shortly.
       this[kCurrentWriteRequest] = req; // Store req, for doClose to cancel
       return 0;
+    } else if (this._handle === null) {
+      // If this._handle is already null, there is nothing left to do with a
+      // pending write request, so we discard it.
+      return 0;
     }
 
     const handle = this._handle;
-    assert(handle !== null);
 
     const self = this;
 

test/parallel/test-h2-large-header-cause-client-to-hangup.js

@@ -7,7 +7,7 @@ const http2 = require('http2');
 const assert = require('assert');
 const {
   DEFAULT_SETTINGS_MAX_HEADER_LIST_SIZE,
-  NGHTTP2_CANCEL,
+  NGHTTP2_FRAME_SIZE_ERROR,
 } = http2.constants;
 
 const headerSize = DEFAULT_SETTINGS_MAX_HEADER_LIST_SIZE;
@@ -28,11 +28,17 @@ server.listen(0, common.mustCall(() => {
   function send() {
     const stream = clientSession.request({ ':path': '/' });
     stream.on('close', common.mustCall(() => {
-      assert.strictEqual(stream.rstCode, NGHTTP2_CANCEL);
+      assert.strictEqual(stream.rstCode, NGHTTP2_FRAME_SIZE_ERROR);
       clearTimeout(timer);
       server.close();
     }));
 
+    stream.on('error', common.expectsError({
+      code: 'ERR_HTTP2_STREAM_ERROR',
+      name: 'Error',
+      message: 'Stream closed with error code NGHTTP2_FRAME_SIZE_ERROR'
+    }));
+
     stream.end();
   }
 }));

test/parallel/test-http2-exceeds-server-trailer-size.js

@@ -43,9 +43,13 @@ server.listen(0, () => {
   const clientStream = clientSession.request();
 
   clientStream.on('close', common.mustCall());
-  // These events mustn't be called once the frame size error is from the server
+  clientStream.on('error', common.expectsError({
+    code: 'ERR_HTTP2_STREAM_ERROR',
+    name: 'Error',
+    message: 'Stream closed with error code NGHTTP2_FRAME_SIZE_ERROR'
+  }));
+  // This event mustn't be called once the frame size error is from the server
   clientStream.on('frameError', common.mustNotCall());
-  clientStream.on('error', common.mustNotCall());
 
   clientStream.end();
 });

test/known_issues/test-http2-trailers-after-session-close.js test/parallel/test-http2-trailers-after-session-close.js

@@ -3,9 +3,6 @@
 // Fixes: https://github.com/nodejs/node/issues/42713
 const common = require('../common');
 if (!common.hasCrypto) {
-  // Remove require('assert').fail when issue is fixed and test
-  // is moved out of the known_issues directory.
-  require('assert').fail('missing crypto');
   common.skip('missing crypto');
 }
 const assert = require('assert');