From 59c76a67d80739e639df3f812f2eb8649464ebf9 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Tue, 12 Sep 2017 07:39:53 -0400 Subject: [PATCH] http2: cleanup of h2 compat layer, add tests Remove unnecessary variable assignments, remove unreachable code pathways, remove path getter and setter, and other very minor cleanup. Only remove reference to stream on nextTick so that users and libraries can access it in the finish event. Fixes: https://github.com/nodejs/node/issues/15313 Refs: https://github.com/expressjs/express/pull/3390 PR-URL: https://github.com/nodejs/node/pull/15254 Reviewed-By: Matteo Collina Reviewed-By: James M Snell --- lib/internal/http2/compat.js | 238 ++++++++---------- ...test-http2-compat-serverrequest-headers.js | 7 +- .../test-http2-compat-serverrequest.js | 2 - ...est-http2-compat-serverresponse-destroy.js | 2 +- ...st-http2-compat-serverresponse-finished.js | 7 +- ...est-http2-compat-serverresponse-headers.js | 33 +++ ...t-http2-compat-serverresponse-writehead.js | 5 + test/parallel/test-http2-createwritereq.js | 2 +- 8 files changed, 158 insertions(+), 138 deletions(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index fa565e9bd67386..04f4bb3c5a338c 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -17,6 +17,21 @@ const kRawHeaders = Symbol('rawHeaders'); const kTrailers = Symbol('trailers'); const kRawTrailers = Symbol('rawTrailers'); +const { + NGHTTP2_NO_ERROR, + + HTTP2_HEADER_AUTHORITY, + HTTP2_HEADER_METHOD, + HTTP2_HEADER_PATH, + HTTP2_HEADER_SCHEME, + HTTP2_HEADER_STATUS, + + HTTP_STATUS_CONTINUE, + HTTP_STATUS_EXPECTATION_FAILED, + HTTP_STATUS_METHOD_NOT_ALLOWED, + HTTP_STATUS_OK +} = constants; + let statusMessageWarned = false; // Defines and implements an API compatibility layer on top of the core @@ -24,6 +39,8 @@ let statusMessageWarned = false; // close as possible to the current require('http') API function assertValidHeader(name, value) { + if (name === '') + throw new errors.TypeError('ERR_INVALID_HTTP_TOKEN', 'Header name', name); if (isPseudoHeader(name)) throw new errors.Error('ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED'); if (value === undefined || value === null) @@ -32,15 +49,11 @@ function assertValidHeader(name, value) { function isPseudoHeader(name) { switch (name) { - case ':status': - return true; - case ':method': - return true; - case ':path': - return true; - case ':authority': - return true; - case ':scheme': + case HTTP2_HEADER_STATUS: // :status + case HTTP2_HEADER_METHOD: // :method + case HTTP2_HEADER_PATH: // :path + case HTTP2_HEADER_AUTHORITY: // :authority + case HTTP2_HEADER_SCHEME: // :scheme return true; default: return false; @@ -58,8 +71,7 @@ function statusMessageWarn() { } function onStreamData(chunk) { - const request = this[kRequest]; - if (!request.push(chunk)) + if (!this[kRequest].push(chunk)) this.pause(); } @@ -71,8 +83,7 @@ function onStreamTrailers(trailers, flags, rawTrailers) { function onStreamEnd() { // Cause the request stream to end as well. - const request = this[kRequest]; - request.push(null); + this[kRequest].push(null); } function onStreamError(error) { @@ -86,13 +97,11 @@ function onStreamError(error) { } function onRequestPause() { - const stream = this[kStream]; - stream.pause(); + this[kStream].pause(); } function onRequestResume() { - const stream = this[kStream]; - stream.resume(); + this[kStream].resume(); } function onRequestDrain() { @@ -101,13 +110,11 @@ function onRequestDrain() { } function onStreamResponseDrain() { - const response = this[kResponse]; - response.emit('drain'); + this[kResponse].emit('drain'); } function onStreamClosedRequest() { - const req = this[kRequest]; - req.push(null); + this[kRequest].push(null); } function onStreamClosedResponse() { @@ -127,9 +134,8 @@ class Http2ServerRequest extends Readable { constructor(stream, headers, options, rawHeaders) { super(options); this[kState] = { - statusCode: null, closed: false, - closedCode: constants.NGHTTP2_NO_ERROR + closedCode: NGHTTP2_NO_ERROR }; this[kHeaders] = headers; this[kRawHeaders] = rawHeaders; @@ -155,23 +161,17 @@ class Http2ServerRequest extends Readable { } get closed() { - const state = this[kState]; - return Boolean(state.closed); + return this[kState].closed; } get code() { - const state = this[kState]; - return Number(state.closedCode); + return this[kState].closedCode; } get stream() { return this[kStream]; } - get statusCode() { - return this[kState].statusCode; - } - get headers() { return this[kHeaders]; } @@ -201,7 +201,10 @@ class Http2ServerRequest extends Readable { } get socket() { - return this.stream.session.socket; + const stream = this[kStream]; + if (stream === undefined) + return; + return stream.session.socket; } get connection() { @@ -210,7 +213,7 @@ class Http2ServerRequest extends Readable { _read(nread) { const stream = this[kStream]; - if (stream) { + if (stream !== undefined) { stream.resume(); } else { this.emit('error', new errors.Error('ERR_HTTP2_STREAM_CLOSED')); @@ -218,46 +221,23 @@ class Http2ServerRequest extends Readable { } get method() { - const headers = this[kHeaders]; - if (headers === undefined) - return; - return headers[constants.HTTP2_HEADER_METHOD]; + return this[kHeaders][HTTP2_HEADER_METHOD]; } get authority() { - const headers = this[kHeaders]; - if (headers === undefined) - return; - return headers[constants.HTTP2_HEADER_AUTHORITY]; + return this[kHeaders][HTTP2_HEADER_AUTHORITY]; } get scheme() { - const headers = this[kHeaders]; - if (headers === undefined) - return; - return headers[constants.HTTP2_HEADER_SCHEME]; + return this[kHeaders][HTTP2_HEADER_SCHEME]; } get url() { - return this.path; + return this[kHeaders][HTTP2_HEADER_PATH]; } set url(url) { - this.path = url; - } - - get path() { - const headers = this[kHeaders]; - if (headers === undefined) - return; - return headers[constants.HTTP2_HEADER_PATH]; - } - - set path(path) { - let headers = this[kHeaders]; - if (headers === undefined) - headers = this[kHeaders] = Object.create(null); - headers[constants.HTTP2_HEADER_PATH] = path; + this[kHeaders][HTTP2_HEADER_PATH] = url; } setTimeout(msecs, callback) { @@ -271,10 +251,10 @@ class Http2ServerRequest extends Readable { if (state.closed) return; if (code !== undefined) - state.closedCode = code; + state.closedCode = Number(code); state.closed = true; this.push(null); - this[kStream] = undefined; + process.nextTick(() => (this[kStream] = undefined)); } } @@ -283,12 +263,12 @@ class Http2ServerResponse extends Stream { super(options); this[kState] = { sendDate: true, - statusCode: constants.HTTP_STATUS_OK, - headerCount: 0, - trailerCount: 0, + statusCode: HTTP_STATUS_OK, closed: false, - closedCode: constants.NGHTTP2_NO_ERROR + closedCode: NGHTTP2_NO_ERROR }; + this[kHeaders] = Object.create(null); + this[kTrailers] = Object.create(null); this[kStream] = stream; stream[kResponse] = this; this.writable = true; @@ -305,13 +285,11 @@ class Http2ServerResponse extends Stream { } get closed() { - const state = this[kState]; - return Boolean(state.closed); + return this[kState].closed; } get code() { - const state = this[kState]; - return Number(state.closedCode); + return this[kState].closedCode; } get stream() { @@ -324,7 +302,7 @@ class Http2ServerResponse extends Stream { } get sendDate() { - return Boolean(this[kState].sendDate); + return this[kState].sendDate; } set sendDate(bool) { @@ -336,70 +314,71 @@ class Http2ServerResponse extends Stream { } set statusCode(code) { - const state = this[kState]; code |= 0; if (code >= 100 && code < 200) throw new errors.RangeError('ERR_HTTP2_INFO_STATUS_NOT_ALLOWED'); - if (code < 200 || code > 599) + if (code < 100 || code > 599) throw new errors.RangeError('ERR_HTTP2_STATUS_INVALID', code); - state.statusCode = code; + this[kState].statusCode = code; + } + + setTrailer(name, value) { + if (typeof name !== 'string') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'name', 'string'); + + name = name.trim().toLowerCase(); + assertValidHeader(name, value); + this[kTrailers][name] = String(value); } addTrailers(headers) { - let trailers = this[kTrailers]; const keys = Object.keys(headers); - if (keys.length === 0) - return; - if (trailers === undefined) - trailers = this[kTrailers] = Object.create(null); + let key = ''; for (var i = 0; i < keys.length; i++) { - trailers[keys[i]] = String(headers[keys[i]]); + key = keys[i]; + this.setTrailer(key, headers[key]); } } getHeader(name) { - const headers = this[kHeaders]; - if (headers === undefined) - return; - name = String(name).trim().toLowerCase(); - return headers[name]; + if (typeof name !== 'string') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'name', 'string'); + + name = name.trim().toLowerCase(); + return this[kHeaders][name]; } getHeaderNames() { - const headers = this[kHeaders]; - if (headers === undefined) - return []; - return Object.keys(headers); + return Object.keys(this[kHeaders]); } getHeaders() { - const headers = this[kHeaders]; - return Object.assign({}, headers); + return Object.assign({}, this[kHeaders]); } hasHeader(name) { - const headers = this[kHeaders]; - if (headers === undefined) - return false; - name = String(name).trim().toLowerCase(); - return Object.prototype.hasOwnProperty.call(headers, name); + if (typeof name !== 'string') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'name', 'string'); + + name = name.trim().toLowerCase(); + return Object.prototype.hasOwnProperty.call(this[kHeaders], name); } removeHeader(name) { - const headers = this[kHeaders]; - if (headers === undefined) - return; - name = String(name).trim().toLowerCase(); - delete headers[name]; + if (typeof name !== 'string') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'name', 'string'); + + name = name.trim().toLowerCase(); + delete this[kHeaders][name]; } setHeader(name, value) { - name = String(name).trim().toLowerCase(); + if (typeof name !== 'string') + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'name', 'string'); + + name = name.trim().toLowerCase(); assertValidHeader(name, value); - let headers = this[kHeaders]; - if (headers === undefined) - headers = this[kHeaders] = Object.create(null); - headers[name] = String(value); + this[kHeaders][name] = String(value); } get statusMessage() { @@ -413,7 +392,8 @@ class Http2ServerResponse extends Stream { } flushHeaders() { - if (this[kStream].headersSent === false) + const stream = this[kStream]; + if (stream !== undefined && stream.headersSent === false) this[kBeginSend](); } @@ -427,11 +407,14 @@ class Http2ServerResponse extends Stream { } const stream = this[kStream]; + if (stream === undefined) { + throw new errors.Error('ERR_HTTP2_STREAM_CLOSED'); + } if (stream.headersSent === true) { throw new errors.Error('ERR_HTTP2_INFO_HEADERS_AFTER_RESPOND'); } - if (headers) { + if (typeof headers === 'object') { const keys = Object.keys(headers); let key = ''; for (var i = 0; i < keys.length; i++) { @@ -479,15 +462,16 @@ class Http2ServerResponse extends Stream { this.write(chunk, encoding); } - if (typeof cb === 'function' && stream !== undefined) { + if (stream === undefined) { + return; + } + + if (typeof cb === 'function') { stream.once('finish', cb); } this[kBeginSend]({ endStream: true }); - - if (stream !== undefined) { - stream.end(); - } + stream.end(); } destroy(err) { @@ -519,19 +503,19 @@ class Http2ServerResponse extends Stream { [kBeginSend](options) { const stream = this[kStream]; - options = options || Object.create(null); - if (stream !== undefined && stream.headersSent === false) { + if (stream !== undefined && + stream.destroyed === false && + stream.headersSent === false) { + options = options || Object.create(null); const state = this[kState]; - const headers = this[kHeaders] || Object.create(null); - headers[constants.HTTP2_HEADER_STATUS] = state.statusCode; + const headers = this[kHeaders]; + headers[HTTP2_HEADER_STATUS] = state.statusCode; if (stream.finished === true) options.endStream = true; options.getTrailers = (trailers) => { Object.assign(trailers, this[kTrailers]); }; - if (stream.destroyed === false) { - stream.respond(headers, options); - } + stream.respond(headers, options); } } @@ -540,11 +524,11 @@ class Http2ServerResponse extends Stream { if (state.closed) return; if (code !== undefined) - state.closedCode = code; + state.closedCode = Number(code); state.closed = true; state.headersSent = this[kStream].headersSent; this.end(); - this[kStream] = undefined; + process.nextTick(() => (this[kStream] = undefined)); this.emit('finish'); } @@ -553,7 +537,7 @@ class Http2ServerResponse extends Stream { const stream = this[kStream]; if (stream === undefined) return false; this[kStream].additionalHeaders({ - [constants.HTTP2_HEADER_STATUS]: constants.HTTP_STATUS_CONTINUE + [HTTP2_HEADER_STATUS]: HTTP_STATUS_CONTINUE }); return true; } @@ -566,10 +550,10 @@ function onServerStream(stream, headers, flags, rawHeaders) { const response = new Http2ServerResponse(stream); // Check for the CONNECT method - const method = headers[constants.HTTP2_HEADER_METHOD]; + const method = headers[HTTP2_HEADER_METHOD]; if (method === 'CONNECT') { if (!server.emit('connect', request, response)) { - response.statusCode = constants.HTTP_STATUS_METHOD_NOT_ALLOWED; + response.statusCode = HTTP_STATUS_METHOD_NOT_ALLOWED; response.end(); } return; @@ -587,7 +571,7 @@ function onServerStream(stream, headers, flags, rawHeaders) { } else if (server.listenerCount('checkExpectation')) { server.emit('checkExpectation', request, response); } else { - response.statusCode = constants.HTTP_STATUS_EXPECTATION_FAILED; + response.statusCode = HTTP_STATUS_EXPECTATION_FAILED; response.end(); } return; diff --git a/test/parallel/test-http2-compat-serverrequest-headers.js b/test/parallel/test-http2-compat-serverrequest-headers.js index d5a66391058708..286a0e19c87c11 100644 --- a/test/parallel/test-http2-compat-serverrequest-headers.js +++ b/test/parallel/test-http2-compat-serverrequest-headers.js @@ -21,9 +21,9 @@ server.listen(0, common.mustCall(function() { 'foo-bar': 'abc123' }; + assert.strictEqual(request.path, undefined); assert.strictEqual(request.method, expected[':method']); assert.strictEqual(request.scheme, expected[':scheme']); - assert.strictEqual(request.path, expected[':path']); assert.strictEqual(request.url, expected[':path']); assert.strictEqual(request.authority, expected[':authority']); @@ -41,11 +41,6 @@ server.listen(0, common.mustCall(function() { request.url = '/one'; assert.strictEqual(request.url, '/one'); - assert.strictEqual(request.path, '/one'); - - request.path = '/two'; - assert.strictEqual(request.url, '/two'); - assert.strictEqual(request.path, '/two'); response.on('finish', common.mustCall(function() { server.close(); diff --git a/test/parallel/test-http2-compat-serverrequest.js b/test/parallel/test-http2-compat-serverrequest.js index ebc91b7a9edabd..a6882c6a9bf1c4 100644 --- a/test/parallel/test-http2-compat-serverrequest.js +++ b/test/parallel/test-http2-compat-serverrequest.js @@ -15,7 +15,6 @@ server.listen(0, common.mustCall(function() { const port = server.address().port; server.once('request', common.mustCall(function(request, response) { const expected = { - statusCode: null, version: '2.0', httpVersionMajor: 2, httpVersionMinor: 0 @@ -24,7 +23,6 @@ server.listen(0, common.mustCall(function() { assert.strictEqual(request.closed, false); assert.strictEqual(request.code, h2.constants.NGHTTP2_NO_ERROR); - assert.strictEqual(request.statusCode, expected.statusCode); assert.strictEqual(request.httpVersion, expected.version); assert.strictEqual(request.httpVersionMajor, expected.httpVersionMajor); assert.strictEqual(request.httpVersionMinor, expected.httpVersionMinor); diff --git a/test/parallel/test-http2-compat-serverresponse-destroy.js b/test/parallel/test-http2-compat-serverresponse-destroy.js index 40c73b0887c32f..f2b3ae7cfefa49 100644 --- a/test/parallel/test-http2-compat-serverresponse-destroy.js +++ b/test/parallel/test-http2-compat-serverresponse-destroy.js @@ -26,7 +26,7 @@ const server = http2.createServer(common.mustCall((req, res) => { assert.strictEqual(res.closed, true); })); - if (req.path !== '/') { + if (req.url !== '/') { nextError = errors.shift(); } res.destroy(nextError); diff --git a/test/parallel/test-http2-compat-serverresponse-finished.js b/test/parallel/test-http2-compat-serverresponse-finished.js index c80bdc33f8615a..e54255f67cd2ee 100644 --- a/test/parallel/test-http2-compat-serverresponse-finished.js +++ b/test/parallel/test-http2-compat-serverresponse-finished.js @@ -8,13 +8,18 @@ const assert = require('assert'); const h2 = require('http2'); // Http2ServerResponse.finished - const server = h2.createServer(); server.listen(0, common.mustCall(function() { const port = server.address().port; server.once('request', common.mustCall(function(request, response) { response.on('finish', common.mustCall(function() { + assert.ok(request.stream !== undefined); + assert.ok(response.stream !== undefined); server.close(); + process.nextTick(common.mustCall(() => { + assert.strictEqual(request.stream, undefined); + assert.strictEqual(response.stream, undefined); + })); })); assert.strictEqual(response.finished, false); response.end(); diff --git a/test/parallel/test-http2-compat-serverresponse-headers.js b/test/parallel/test-http2-compat-serverresponse-headers.js index b2285f9d2aabfc..2cc82d4dd3c82f 100644 --- a/test/parallel/test-http2-compat-serverresponse-headers.js +++ b/test/parallel/test-http2-compat-serverresponse-headers.js @@ -42,6 +42,31 @@ server.listen(0, common.mustCall(function() { response.removeHeader(denormalised); assert.strictEqual(response.hasHeader(denormalised), false); + common.expectsError( + () => response.hasHeader(), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "name" argument must be of type string' + } + ); + common.expectsError( + () => response.getHeader(), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "name" argument must be of type string' + } + ); + common.expectsError( + () => response.removeHeader(), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "name" argument must be of type string' + } + ); + [ ':status', ':method', @@ -70,6 +95,14 @@ server.listen(0, common.mustCall(function() { type: TypeError, message: 'Value must not be undefined or null' })); + common.expectsError( + () => response.setHeader(), // header name undefined + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "name" argument must be of type string' + } + ); response.setHeader(real, expectedValue); const expectedHeaderNames = [real]; diff --git a/test/parallel/test-http2-compat-serverresponse-writehead.js b/test/parallel/test-http2-compat-serverresponse-writehead.js index 04d7499083524f..12de2983468781 100644 --- a/test/parallel/test-http2-compat-serverresponse-writehead.js +++ b/test/parallel/test-http2-compat-serverresponse-writehead.js @@ -22,6 +22,11 @@ server.listen(0, common.mustCall(function() { response.on('finish', common.mustCall(function() { server.close(); + process.nextTick(common.mustCall(() => { + common.expectsError(() => { response.writeHead(300); }, { + code: 'ERR_HTTP2_STREAM_CLOSED' + }); + })); })); response.end(); })); diff --git a/test/parallel/test-http2-createwritereq.js b/test/parallel/test-http2-createwritereq.js index 7e4bd5cf112ee9..f4151d94e6aa10 100644 --- a/test/parallel/test-http2-createwritereq.js +++ b/test/parallel/test-http2-createwritereq.js @@ -30,7 +30,7 @@ const testsToRun = Object.keys(encodings).length; let testsFinished = 0; const server = http2.createServer(common.mustCall((req, res) => { - const testEncoding = encodings[req.path.slice(1)]; + const testEncoding = encodings[req.url.slice(1)]; req.on('data', common.mustCall((chunk) => assert.ok( Buffer.from(testString, testEncoding).equals(chunk)