From 3619cb3e0b06df99043ec4bb665daf168e5dd132 Mon Sep 17 00:00:00 2001 From: Santiago Gimeno Date: Wed, 18 Jan 2023 09:38:01 +0100 Subject: [PATCH 1/4] Revert "src: let http2 streams end after session close" This reverts commit dee882e94f4caa4e1cd608013f90f6a14629403f. Moved the test that demonstrated what this commit was fixing to the `known_issues` folder. Fixes: https://github.com/nodejs/node/issues/46234 --- src/node_http2.cc | 11 ----------- .../test-http2-trailers-after-session-close.js | 1 + 2 files changed, 1 insertion(+), 11 deletions(-) rename test/{parallel => known_issues}/test-http2-trailers-after-session-close.js (95%) diff --git a/src/node_http2.cc b/src/node_http2.cc index bb4057c9925676..4209a1e1910d3b 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1124,17 +1124,6 @@ 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->can_call_into_js()) { - 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/known_issues/test-http2-trailers-after-session-close.js similarity index 95% rename from test/parallel/test-http2-trailers-after-session-close.js rename to test/known_issues/test-http2-trailers-after-session-close.js index d08f875494139c..43c76d8b74392d 100644 --- a/test/parallel/test-http2-trailers-after-session-close.js +++ b/test/known_issues/test-http2-trailers-after-session-close.js @@ -1,5 +1,6 @@ 'use strict'; +// Fixes: https://github.com/nodejs/node/issues/42713 const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); From cceb0187f692afa7ccf032ecc0a0aef1c328e34f Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 19 Feb 2023 07:21:57 -0800 Subject: [PATCH 2/4] Update test/known_issues/test-http2-trailers-after-session-close.js --- test/known_issues/test-http2-trailers-after-session-close.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/known_issues/test-http2-trailers-after-session-close.js b/test/known_issues/test-http2-trailers-after-session-close.js index 43c76d8b74392d..2d7f7b41b04ef5 100644 --- a/test/known_issues/test-http2-trailers-after-session-close.js +++ b/test/known_issues/test-http2-trailers-after-session-close.js @@ -3,7 +3,9 @@ // Fixes: https://github.com/nodejs/node/issues/42713 const common = require('../common'); if (!common.hasCrypto) - common.skip('missing crypto'); + // Change require('assert').fail to common.skip when issue is fixed and test + // is moved out of the known_issues directory + require('assert').fail('missing crypto'); const assert = require('assert'); const http2 = require('http2'); From 2f640df31b05ba191abded95b2af4ef8b9aeeec5 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 19 Feb 2023 07:31:26 -0800 Subject: [PATCH 3/4] Update test/known_issues/test-http2-trailers-after-session-close.js --- .../test-http2-trailers-after-session-close.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/known_issues/test-http2-trailers-after-session-close.js b/test/known_issues/test-http2-trailers-after-session-close.js index 2d7f7b41b04ef5..9bd5c4cdf5c246 100644 --- a/test/known_issues/test-http2-trailers-after-session-close.js +++ b/test/known_issues/test-http2-trailers-after-session-close.js @@ -2,7 +2,12 @@ // Fixes: https://github.com/nodejs/node/issues/42713 const common = require('../common'); -if (!common.hasCrypto) +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'); +} // Change require('assert').fail to common.skip when issue is fixed and test // is moved out of the known_issues directory require('assert').fail('missing crypto'); From f3d066c2505acad79d5cf8586ca5be2bd52ceac5 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 19 Feb 2023 07:31:48 -0800 Subject: [PATCH 4/4] Update test/known_issues/test-http2-trailers-after-session-close.js --- test/known_issues/test-http2-trailers-after-session-close.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/known_issues/test-http2-trailers-after-session-close.js b/test/known_issues/test-http2-trailers-after-session-close.js index 9bd5c4cdf5c246..7f05fda2b26b72 100644 --- a/test/known_issues/test-http2-trailers-after-session-close.js +++ b/test/known_issues/test-http2-trailers-after-session-close.js @@ -8,9 +8,6 @@ if (!common.hasCrypto) { require('assert').fail('missing crypto'); common.skip('missing crypto'); } - // Change require('assert').fail to common.skip when issue is fixed and test - // is moved out of the known_issues directory - require('assert').fail('missing crypto'); const assert = require('assert'); const http2 = require('http2');