From 278f42dabf8abeb79b2eb38d090bead740959aeb Mon Sep 17 00:00:00 2001 From: Sebastiaan Deckers Date: Thu, 6 Jul 2017 09:25:53 +0800 Subject: [PATCH 1/7] http2: edge case errors, consistency, nits --- lib/internal/http2/compat.js | 29 ++++++++++--------- .../test-http2-compat-serverresponse-end.js | 16 +++++----- .../test-http2-respond-file-compat.js | 23 +++++++++++++++ test/parallel/test-http2-respond-file.js | 1 - 4 files changed, 46 insertions(+), 23 deletions(-) create mode 100644 test/parallel/test-http2-respond-file-compat.js diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 89381ce639..635b09749e 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -439,27 +439,25 @@ class Http2ServerResponse extends Stream { if (typeof chunk === 'function') { cb = chunk; - chunk = ''; + chunk = null; encoding = 'utf8'; } else if (typeof encoding === 'function') { cb = encoding; encoding = 'utf8'; } - - if (stream === undefined) { - var err = new errors.Error('ERR_HTTP2_STREAM_CLOSED'); - if (cb) - process.nextTick(cb, err); - else - throw err; - return; + if (chunk !== null && chunk !== undefined) { + this.write(chunk, encoding); } - if (chunk !== undefined) - this.write(chunk, encoding); + if (typeof cb === 'function' && stream !== undefined) { + stream.once('finish', cb); + } this[kBeginSend]({endStream: true}); - stream.end(); + + if (stream !== undefined) { + stream.end(); + } } destroy(err) { @@ -517,8 +515,13 @@ class Http2ServerResponse extends Stream { state.headersSent = true; const headers = this[kHeaders] || Object.create(null); headers[constants.HTTP2_HEADER_STATUS] = state.statusCode; - if (stream.destroyed === false) + var _writableState = stream._writableState; + if (stream.destroyed === false && + _writableState.ending === false && + _writableState.finished === false + ) { stream.respond(headers, options); + } } } diff --git a/test/parallel/test-http2-compat-serverresponse-end.js b/test/parallel/test-http2-compat-serverresponse-end.js index 0234d7b442..49960e69a1 100644 --- a/test/parallel/test-http2-compat-serverresponse-end.js +++ b/test/parallel/test-http2-compat-serverresponse-end.js @@ -7,16 +7,14 @@ const { createServer, connect } = require('http2'); // Http2ServerResponse.end { - const server = createServer(); - server.listen(0, mustCall(() => { - const port = server.address().port; - server.once('request', mustCall((request, response) => { - response.on('finish', mustCall(() => { - server.close(); - })); - response.end(); + const server = createServer(mustCall((request, response) => { + response.end(mustCall(() => { + server.close(); })); - + response.end(mustNotCall()); + })); + server.listen(0, mustCall(() => { + const {port} = server.address(); const url = `http://localhost:${port}`; const client = connect(url, mustCall(() => { const headers = { diff --git a/test/parallel/test-http2-respond-file-compat.js b/test/parallel/test-http2-respond-file-compat.js new file mode 100644 index 0000000000..be256ee2bf --- /dev/null +++ b/test/parallel/test-http2-respond-file-compat.js @@ -0,0 +1,23 @@ +// Flags: --expose-http2 +'use strict'; + +const common = require('../common'); +const http2 = require('http2'); +const path = require('path'); + +const fname = path.resolve(common.fixturesDir, 'elipses.txt'); + +const server = http2.createServer(common.mustCall((request, response) => { + response.stream.respondWithFile(fname); +})); +server.listen(0, () => { + const client = http2.connect(`http://localhost:${server.address().port}`); + const req = client.request(); + req.on('response', common.mustCall()); + req.on('end', common.mustCall(() => { + client.destroy(); + server.close(); + })); + req.end(); + req.resume(); +}); diff --git a/test/parallel/test-http2-respond-file.js b/test/parallel/test-http2-respond-file.js index d5b65a1caa..81babb58fa 100644 --- a/test/parallel/test-http2-respond-file.js +++ b/test/parallel/test-http2-respond-file.js @@ -34,7 +34,6 @@ server.listen(0, () => { const req = client.request(); req.on('response', common.mustCall((headers) => { - console.log(headers); assert.strictEqual(headers[HTTP2_HEADER_CONTENT_TYPE], 'text/plain'); assert.strictEqual(+headers[HTTP2_HEADER_CONTENT_LENGTH], data.length); assert.strictEqual(headers[HTTP2_HEADER_LAST_MODIFIED], From f3e65eb45414c2b781c5d4228564ae91e2fb7104 Mon Sep 17 00:00:00 2001 From: Sebastiaan Deckers Date: Sat, 8 Jul 2017 18:18:39 +0800 Subject: [PATCH 2/7] http2: ServerHttp2Stream.headersSent property, proxied by Http2ServerResponse This avoids brittle abstraction-leaking to guess whether or not ServerHttp2Stream.respond() may still be called on HEAD requests where the stream is ended before headers are sent. --- lib/internal/http2/compat.js | 28 ++++------- lib/internal/http2/core.js | 5 ++ .../test-http2-compat-serverresponse-end.js | 50 +++++++++++++++++-- 3 files changed, 63 insertions(+), 20 deletions(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 635b09749e..abc8e2ec40 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -262,7 +262,6 @@ class Http2ServerResponse extends Stream { this[kState] = { sendDate: true, statusCode: constants.HTTP_STATUS_OK, - headersSent: false, headerCount: 0, trailerCount: 0, closed: false, @@ -300,8 +299,8 @@ class Http2ServerResponse extends Stream { } get headersSent() { - var state = this[kState]; - return state.headersSent; + var stream = this[kStream]; + return stream.headersSent; } get sendDate() { @@ -388,7 +387,7 @@ class Http2ServerResponse extends Stream { } flushHeaders() { - if (this[kState].headersSent === false) + if (this[kStream].headersSent === false) this[kBeginSend](); } @@ -480,19 +479,17 @@ class Http2ServerResponse extends Stream { } sendInfo(code, headers) { - var state = this[kState]; - if (state.headersSent === true) { + var stream = this[kStream]; + if (stream.headersSent === true) { throw new errors.Error('ERR_HTTP2_INFO_HEADERS_AFTER_RESPOND'); } if (headers && typeof headers !== 'object') throw new errors.TypeError('ERR_HTTP2_HEADERS_OBJECT'); - var stream = this[kStream]; if (stream === undefined) return; code |= 0; if (code < 100 || code >= 200) throw new errors.RangeError('ERR_HTTP2_INVALID_INFO_STATUS', code); - state.headersSent = true; headers[constants.HTTP2_HEADER_STATUS] = code; stream.respond(headers); } @@ -509,17 +506,14 @@ class Http2ServerResponse extends Stream { } [kBeginSend](options) { - var state = this[kState]; - var stream = this[kStream]; - if (state.headersSent === false) { - state.headersSent = true; + const stream = this[kStream]; + if (stream !== undefined && stream.headersSent === false) { + const state = this[kState]; const headers = this[kHeaders] || Object.create(null); headers[constants.HTTP2_HEADER_STATUS] = state.statusCode; - var _writableState = stream._writableState; - if (stream.destroyed === false && - _writableState.ending === false && - _writableState.finished === false - ) { + if (stream.finished === true) + options.endStream = true; + if (stream.destroyed === false) { stream.respond(headers, options); } } diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 014ea1f850..bebb00059e 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1536,6 +1536,11 @@ class ServerHttp2Stream extends Http2Stream { debug(`[${sessionName(session[kType])}] created serverhttp2stream`); } + // true if the HEADERS frame has been sent + get headersSent() { + return this[kState].headersSent; + } + // true if the remote peer accepts push streams get pushAllowed() { return this[kSession].remoteSettings.enablePush; diff --git a/test/parallel/test-http2-compat-serverresponse-end.js b/test/parallel/test-http2-compat-serverresponse-end.js index 49960e69a1..a188ae5a58 100644 --- a/test/parallel/test-http2-compat-serverresponse-end.js +++ b/test/parallel/test-http2-compat-serverresponse-end.js @@ -1,12 +1,20 @@ // Flags: --expose-http2 'use strict'; +const { strictEqual } = require('assert'); const { mustCall, mustNotCall } = require('../common'); -const { createServer, connect } = require('http2'); - -// Http2ServerResponse.end +const { + createServer, + connect, + constants: { + HTTP2_HEADER_STATUS, + HTTP_STATUS_OK + } +} = require('http2'); { + // Http2ServerResponse.end callback is called only the first time, + // but may be invoked repeatedly without throwing errors. const server = createServer(mustCall((request, response) => { response.end(mustCall(() => { server.close(); @@ -31,3 +39,39 @@ const { createServer, connect } = require('http2'); })); })); } + +{ + // Http2ServerResponse.end is not necessary on HEAD requests since the stream + // is already closed. Headers, however, can still be sent to the client. + const server = createServer(mustCall((request, response) => { + strictEqual(response.finished, true); + response.writeHead(HTTP_STATUS_OK, {foo: 'bar'}); + response.flushHeaders(); + response.end(mustNotCall()); + })); + server.listen(0, mustCall(() => { + const {port} = server.address(); + const url = `http://localhost:${port}`; + const client = connect(url, mustCall(() => { + const headers = { + ':path': '/', + ':method': 'HEAD', + ':scheme': 'http', + ':authority': `localhost:${port}` + }; + const request = client.request(headers); + request.on('response', mustCall((headers, flags) => { + strictEqual(headers[HTTP2_HEADER_STATUS], HTTP_STATUS_OK); + strictEqual(flags, 5); // the end of stream flag is set + strictEqual(headers.foo, 'bar'); + })); + request.on('data', mustNotCall()); + request.on('end', mustCall(() => { + client.destroy(); + server.close(); + })); + request.end(); + request.resume(); + })); + })); +} From b181b8bfcebfbcf494e2ef20f372bc3831c9010f Mon Sep 17 00:00:00 2001 From: Sebastiaan Deckers Date: Sat, 8 Jul 2017 18:37:27 +0800 Subject: [PATCH 3/7] http2: docs for ServerHttp2Stream headersSent & pushAllowed (Squash) --- doc/api/http2.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/doc/api/http2.md b/doc/api/http2.md index f10bd26773..de9702e543 100755 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -921,6 +921,27 @@ added: REPLACEME Sends an additional informational `HEADERS` frame to the connected HTTP/2 peer. +#### http2stream.headersSent + + +* Value: {boolean} + +Boolean (read-only). True if headers were sent, false otherwise. + +#### http2stream.pushAllowed + + +* Value: {boolean} + +Read-only property mapped to the `SETTINGS_ENABLE_PUSH` flag of the remote +client's most recent `SETTINGS` frame. Will be `true` if the remote peer +accepts push streams, `false` otherwise. Settings are the same for every +`Http2Stream` in the same `Http2Session`. + #### http2stream.pushStream(headers[, options], callback) diff --git a/test/parallel/test-http2-connect.js b/test/parallel/test-http2-connect.js new file mode 100644 index 0000000000..305ea034c9 --- /dev/null +++ b/test/parallel/test-http2-connect.js @@ -0,0 +1,29 @@ +// Flags: --expose-http2 +'use strict'; + +const { mustCall } = require('../common'); +const { doesNotThrow } = require('assert'); +const { createServer, connect } = require('http2'); + +const server = createServer(); +server.listen(0, mustCall(() => { + const authority = `http://localhost:${server.address().port}`; + const options = {}; + const listener = () => mustCall(); + + const clients = new Set(); + doesNotThrow(() => clients.add(connect(authority))); + doesNotThrow(() => clients.add(connect(authority, options))); + doesNotThrow(() => clients.add(connect(authority, options, listener()))); + doesNotThrow(() => clients.add(connect(authority, listener()))); + + for (const client of clients) { + client.once('connect', mustCall((headers) => { + client.destroy(); + clients.delete(client); + if (clients.size === 0) { + server.close(); + } + })); + } +})); From 64376eb03538d9ce4b91c2a1ec0401620401d74a Mon Sep 17 00:00:00 2001 From: Sebastiaan Deckers Date: Tue, 11 Jul 2017 07:55:48 +0800 Subject: [PATCH 7/7] http2: eslint object-curly-spacing: ["error", "always"] (Squash) --- test/parallel/test-http2-compat-serverresponse-end.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-http2-compat-serverresponse-end.js b/test/parallel/test-http2-compat-serverresponse-end.js index a188ae5a58..1274f3d6b3 100644 --- a/test/parallel/test-http2-compat-serverresponse-end.js +++ b/test/parallel/test-http2-compat-serverresponse-end.js @@ -22,7 +22,7 @@ const { response.end(mustNotCall()); })); server.listen(0, mustCall(() => { - const {port} = server.address(); + const { port } = server.address(); const url = `http://localhost:${port}`; const client = connect(url, mustCall(() => { const headers = { @@ -45,12 +45,12 @@ const { // is already closed. Headers, however, can still be sent to the client. const server = createServer(mustCall((request, response) => { strictEqual(response.finished, true); - response.writeHead(HTTP_STATUS_OK, {foo: 'bar'}); + response.writeHead(HTTP_STATUS_OK, { foo: 'bar' }); response.flushHeaders(); response.end(mustNotCall()); })); server.listen(0, mustCall(() => { - const {port} = server.address(); + const { port } = server.address(); const url = `http://localhost:${port}`; const client = connect(url, mustCall(() => { const headers = {