From 3ddd012bacbe49fbd1376a45fcc66c6ade0f9bb4 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Tue, 19 Sep 2017 11:22:35 -0400 Subject: [PATCH 1/2] http2: small fixes to compatibility layer Expand argument validation through compat API, adjust behaviour of response.end to not throw if stream already closed to match http1, adjust behaviour of writeContinue to not throw if stream already closed and other very small tweaks. Add tests for added and fixed behaviour. Add tests for edge case behaviours of setTimeout, createPushResponse, destroy, end and trailers. --- lib/internal/http2/compat.js | 22 ++--- ...test-http2-compat-expect-continue-check.js | 5 ++ ...t-http2-compat-serverrequest-settimeout.js | 14 +++- .../test-http2-compat-serverrequest.js | 5 +- ...ompat-serverresponse-createpushresponse.js | 13 +++ ...est-http2-compat-serverresponse-destroy.js | 3 + .../test-http2-compat-serverresponse-end.js | 80 +++++++++++++++++-- ...est-http2-compat-serverresponse-headers.js | 15 +++- ...-http2-compat-serverresponse-settimeout.js | 14 +++- ...st-http2-compat-serverresponse-trailers.js | 44 ++++++++++ 10 files changed, 191 insertions(+), 24 deletions(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 15e7358627dd..096416414831 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -92,7 +92,7 @@ function onStreamError(error) { // errors in compatibility mode are // not forwarded to the request // and response objects. However, - // they are forwarded to 'clientError' + // they are forwarded to 'streamError' // on the server by Http2Stream } @@ -445,7 +445,7 @@ class Http2ServerResponse extends Stream { if (stream === undefined) { const err = new errors.Error('ERR_HTTP2_STREAM_CLOSED'); - if (cb) + if (typeof cb === 'function') process.nextTick(cb, err); else throw err; @@ -461,12 +461,11 @@ class Http2ServerResponse extends Stream { if (typeof chunk === 'function') { cb = chunk; chunk = null; - encoding = 'utf8'; } else if (typeof encoding === 'function') { cb = encoding; encoding = 'utf8'; } - if (stream === undefined || stream.finished === true) { + if (this.finished === true) { return false; } if (chunk !== null && chunk !== undefined) { @@ -497,6 +496,8 @@ class Http2ServerResponse extends Stream { } createPushResponse(headers, callback) { + if (typeof callback !== 'function') + throw new errors.TypeError('ERR_INVALID_CALLBACK'); const stream = this[kStream]; if (stream === undefined) { process.nextTick(callback, new errors.Error('ERR_HTTP2_STREAM_CLOSED')); @@ -513,12 +514,9 @@ class Http2ServerResponse extends Stream { if (stream !== undefined && stream.destroyed === false && stream.headersSent === false) { - options = options || Object.create(null); - const state = this[kState]; const headers = this[kHeaders]; - headers[HTTP2_HEADER_STATUS] = state.statusCode; - if (stream.finished === true) - options.endStream = true; + headers[HTTP2_HEADER_STATUS] = this[kState].statusCode; + options = options || Object.create(null); options.getTrailers = (trailers) => { Object.assign(trailers, this[kTrailers]); }; @@ -542,7 +540,11 @@ class Http2ServerResponse extends Stream { // TODO doesn't support callbacks writeContinue() { const stream = this[kStream]; - if (stream === undefined) return false; + if (stream === undefined || + stream.headersSent === true || + stream.destroyed === true) { + return false; + } this[kStream].additionalHeaders({ [HTTP2_HEADER_STATUS]: HTTP_STATUS_CONTINUE }); diff --git a/test/parallel/test-http2-compat-expect-continue-check.js b/test/parallel/test-http2-compat-expect-continue-check.js index a956697b5cb5..14e781552047 100644 --- a/test/parallel/test-http2-compat-expect-continue-check.js +++ b/test/parallel/test-http2-compat-expect-continue-check.js @@ -21,6 +21,11 @@ function handler(req, res) { 'abcd': '1' }); res.end(testResBody); + // should simply return false if already too late to write + assert.strictEqual(res.writeContinue(), false); + res.on('finish', common.mustCall( + () => process.nextTick(() => assert.strictEqual(res.writeContinue(), false)) + )); } const server = http2.createServer( diff --git a/test/parallel/test-http2-compat-serverrequest-settimeout.js b/test/parallel/test-http2-compat-serverrequest-settimeout.js index 6e02fe0cffb2..a0ca44dfd16a 100644 --- a/test/parallel/test-http2-compat-serverrequest-settimeout.js +++ b/test/parallel/test-http2-compat-serverrequest-settimeout.js @@ -4,14 +4,25 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); +const assert = require('assert'); const http2 = require('http2'); +const msecs = common.platformTimeout(1); const server = http2.createServer(); server.on('request', (req, res) => { - req.setTimeout(common.platformTimeout(1), common.mustCall(() => { + req.setTimeout(msecs, common.mustCall(() => { res.end(); })); + res.on('finish', common.mustCall(() => { + req.setTimeout(msecs, common.mustNotCall()); + process.nextTick(() => { + assert.doesNotThrow( + () => req.setTimeout(msecs, common.mustNotCall()) + ); + server.close(); + }); + })); }); server.listen(0, common.mustCall(() => { @@ -24,7 +35,6 @@ server.listen(0, common.mustCall(() => { ':authority': `localhost:${port}` }); req.on('end', common.mustCall(() => { - server.close(); client.destroy(); })); req.resume(); diff --git a/test/parallel/test-http2-compat-serverrequest.js b/test/parallel/test-http2-compat-serverrequest.js index a6882c6a9bf1..8c72e3876527 100644 --- a/test/parallel/test-http2-compat-serverrequest.js +++ b/test/parallel/test-http2-compat-serverrequest.js @@ -34,7 +34,10 @@ server.listen(0, common.mustCall(function() { response.on('finish', common.mustCall(function() { assert.strictEqual(request.closed, true); assert.strictEqual(request.code, h2.constants.NGHTTP2_NO_ERROR); - server.close(); + process.nextTick(() => { + assert.strictEqual(request.socket, undefined); + server.close(); + }); })); response.end(); })); diff --git a/test/parallel/test-http2-compat-serverresponse-createpushresponse.js b/test/parallel/test-http2-compat-serverresponse-createpushresponse.js index 8c5ff3127432..ecc8fea74ce6 100644 --- a/test/parallel/test-http2-compat-serverresponse-createpushresponse.js +++ b/test/parallel/test-http2-compat-serverresponse-createpushresponse.js @@ -16,6 +16,19 @@ const server = h2.createServer((request, response) => { assert.strictEqual(response.stream.id % 2, 1); response.write(servExpect); + // callback must be specified (and be a function) + common.expectsError( + () => response.createPushResponse({ + ':path': '/pushed', + ':method': 'GET' + }, undefined), + { + code: 'ERR_INVALID_CALLBACK', + type: TypeError, + message: 'Callback must be a function' + } + ); + response.createPushResponse({ ':path': '/pushed', ':method': 'GET' diff --git a/test/parallel/test-http2-compat-serverresponse-destroy.js b/test/parallel/test-http2-compat-serverresponse-destroy.js index f2b3ae7cfefa..20329e0d8fdf 100644 --- a/test/parallel/test-http2-compat-serverresponse-destroy.js +++ b/test/parallel/test-http2-compat-serverresponse-destroy.js @@ -24,6 +24,9 @@ const server = http2.createServer(common.mustCall((req, res) => { res.on('finish', common.mustCall(() => { assert.doesNotThrow(() => res.destroy(nextError)); assert.strictEqual(res.closed, true); + process.nextTick(() => { + assert.doesNotThrow(() => res.destroy(nextError)); + }); })); if (req.url !== '/') { diff --git a/test/parallel/test-http2-compat-serverresponse-end.js b/test/parallel/test-http2-compat-serverresponse-end.js index 92a0fab4e0a4..957983b0aedb 100644 --- a/test/parallel/test-http2-compat-serverresponse-end.js +++ b/test/parallel/test-http2-compat-serverresponse-end.js @@ -4,7 +4,7 @@ const { mustCall, mustNotCall, hasCrypto, skip } = require('../common'); if (!hasCrypto) skip('missing crypto'); -const { doesNotThrow, strictEqual } = require('assert'); +const { strictEqual } = require('assert'); const { createServer, connect, @@ -15,18 +15,82 @@ const { } = require('http2'); { - // Http2ServerResponse.end callback is called only the first time, - // but may be invoked repeatedly without throwing errors. + // Http2ServerResponse.end accepts chunk, encoding, cb as args + // It may be invoked repeatedly without throwing errors + // but callback will only be called once const server = createServer(mustCall((request, response) => { strictEqual(response.closed, false); - response.on('finish', mustCall(() => process.nextTick( - mustCall(() => doesNotThrow(() => response.end('test', mustNotCall()))) - ))); + response.end('end', 'utf8', mustCall(() => { + strictEqual(response.closed, true); + response.end(mustNotCall()); + process.nextTick(() => { + response.end(mustNotCall()); + server.close(); + }); + })); + response.end(mustNotCall()); + })); + server.listen(0, mustCall(() => { + let data = ''; + const { port } = server.address(); + const url = `http://localhost:${port}`; + const client = connect(url, mustCall(() => { + const headers = { + ':path': '/', + ':method': 'GET', + ':scheme': 'http', + ':authority': `localhost:${port}` + }; + const request = client.request(headers); + request.setEncoding('utf8'); + request.on('data', (chunk) => (data += chunk)); + request.on('end', mustCall(() => { + strictEqual(data, 'end'); + client.destroy(); + })); + request.end(); + request.resume(); + })); + })); +} + +{ + // Http2ServerResponse.end can omit encoding arg, sets it to utf-8 + const server = createServer(mustCall((request, response) => { + response.end('test\uD83D\uDE00', mustCall(() => { + server.close(); + })); + })); + server.listen(0, mustCall(() => { + let data = ''; + const { port } = server.address(); + const url = `http://localhost:${port}`; + const client = connect(url, mustCall(() => { + const headers = { + ':path': '/', + ':method': 'GET', + ':scheme': 'http', + ':authority': `localhost:${port}` + }; + const request = client.request(headers); + request.setEncoding('utf8'); + request.on('data', (chunk) => (data += chunk)); + request.on('end', mustCall(() => { + strictEqual(data, 'test\uD83D\uDE00'); + client.destroy(); + })); + request.end(); + request.resume(); + })); + })); +} + +{ + // Http2ServerResponse.end can omit chunk & encoding args + const server = createServer(mustCall((request, response) => { response.end(mustCall(() => { server.close(); })); - response.end(mustNotCall()); - strictEqual(response.closed, true); })); server.listen(0, mustCall(() => { const { port } = server.address(); diff --git a/test/parallel/test-http2-compat-serverresponse-headers.js b/test/parallel/test-http2-compat-serverresponse-headers.js index 2cc82d4dd3c8..d753b3e6cc42 100644 --- a/test/parallel/test-http2-compat-serverresponse-headers.js +++ b/test/parallel/test-http2-compat-serverresponse-headers.js @@ -103,6 +103,14 @@ server.listen(0, common.mustCall(function() { message: 'The "name" argument must be of type string' } ); + common.expectsError( + () => response.setHeader(''), + { + code: 'ERR_INVALID_HTTP_TOKEN', + type: TypeError, + message: 'Header name must be a valid HTTP token [""]' + } + ); response.setHeader(real, expectedValue); const expectedHeaderNames = [real]; @@ -122,7 +130,12 @@ server.listen(0, common.mustCall(function() { response.on('finish', common.mustCall(function() { assert.strictEqual(response.code, h2.constants.NGHTTP2_NO_ERROR); assert.strictEqual(response.headersSent, true); - server.close(); + process.nextTick(() => { + // can access headersSent after stream is undefined + assert.strictEqual(response.stream, undefined); + assert.strictEqual(response.headersSent, true); + server.close(); + }); })); response.end(); })); diff --git a/test/parallel/test-http2-compat-serverresponse-settimeout.js b/test/parallel/test-http2-compat-serverresponse-settimeout.js index 66441d390ae9..6e76bbc47a1f 100644 --- a/test/parallel/test-http2-compat-serverresponse-settimeout.js +++ b/test/parallel/test-http2-compat-serverresponse-settimeout.js @@ -4,14 +4,25 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); +const assert = require('assert'); const http2 = require('http2'); +const msecs = common.platformTimeout(1); const server = http2.createServer(); server.on('request', (req, res) => { - res.setTimeout(common.platformTimeout(1), common.mustCall(() => { + res.setTimeout(msecs, common.mustCall(() => { res.end(); })); + res.on('finish', common.mustCall(() => { + res.setTimeout(msecs, common.mustNotCall()); + process.nextTick(() => { + assert.doesNotThrow( + () => res.setTimeout(msecs, common.mustNotCall()) + ); + server.close(); + }); + })); }); server.listen(0, common.mustCall(() => { @@ -24,7 +35,6 @@ server.listen(0, common.mustCall(() => { ':authority': `localhost:${port}` }); req.on('end', common.mustCall(() => { - server.close(); client.destroy(); })); req.resume(); diff --git a/test/parallel/test-http2-compat-serverresponse-trailers.js b/test/parallel/test-http2-compat-serverresponse-trailers.js index 17c2d7344251..3f3aa2045b49 100644 --- a/test/parallel/test-http2-compat-serverresponse-trailers.js +++ b/test/parallel/test-http2-compat-serverresponse-trailers.js @@ -14,6 +14,49 @@ server.listen(0, common.mustCall(() => { response.addTrailers({ ABC: 123 }); + response.setTrailer('ABCD', 123); + + common.expectsError( + () => response.addTrailers({ '': 'test' }), + { + code: 'ERR_INVALID_HTTP_TOKEN', + type: TypeError, + message: 'Header name must be a valid HTTP token [""]' + } + ); + common.expectsError( + () => response.setTrailer('test', undefined), + { + code: 'ERR_HTTP2_INVALID_HEADER_VALUE', + type: TypeError, + message: 'Value must not be undefined or null' + } + ); + common.expectsError( + () => response.setTrailer('test', null), + { + code: 'ERR_HTTP2_INVALID_HEADER_VALUE', + type: TypeError, + message: 'Value must not be undefined or null' + } + ); + common.expectsError( + () => response.setTrailer(), // trailer name undefined + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "name" argument must be of type string' + } + ); + common.expectsError( + () => response.setTrailer(''), + { + code: 'ERR_INVALID_HTTP_TOKEN', + type: TypeError, + message: 'Header name must be a valid HTTP token [""]' + } + ); + response.end('hello'); })); @@ -22,6 +65,7 @@ server.listen(0, common.mustCall(() => { const request = client.request(); request.on('trailers', common.mustCall((headers) => { assert.strictEqual(headers.abc, '123'); + assert.strictEqual(headers.abcd, '123'); })); request.resume(); request.on('end', common.mustCall(() => { From 506f9d1a5b1cd2c7b8f0834e8ed720ca09bf8383 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Wed, 20 Sep 2017 07:49:35 -0400 Subject: [PATCH 2/2] adjust settimeout behaviour after stream closed --- lib/internal/http2/compat.js | 16 +++++++--------- ...test-http2-compat-serverrequest-settimeout.js | 1 + ...est-http2-compat-serverresponse-settimeout.js | 1 + 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 096416414831..2523fb0ba372 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -248,9 +248,9 @@ class Http2ServerRequest extends Readable { } setTimeout(msecs, callback) { - const stream = this[kStream]; - if (stream === undefined) return; - stream.setTimeout(msecs, callback); + if (this[kState].closed) + return; + this[kStream].setTimeout(msecs, callback); } [kFinish](code) { @@ -481,17 +481,15 @@ class Http2ServerResponse extends Stream { } destroy(err) { - const stream = this[kStream]; - if (stream === undefined) { - // nothing to do, already closed + if (this[kState].closed) return; - } - stream.destroy(err); + this[kStream].destroy(err); } setTimeout(msecs, callback) { const stream = this[kStream]; - if (stream === undefined) return; + if (this[kState].closed) + return; stream.setTimeout(msecs, callback); } diff --git a/test/parallel/test-http2-compat-serverrequest-settimeout.js b/test/parallel/test-http2-compat-serverrequest-settimeout.js index a0ca44dfd16a..7cdae697cc0b 100644 --- a/test/parallel/test-http2-compat-serverrequest-settimeout.js +++ b/test/parallel/test-http2-compat-serverrequest-settimeout.js @@ -13,6 +13,7 @@ const server = http2.createServer(); server.on('request', (req, res) => { req.setTimeout(msecs, common.mustCall(() => { res.end(); + req.setTimeout(msecs, common.mustNotCall()); })); res.on('finish', common.mustCall(() => { req.setTimeout(msecs, common.mustNotCall()); diff --git a/test/parallel/test-http2-compat-serverresponse-settimeout.js b/test/parallel/test-http2-compat-serverresponse-settimeout.js index 6e76bbc47a1f..6b5bf03d5220 100644 --- a/test/parallel/test-http2-compat-serverresponse-settimeout.js +++ b/test/parallel/test-http2-compat-serverresponse-settimeout.js @@ -13,6 +13,7 @@ const server = http2.createServer(); server.on('request', (req, res) => { res.setTimeout(msecs, common.mustCall(() => { res.end(); + res.setTimeout(msecs, common.mustNotCall()); })); res.on('finish', common.mustCall(() => { res.setTimeout(msecs, common.mustNotCall());