From 5289fc6adf6f928fe4f3a2539611b9ca2bd29489 Mon Sep 17 00:00:00 2001 From: ofir Date: Sat, 29 Jan 2022 02:14:00 +0200 Subject: [PATCH 1/4] http2: fix no response event on continue request When sending a continue request, server response with null, it does not fires the response event type Fixes: https://github.com/nodejs/node/issues/38258 --- lib/internal/http2/compat.js | 4 +- .../test-http2-compat-expect-continue.js | 107 ++++++++++++------ 2 files changed, 77 insertions(+), 34 deletions(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 0c4519dfff5986..20027f7d19ccb0 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -760,7 +760,9 @@ class Http2ServerResponse extends Stream { } if (chunk !== null && chunk !== undefined) - this.write(chunk, encoding); + chunk = ''; + + this.write(chunk, encoding); state.headRequest = stream.headRequest; state.ending = true; diff --git a/test/parallel/test-http2-compat-expect-continue.js b/test/parallel/test-http2-compat-expect-continue.js index cb90e51f3bc9d7..de36c8ed9d4e90 100644 --- a/test/parallel/test-http2-compat-expect-continue.js +++ b/test/parallel/test-http2-compat-expect-continue.js @@ -6,49 +6,90 @@ if (!common.hasCrypto) const assert = require('assert'); const http2 = require('http2'); -const testResBody = 'other stuff!\n'; +{ + const testResBody = 'other stuff!\n'; -// Checks the full 100-continue flow from client sending 'expect: 100-continue' -// through server receiving it, sending back :status 100, writing the rest of -// the request to finally the client receiving to. + // Checks the full 100-continue flow from client sending 'expect: 100-continue' + // through server receiving it, sending back :status 100, writing the rest of + // the request to finally the client receiving to. -const server = http2.createServer(); + const server = http2.createServer(); -let sentResponse = false; + let sentResponse = false; -server.on('request', common.mustCall((req, res) => { - res.end(testResBody); - sentResponse = true; -})); + server.on('request', common.mustCall((req, res) => { + res.end(testResBody); + sentResponse = true; + })); + + server.listen(0); + + server.on('listening', common.mustCall(() => { + let body = ''; -server.listen(0); + const client = http2.connect(`http://localhost:${server.address().port}`); + const req = client.request({ + ':method': 'POST', + 'expect': '100-continue' + }); -server.on('listening', common.mustCall(() => { - let body = ''; + let gotContinue = false; + req.on('continue', common.mustCall(() => { + gotContinue = true; + })); - const client = http2.connect(`http://localhost:${server.address().port}`); - const req = client.request({ - ':method': 'POST', - 'expect': '100-continue' - }); + req.on('response', common.mustCall((headers) => { + assert.strictEqual(gotContinue, true); + assert.strictEqual(sentResponse, true); + assert.strictEqual(headers[':status'], 200); + req.end(); + })); - let gotContinue = false; - req.on('continue', common.mustCall(() => { - gotContinue = true; + req.setEncoding('utf8'); + req.on('data', common.mustCall((chunk) => { body += chunk; })); + req.on('end', common.mustCall(() => { + assert.strictEqual(body, testResBody); + client.close(); + server.close(); + })); })); +} + +{ + // Checks the full 100-continue flow from client sending 'expect: 100-continue' + // through server receiving it and ending the request. + + const server = http2.createServer(); - req.on('response', common.mustCall((headers) => { - assert.strictEqual(gotContinue, true); - assert.strictEqual(sentResponse, true); - assert.strictEqual(headers[':status'], 200); - req.end(); + server.on('request', common.mustCall((req, res) => { + res.end(); })); - req.setEncoding('utf8'); - req.on('data', common.mustCall((chunk) => { body += chunk; })); - req.on('end', common.mustCall(() => { - assert.strictEqual(body, testResBody); - client.close(); - server.close(); + server.listen(0); + + server.on('listening', common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`); + const req = client.request({ + ":path": "/", + 'expect': '100-continue' + }); + + let gotContinue = false; + req.on('continue', common.mustCall(() => { + gotContinue = true; + })); + + let gotResponse = false; + req.on('response', common.mustCall(() => { + gotResponse = true; + })); + + req.setEncoding('utf8'); + req.on('end', common.mustCall(() => { + assert.strictEqual(gotContinue, true); + assert.strictEqual(gotResponse, true); + client.close(); + server.close(); + })); })); -})); +} From e147b0e07f02d287760f09b6d12bb123cfccdea5 Mon Sep 17 00:00:00 2001 From: ofir Date: Sat, 29 Jan 2022 03:11:06 +0200 Subject: [PATCH 2/4] fix liniting issues --- lib/internal/http2/compat.js | 2 +- test/parallel/test-http2-compat-expect-continue.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 20027f7d19ccb0..90b9fdcc728659 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -760,7 +760,7 @@ class Http2ServerResponse extends Stream { } if (chunk !== null && chunk !== undefined) - chunk = ''; + chunk = ''; this.write(chunk, encoding); diff --git a/test/parallel/test-http2-compat-expect-continue.js b/test/parallel/test-http2-compat-expect-continue.js index de36c8ed9d4e90..d0decb1472a9e1 100644 --- a/test/parallel/test-http2-compat-expect-continue.js +++ b/test/parallel/test-http2-compat-expect-continue.js @@ -70,7 +70,7 @@ const http2 = require('http2'); server.on('listening', common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request({ - ":path": "/", + ':path': '/', 'expect': '100-continue' }); From 8609ae4634e0b9dbf67605f52f3c55cfdfc07fc9 Mon Sep 17 00:00:00 2001 From: ofir Date: Sat, 29 Jan 2022 13:39:06 +0200 Subject: [PATCH 3/4] correct the fix --- lib/internal/http2/compat.js | 4 +--- lib/internal/http2/core.js | 4 +++- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 90b9fdcc728659..0c4519dfff5986 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -760,9 +760,7 @@ class Http2ServerResponse extends Stream { } if (chunk !== null && chunk !== undefined) - chunk = ''; - - this.write(chunk, encoding); + this.write(chunk, encoding); state.headRequest = stream.headRequest; state.ending = true; diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index c486676f72a2bd..7bc500f203443e 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -394,9 +394,11 @@ function onSessionHeaders(handle, id, cat, flags, headers, sensitiveHeaders) { } } else if (cat === NGHTTP2_HCAT_PUSH_RESPONSE) { event = 'push'; - // cat === NGHTTP2_HCAT_HEADERS: } else if (!endOfStream && status !== undefined && status >= 200) { event = 'response'; + } else if (endOfStream && status !== undefined && status >= 200) { + // cat === NGHTTP2_HCAT_HEADERS + event = 'response'; } else { event = endOfStream ? 'trailers' : 'headers'; } From 91db9b4d8b22fc9e0aaa4753f4f28e9f7bf3011b Mon Sep 17 00:00:00 2001 From: ofir Date: Sun, 30 Jan 2022 15:22:04 +0200 Subject: [PATCH 4/4] simplify condition --- lib/internal/http2/core.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 7bc500f203443e..0960bcc5e63e40 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -394,10 +394,7 @@ function onSessionHeaders(handle, id, cat, flags, headers, sensitiveHeaders) { } } else if (cat === NGHTTP2_HCAT_PUSH_RESPONSE) { event = 'push'; - } else if (!endOfStream && status !== undefined && status >= 200) { - event = 'response'; - } else if (endOfStream && status !== undefined && status >= 200) { - // cat === NGHTTP2_HCAT_HEADERS + } else if (status !== undefined && status >= 200) { event = 'response'; } else { event = endOfStream ? 'trailers' : 'headers';