From ba9012d4bcd751c38a5e3f1ef07673640c7eb856 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Wed, 13 Sep 2017 09:34:14 -0400 Subject: [PATCH] http2: add tests for push stream error handling Add tests that cover errors for wrong arguments, as well as tests for error codes from nghttp2. Fix pushStream to emit NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE on session rather than stream. PR-URL: https://github.com/nodejs/node/pull/15281 Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- lib/internal/http2/core.js | 2 +- ...st-http2-server-push-stream-errors-args.js | 57 ++++++++ .../test-http2-server-push-stream-errors.js | 130 ++++++++++++++++++ .../test-http2-server-push-stream-head.js | 54 ++++++++ .../parallel/test-http2-server-push-stream.js | 4 +- 5 files changed, 244 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-http2-server-push-stream-errors-args.js create mode 100644 test/parallel/test-http2-server-push-stream-errors.js create mode 100644 test/parallel/test-http2-server-push-stream-head.js diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 471fa5b1c5d5a9..0ea3ca75f4d43e 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1790,7 +1790,7 @@ class ServerHttp2Stream extends Http2Stream { break; case NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE: err = new errors.Error('ERR_HTTP2_OUT_OF_STREAMS'); - process.nextTick(() => this.emit('error', err)); + process.nextTick(() => session.emit('error', err)); break; case NGHTTP2_ERR_STREAM_CLOSED: err = new errors.Error('ERR_HTTP2_STREAM_CLOSED'); diff --git a/test/parallel/test-http2-server-push-stream-errors-args.js b/test/parallel/test-http2-server-push-stream-errors-args.js new file mode 100644 index 00000000000000..9924517d406c58 --- /dev/null +++ b/test/parallel/test-http2-server-push-stream-errors-args.js @@ -0,0 +1,57 @@ +// Flags: --expose-http2 +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const http2 = require('http2'); + +// Check that pushStream handles being passed wrong arguments +// in the expected manner + +const server = http2.createServer(); +server.on('stream', common.mustCall((stream, headers) => { + const port = server.address().port; + + // Must receive a callback (function) + common.expectsError( + () => stream.pushStream({ + ':scheme': 'http', + ':path': '/foobar', + ':authority': `localhost:${port}`, + }, {}, 'callback'), + { + code: 'ERR_INVALID_CALLBACK', + message: 'callback must be a function' + } + ); + + // Must validate headers + common.expectsError( + () => stream.pushStream({ 'connection': 'test' }, {}, () => {}), + { + code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS', + message: 'HTTP/1 Connection specific headers are forbidden' + } + ); + + stream.end('test'); +})); + +server.listen(0, common.mustCall(() => { + const port = server.address().port; + const headers = { ':path': '/' }; + const client = http2.connect(`http://localhost:${port}`); + const req = client.request(headers); + req.setEncoding('utf8'); + + let data = ''; + req.on('data', common.mustCall((d) => data += d)); + req.on('end', common.mustCall(() => { + assert.strictEqual(data, 'test'); + server.close(); + client.destroy(); + })); + req.end(); +})); diff --git a/test/parallel/test-http2-server-push-stream-errors.js b/test/parallel/test-http2-server-push-stream-errors.js new file mode 100644 index 00000000000000..ad26874f8a9f91 --- /dev/null +++ b/test/parallel/test-http2-server-push-stream-errors.js @@ -0,0 +1,130 @@ +// Flags: --expose-http2 +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const http2 = require('http2'); +const { + constants, + Http2Session, + nghttp2ErrorString +} = process.binding('http2'); + +// tests error handling within pushStream +// - NGHTTP2_ERR_NOMEM (should emit session error) +// - NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE (should emit session error) +// - NGHTTP2_ERR_STREAM_CLOSED (should emit stream error) +// - every other NGHTTP2 error from binding (should emit stream error) + +const specificTestKeys = [ + 'NGHTTP2_ERR_NOMEM', + 'NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE', + 'NGHTTP2_ERR_STREAM_CLOSED' +]; + +const specificTests = [ + { + ngError: constants.NGHTTP2_ERR_NOMEM, + error: { + code: 'ERR_OUTOFMEMORY', + type: Error, + message: 'Out of memory' + }, + type: 'session' + }, + { + ngError: constants.NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE, + error: { + code: 'ERR_HTTP2_OUT_OF_STREAMS', + type: Error, + message: 'No stream ID is available because ' + + 'maximum stream ID has been reached' + }, + type: 'session' + }, + { + ngError: constants.NGHTTP2_ERR_STREAM_CLOSED, + error: { + code: 'ERR_HTTP2_STREAM_CLOSED', + type: Error, + message: 'The stream is already closed' + }, + type: 'stream' + }, +]; + +const genericTests = Object.getOwnPropertyNames(constants) + .filter((key) => ( + key.indexOf('NGHTTP2_ERR') === 0 && specificTestKeys.indexOf(key) < 0 + )) + .map((key) => ({ + ngError: constants[key], + error: { + code: 'ERR_HTTP2_ERROR', + type: Error, + message: nghttp2ErrorString(constants[key]) + }, + type: 'stream' + })); + + +const tests = specificTests.concat(genericTests); + +let currentError; + +// mock submitPushPromise because we only care about testing error handling +Http2Session.prototype.submitPushPromise = () => currentError.ngError; + +const server = http2.createServer(); +server.on('stream', common.mustCall((stream, headers) => { + const errorMustCall = common.expectsError(currentError.error); + const errorMustNotCall = common.mustNotCall( + `${currentError.error.code} should emit on ${currentError.type}` + ); + console.log(currentError); + + if (currentError.type === 'stream') { + stream.session.on('error', errorMustNotCall); + stream.on('error', errorMustCall); + stream.on('error', common.mustCall(() => { + stream.respond(); + stream.end(); + })); + } else { + stream.session.once('error', errorMustCall); + stream.on('error', errorMustNotCall); + } + + stream.pushStream({}, () => {}); +}, tests.length)); + +server.listen(0, common.mustCall(() => runTest(tests.shift()))); + +function runTest(test) { + const port = server.address().port; + const url = `http://localhost:${port}`; + const headers = { + ':path': '/', + ':method': 'POST', + ':scheme': 'http', + ':authority': `localhost:${port}` + }; + + const client = http2.connect(url); + const req = client.request(headers); + + currentError = test; + req.resume(); + req.end(); + + req.on('end', common.mustCall(() => { + client.destroy(); + + if (!tests.length) { + server.close(); + } else { + runTest(tests.shift()); + } + })); +} diff --git a/test/parallel/test-http2-server-push-stream-head.js b/test/parallel/test-http2-server-push-stream-head.js new file mode 100644 index 00000000000000..a1172b8ecf3789 --- /dev/null +++ b/test/parallel/test-http2-server-push-stream-head.js @@ -0,0 +1,54 @@ +// Flags: --expose-http2 +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const http2 = require('http2'); + +// Check that pushStream handles method HEAD correctly +// - stream should end immediately (no body) + +const server = http2.createServer(); +server.on('stream', common.mustCall((stream, headers) => { + const port = server.address().port; + if (headers[':path'] === '/') { + stream.pushStream({ + ':scheme': 'http', + ':method': 'HEAD', + ':authority': `localhost:${port}`, + }, common.mustCall((push, headers) => { + assert.strictEqual(push._writableState.ended, true); + stream.end('test'); + })); + } + stream.respond({ + 'content-type': 'text/html', + ':status': 200 + }); +})); + +server.listen(0, common.mustCall(() => { + const port = server.address().port; + const headers = { ':path': '/' }; + const client = http2.connect(`http://localhost:${port}`); + const req = client.request(headers); + req.setEncoding('utf8'); + + client.on('stream', common.mustCall((stream, headers) => { + assert.strictEqual(headers[':scheme'], 'http'); + assert.strictEqual(headers[':path'], '/'); + assert.strictEqual(headers[':authority'], `localhost:${port}`); + })); + + let data = ''; + + req.on('data', common.mustCall((d) => data += d)); + req.on('end', common.mustCall(() => { + assert.strictEqual(data, 'test'); + server.close(); + client.destroy(); + })); + req.end(); +})); diff --git a/test/parallel/test-http2-server-push-stream.js b/test/parallel/test-http2-server-push-stream.js index c1de1195f76be3..c79ff7caecf0a9 100644 --- a/test/parallel/test-http2-server-push-stream.js +++ b/test/parallel/test-http2-server-push-stream.js @@ -15,7 +15,7 @@ server.on('stream', common.mustCall((stream, headers) => { ':scheme': 'http', ':path': '/foobar', ':authority': `localhost:${port}`, - }, (push, headers) => { + }, common.mustCall((push, headers) => { push.respond({ 'content-type': 'text/html', ':status': 200, @@ -23,7 +23,7 @@ server.on('stream', common.mustCall((stream, headers) => { }); push.end('pushed by server data'); stream.end('test'); - }); + })); } stream.respond({ 'content-type': 'text/html',