diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 15e7358627dd8d..2523fb0ba372ea 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 } @@ -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) { @@ -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) { @@ -482,21 +481,21 @@ 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); } 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 +512,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 +538,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 a956697b5cb5a7..14e78155204770 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 6e02fe0cffb2ab..7cdae697cc0b63 100644 --- a/test/parallel/test-http2-compat-serverrequest-settimeout.js +++ b/test/parallel/test-http2-compat-serverrequest-settimeout.js @@ -4,13 +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(); + req.setTimeout(msecs, common.mustNotCall()); + })); + res.on('finish', common.mustCall(() => { + req.setTimeout(msecs, common.mustNotCall()); + process.nextTick(() => { + assert.doesNotThrow( + () => req.setTimeout(msecs, common.mustNotCall()) + ); + server.close(); + }); })); }); @@ -24,7 +36,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 a6882c6a9bf1c4..8c72e3876527c2 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 8c5ff31274322e..ecc8fea74ce637 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 f2b3ae7cfefa49..20329e0d8fdfb4 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 92a0fab4e0a49d..957983b0aedb6d 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 2cc82d4dd3c82f..d753b3e6cc4230 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 66441d390ae938..6b5bf03d522045 100644 --- a/test/parallel/test-http2-compat-serverresponse-settimeout.js +++ b/test/parallel/test-http2-compat-serverresponse-settimeout.js @@ -4,13 +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.setTimeout(msecs, common.mustNotCall()); + })); + res.on('finish', common.mustCall(() => { + res.setTimeout(msecs, common.mustNotCall()); + process.nextTick(() => { + assert.doesNotThrow( + () => res.setTimeout(msecs, common.mustNotCall()) + ); + server.close(); + }); })); }); @@ -24,7 +36,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 17c2d734425137..3f3aa2045b4929 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(() => {