From 5222ec209e20ec9af55471156e7787367f81b1bc Mon Sep 17 00:00:00 2001 From: RidgeA Date: Wed, 11 Jul 2018 17:29:42 +0300 Subject: [PATCH 1/2] http2: remove `waitTrailers` listener after closing a stream When `writeHear` of `Http2ServerResponse` instance are called with 204, 205 and 304 status codes an underlying stream closes. If call `end` method after sending any of these status codes it will cause an error `TypeError: Cannot read property 'Symbol(trailers)' of undefined` because a reference to `Http2ServerResponse` instance associated with Http2Stream already was deleted. The closing of stream causes emitting `watiTrailers` event and, when this event handles inside `onStreamTrailerReady` handler, there is no reference to Http2ServerResponse instance. Fises: https://github.com/nodejs/node/issues/21740 --- lib/internal/http2/compat.js | 2 + ...response-end-after-statuses-withou-body.js | 47 +++++++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 test/parallel/test-http2-compat-serverresponse-end-after-statuses-withou-body.js diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 88879484ebaab7..37429c7b8f6a5c 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -391,6 +391,8 @@ function onStreamCloseResponse() { state.closed = true; this[kProxySocket] = null; + + this.off('wantTrailers', onStreamTrailersReady); this[kResponse] = undefined; res.emit('finish'); diff --git a/test/parallel/test-http2-compat-serverresponse-end-after-statuses-withou-body.js b/test/parallel/test-http2-compat-serverresponse-end-after-statuses-withou-body.js new file mode 100644 index 00000000000000..c9991d6269bffa --- /dev/null +++ b/test/parallel/test-http2-compat-serverresponse-end-after-statuses-withou-body.js @@ -0,0 +1,47 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const h2 = require('http2'); + +// This test case ensures that calling of res.end after sending +// 204, 205 and 304 HTTP statuses will not cause an error +// See issue: https://github.com/nodejs/node/issues/21740 + +const { + HTTP_STATUS_NO_CONTENT, + HTTP_STATUS_RESET_CONTENT, + HTTP_STATUS_NOT_MODIFIED +} = h2.constants; + +const STATUS_WITHOUT_BODY = [ + HTTP_STATUS_NO_CONTENT, + HTTP_STATUS_RESET_CONTENT, + HTTP_STATUS_NOT_MODIFIED, +]; +const STATUS_CODES_COUNT = STATUS_WITHOUT_BODY.length; + +const server = h2.createServer(common.mustCall(function(req, res) { + res.writeHead(STATUS_WITHOUT_BODY.pop()); + res.end(); +}, STATUS_CODES_COUNT)); + +server.listen(0, common.mustCall(function() { + const url = `http://localhost:${server.address().port}`; + const client = h2.connect(url, common.mustCall(() => { + let responseCount = 0; + const closeAfterResponse = () => { + if (STATUS_CODES_COUNT === ++responseCount) { + client.destroy(); + server.close(); + } + }; + + for (let i = 0; i < STATUS_CODES_COUNT; i++) { + const request = client.request(); + request.on('response', common.mustCall(closeAfterResponse)); + } + + })); +})); From 74389f2fc62a739496e236f60c7044b8626f6850 Mon Sep 17 00:00:00 2001 From: RidgeA Date: Fri, 13 Jul 2018 09:30:01 +0300 Subject: [PATCH 2/2] Change to `off` -> `removeListener` and variable to lowercase --- lib/internal/http2/compat.js | 2 +- ...-compat-serverresponse-end-after-statuses-withou-body.js | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 37429c7b8f6a5c..4a006107b2fcea 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -392,7 +392,7 @@ function onStreamCloseResponse() { this[kProxySocket] = null; - this.off('wantTrailers', onStreamTrailersReady); + this.removeListener('wantTrailers', onStreamTrailersReady); this[kResponse] = undefined; res.emit('finish'); diff --git a/test/parallel/test-http2-compat-serverresponse-end-after-statuses-withou-body.js b/test/parallel/test-http2-compat-serverresponse-end-after-statuses-withou-body.js index c9991d6269bffa..83d5521bf2473f 100644 --- a/test/parallel/test-http2-compat-serverresponse-end-after-statuses-withou-body.js +++ b/test/parallel/test-http2-compat-serverresponse-end-after-statuses-withou-body.js @@ -15,15 +15,15 @@ const { HTTP_STATUS_NOT_MODIFIED } = h2.constants; -const STATUS_WITHOUT_BODY = [ +const statusWithouBody = [ HTTP_STATUS_NO_CONTENT, HTTP_STATUS_RESET_CONTENT, HTTP_STATUS_NOT_MODIFIED, ]; -const STATUS_CODES_COUNT = STATUS_WITHOUT_BODY.length; +const STATUS_CODES_COUNT = statusWithouBody.length; const server = h2.createServer(common.mustCall(function(req, res) { - res.writeHead(STATUS_WITHOUT_BODY.pop()); + res.writeHead(statusWithouBody.pop()); res.end(); }, STATUS_CODES_COUNT));