From a07a91092fa4230c477db5330cd3ac70b51f998a Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 5 Sep 2017 15:02:48 -0700 Subject: [PATCH] http2: improve http2 coverage Improve http2 coverage through refactoring and tests PR-URL: https://github.com/nodejs/node/pull/15210 Reviewed-By: Matteo Collina --- lib/internal/http2/core.js | 106 +++++++----------- lib/internal/http2/util.js | 20 +++- src/node_http2.cc | 2 + ...test-http2-createsecureserver-nooptions.js | 45 ++++++++ test/parallel/test-http2-misc-util.js | 49 ++++++++ ...est-http2-server-settimeout-no-callback.js | 42 +++++++ 6 files changed, 196 insertions(+), 68 deletions(-) create mode 100644 test/parallel/test-http2-createsecureserver-nooptions.js create mode 100644 test/parallel/test-http2-misc-util.js create mode 100644 test/parallel/test-http2-server-settimeout-no-callback.js diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index eb6b605f67..cb9bfc37ce 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -37,6 +37,7 @@ const { isPayloadMeaningless, mapToHeaders, NghttpError, + sessionName, toHeaderObject, updateOptionsBuffer, updateSettingsBuffer @@ -87,7 +88,6 @@ const { NGHTTP2_CANCEL, NGHTTP2_DEFAULT_WEIGHT, NGHTTP2_FLAG_END_STREAM, - NGHTTP2_HCAT_HEADERS, NGHTTP2_HCAT_PUSH_RESPONSE, NGHTTP2_HCAT_RESPONSE, NGHTTP2_INTERNAL_ERROR, @@ -128,17 +128,6 @@ const { HTTP_STATUS_SWITCHING_PROTOCOLS } = constants; -function sessionName(type) { - switch (type) { - case NGHTTP2_SESSION_CLIENT: - return 'client'; - case NGHTTP2_SESSION_SERVER: - return 'server'; - default: - return ''; - } -} - // Top level to avoid creating a closure function emit() { this.emit.apply(this, arguments); @@ -163,59 +152,43 @@ function onSessionHeaders(id, cat, flags, headers) { const obj = toHeaderObject(headers); if (stream === undefined) { - switch (owner[kType]) { - case NGHTTP2_SESSION_SERVER: - stream = new ServerHttp2Stream(owner, id, - { readable: !endOfStream }, - obj); - if (obj[HTTP2_HEADER_METHOD] === HTTP2_METHOD_HEAD) { - // For head requests, there must not be a body... - // end the writable side immediately. - stream.end(); - const state = stream[kState]; - state.headRequest = true; - } - break; - case NGHTTP2_SESSION_CLIENT: - stream = new ClientHttp2Stream(owner, id, { readable: !endOfStream }); - break; - default: - assert.fail(null, null, - 'Internal HTTP/2 Error. Invalid session type. Please ' + - 'report this as a bug in Node.js'); + // owner[kType] can be only one of two possible values + if (owner[kType] === NGHTTP2_SESSION_SERVER) { + stream = new ServerHttp2Stream(owner, id, + { readable: !endOfStream }, + obj); + if (obj[HTTP2_HEADER_METHOD] === HTTP2_METHOD_HEAD) { + // For head requests, there must not be a body... + // end the writable side immediately. + stream.end(); + const state = stream[kState]; + state.headRequest = true; + } + } else { + stream = new ClientHttp2Stream(owner, id, { readable: !endOfStream }); } streams.set(id, stream); process.nextTick(emit.bind(owner, 'stream', stream, obj, flags, headers)); } else { let event; - let status; - switch (cat) { - case NGHTTP2_HCAT_RESPONSE: - status = obj[HTTP2_HEADER_STATUS]; - if (!endOfStream && - status !== undefined && - status >= 100 && - status < 200) { - event = 'headers'; - } else { - event = 'response'; - } - break; - case NGHTTP2_HCAT_PUSH_RESPONSE: - event = 'push'; - break; - case NGHTTP2_HCAT_HEADERS: - status = obj[HTTP2_HEADER_STATUS]; - if (!endOfStream && status !== undefined && status >= 200) { - event = 'response'; - } else { - event = endOfStream ? 'trailers' : 'headers'; - } - break; - default: - assert.fail(null, null, - 'Internal HTTP/2 Error. Invalid headers category. Please ' + - 'report this as a bug in Node.js'); + const status = obj[HTTP2_HEADER_STATUS]; + if (cat === NGHTTP2_HCAT_RESPONSE) { + if (!endOfStream && + status !== undefined && + status >= 100 && + status < 200) { + event = 'headers'; + } else { + event = 'response'; + } + } else if (cat === NGHTTP2_HCAT_PUSH_RESPONSE) { + event = 'push'; + } else { // cat === NGHTTP2_HCAT_HEADERS: + if (!endOfStream && status !== undefined && status >= 200) { + event = 'response'; + } else { + event = endOfStream ? 'trailers' : 'headers'; + } } debug(`[${sessionName(owner[kType])}] emitting stream '${event}' event`); process.nextTick(emit.bind(stream, event, obj, flags, headers)); @@ -242,8 +215,10 @@ function onSessionTrailers(id) { 'Internal HTTP/2 Failure. Stream does not exist. Please ' + 'report this as a bug in Node.js'); const getTrailers = stream[kState].getTrailers; - if (typeof getTrailers !== 'function') - return []; + // We should not have been able to get here at all if getTrailers + // was not a function, and there's no code that could have changed + // it between there and here. + assert.strictEqual(typeof getTrailers, 'function'); const trailers = Object.create(null); getTrailers.call(stream, trailers); const headersList = mapToHeaders(trailers, assertValidPseudoHeaderTrailer); @@ -2480,9 +2455,10 @@ function connect(authority, options, listener) { } function createSecureServer(options, handler) { - if (typeof options === 'function') { - handler = options; - options = Object.create(null); + if (options == null || typeof options !== 'object') { + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', + 'options', + 'object'); } debug('creating http2secureserver'); return new Http2SecureServer(options, handler); diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index 17f4c22252..267ea4fe4f 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -4,6 +4,9 @@ const binding = process.binding('http2'); const errors = require('internal/errors'); const { + NGHTTP2_SESSION_CLIENT, + NGHTTP2_SESSION_SERVER, + HTTP2_HEADER_STATUS, HTTP2_HEADER_METHOD, HTTP2_HEADER_AUTHORITY, @@ -439,13 +442,12 @@ class NghttpError extends Error { } } -function assertIsObject(value, name, types) { +function assertIsObject(value, name, types = 'object') { if (value !== undefined && (value === null || typeof value !== 'object' || Array.isArray(value))) { - const err = new errors.TypeError('ERR_INVALID_ARG_TYPE', - name, types || 'object'); + const err = new errors.TypeError('ERR_INVALID_ARG_TYPE', name, types); Error.captureStackTrace(err, assertIsObject); throw err; } @@ -515,6 +517,17 @@ function isPayloadMeaningless(method) { return kNoPayloadMethods.has(method); } +function sessionName(type) { + switch (type) { + case NGHTTP2_SESSION_CLIENT: + return 'client'; + case NGHTTP2_SESSION_SERVER: + return 'server'; + default: + return ''; + } +} + module.exports = { assertIsObject, assertValidPseudoHeaderResponse, @@ -527,6 +540,7 @@ module.exports = { isPayloadMeaningless, mapToHeaders, NghttpError, + sessionName, toHeaderObject, updateOptionsBuffer, updateSettingsBuffer diff --git a/src/node_http2.cc b/src/node_http2.cc index 65528fc98e..c1e178f717 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -842,6 +842,8 @@ void Http2Session::OnHeaders(Nghttp2Stream* stream, Local fn = env()->push_values_to_array_function(); Local argv[NODE_PUSH_VAL_TO_ARRAY_MAX * 2]; + CHECK_LE(cat, NGHTTP2_HCAT_HEADERS); + // The headers are passed in above as a linked list of nghttp2_header_list // structs. The following converts that into a JS array with the structure: // [name1, value1, name2, value2, name3, value3, name3, value4] and so on. diff --git a/test/parallel/test-http2-createsecureserver-nooptions.js b/test/parallel/test-http2-createsecureserver-nooptions.js new file mode 100644 index 0000000000..5a9849f9e1 --- /dev/null +++ b/test/parallel/test-http2-createsecureserver-nooptions.js @@ -0,0 +1,45 @@ +// Flags: --expose-http2 +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const http2 = require('http2'); + +// Error if options are not passed to createSecureServer + +common.expectsError( + () => http2.createSecureServer(), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError + }); + +common.expectsError( + () => http2.createSecureServer(() => {}), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError + }); + +common.expectsError( + () => http2.createSecureServer(1), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError + }); + +common.expectsError( + () => http2.createSecureServer('test'), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError + }); + +common.expectsError( + () => http2.createSecureServer(null), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError + }); diff --git a/test/parallel/test-http2-misc-util.js b/test/parallel/test-http2-misc-util.js new file mode 100644 index 0000000000..eaa297309b --- /dev/null +++ b/test/parallel/test-http2-misc-util.js @@ -0,0 +1,49 @@ +// Flags: --expose-internals +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); + +const { + assertIsObject, + assertWithinRange, + sessionName +} = require('internal/http2/util'); + +// Code coverage for sessionName utility function +assert.strictEqual(sessionName(0), 'server'); +assert.strictEqual(sessionName(1), 'client'); +[2, '', 'test', {}, [], true].forEach((i) => { + assert.strictEqual(sessionName(2), ''); +}); + +// Code coverage for assertWithinRange function +common.expectsError( + () => assertWithinRange('test', -1), + { + code: 'ERR_HTTP2_INVALID_SETTING_VALUE', + type: RangeError, + message: 'Invalid value for setting "test": -1' + }); + +assertWithinRange('test', 1); + +common.expectsError( + () => assertIsObject('foo', 'test'), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "test" argument must be of type object' + }); + +common.expectsError( + () => assertIsObject('foo', 'test', ['Date']), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "test" argument must be of type Date' + }); + +assertIsObject({}, 'test'); diff --git a/test/parallel/test-http2-server-settimeout-no-callback.js b/test/parallel/test-http2-server-settimeout-no-callback.js new file mode 100644 index 0000000000..eff0418fa7 --- /dev/null +++ b/test/parallel/test-http2-server-settimeout-no-callback.js @@ -0,0 +1,42 @@ +// Flags: --expose-http2 +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const http2 = require('http2'); + +// Verify that setTimeout callback verifications work correctly + +{ + const server = http2.createServer(); + common.expectsError( + () => server.setTimeout(10, 'test'), + { + code: 'ERR_INVALID_CALLBACK', + type: TypeError + }); + common.expectsError( + () => server.setTimeout(10, 1), + { + code: 'ERR_INVALID_CALLBACK', + type: TypeError + }); +} + +{ + const server = http2.createSecureServer({}); + common.expectsError( + () => server.setTimeout(10, 'test'), + { + code: 'ERR_INVALID_CALLBACK', + type: TypeError + }); + common.expectsError( + () => server.setTimeout(10, 1), + { + code: 'ERR_INVALID_CALLBACK', + type: TypeError + }); +}