From 5335e29ce78795010922de89bfb6eb4d3fc63cec Mon Sep 17 00:00:00 2001
From: Santiago Gimeno <santiago.gimeno@gmail.com>
Date: Thu, 27 Oct 2022 11:34:45 +0200
Subject: [PATCH] src: let http2 streams end after session close

After the stream has been marked as closed by the nghttp2 stack, there
might be still pending data to be sent: trailing headers is an example
of this. In that case, avoid reentering the nghttp2 stack synchronously
to allow the data to be written before actually closing the stream.

Fixes: https://github.com/nodejs/node/issues/42713
PR-URL: https://github.com/nodejs/node/pull/45153
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
---
 src/node_http2.cc                             | 11 +++++
 ...test-http2-trailers-after-session-close.js | 49 +++++++++++++++++++
 2 files changed, 60 insertions(+)
 create mode 100644 test/parallel/test-http2-trailers-after-session-close.js

diff --git a/src/node_http2.cc b/src/node_http2.cc
index db841e137095fe..f8f8af3a3dc2e6 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -1120,6 +1120,17 @@ int Http2Session::OnStreamClose(nghttp2_session* handle,
   if (!stream || stream->is_destroyed())
     return 0;
 
+  // Don't close synchronously in case there's pending data to be written. This
+  // may happen when writing trailing headers.
+  if (code == NGHTTP2_NO_ERROR && nghttp2_session_want_write(handle) &&
+      !env->is_stopping()) {
+    env->SetImmediate([handle, id, code, user_data](Environment* env) {
+      OnStreamClose(handle, id, code, user_data);
+    });
+
+    return 0;
+  }
+
   stream->Close(code);
 
   // It is possible for the stream close to occur before the stream is
diff --git a/test/parallel/test-http2-trailers-after-session-close.js b/test/parallel/test-http2-trailers-after-session-close.js
new file mode 100644
index 00000000000000..d08f875494139c
--- /dev/null
+++ b/test/parallel/test-http2-trailers-after-session-close.js
@@ -0,0 +1,49 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const http2 = require('http2');
+
+const {
+  HTTP2_HEADER_PATH,
+  HTTP2_HEADER_STATUS,
+  HTTP2_HEADER_METHOD,
+} = http2.constants;
+
+const server = http2.createServer();
+server.on('stream', common.mustCall((stream) => {
+  server.close();
+  stream.session.close();
+  stream.on('wantTrailers', common.mustCall(() => {
+    stream.sendTrailers({ xyz: 'abc' });
+  }));
+
+  stream.respond({ [HTTP2_HEADER_STATUS]: 200 }, { waitForTrailers: true });
+  stream.write('some data');
+  stream.end();
+}));
+
+server.listen(0, common.mustCall(() => {
+  const port = server.address().port;
+  const client = http2.connect(`http://localhost:${port}`);
+  client.socket.on('close', common.mustCall());
+  const req = client.request({
+    [HTTP2_HEADER_PATH]: '/',
+    [HTTP2_HEADER_METHOD]: 'POST'
+  });
+  req.end();
+  req.on('response', common.mustCall());
+  let data = '';
+  req.on('data', (chunk) => {
+    data += chunk;
+  });
+  req.on('end', common.mustCall(() => {
+    assert.strictEqual(data, 'some data');
+  }));
+  req.on('trailers', common.mustCall((headers) => {
+    assert.strictEqual(headers.xyz, 'abc');
+  }));
+  req.on('close', common.mustCall());
+}));