http2: improve http2 coverage

Improve http2 coverage through refactoring and tests

PR-URL: https://github.com/nodejs/node/pull/15210
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 '<invalid>';
-  }
-}
-
 // Top level to avoid creating a closure
 function emit() {
   this.emit.apply(this, arguments);
@@ -163,8 +152,8 @@ function onSessionHeaders(id, cat, flags, headers) {
   const obj = toHeaderObject(headers);
 
   if (stream === undefined) {
-    switch (owner[kType]) {
+    // owner[kType] can be only one of two possible values
-      case NGHTTP2_SESSION_SERVER:
+    if (owner[kType] === NGHTTP2_SESSION_SERVER) {
       stream = new ServerHttp2Stream(owner, id,
                                      { readable: !endOfStream },
                                      obj);
@@ -175,23 +164,15 @@ function onSessionHeaders(id, cat, flags, headers) {
         const state = stream[kState];
         state.headRequest = true;
       }
-        break;
+    } else {
-      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');
     }
     streams.set(id, stream);
     process.nextTick(emit.bind(owner, 'stream', stream, obj, flags, headers));
   } else {
     let event;
-    let status;
+    const status = obj[HTTP2_HEADER_STATUS];
-    switch (cat) {
+    if (cat === NGHTTP2_HCAT_RESPONSE) {
-      case NGHTTP2_HCAT_RESPONSE:
-        status = obj[HTTP2_HEADER_STATUS];
       if (!endOfStream &&
           status !== undefined &&
           status >= 100 &&
@@ -200,22 +181,14 @@ function onSessionHeaders(id, cat, flags, headers) {
       } else {
         event = 'response';
       }
-        break;
+    } else if (cat === NGHTTP2_HCAT_PUSH_RESPONSE) {
-      case NGHTTP2_HCAT_PUSH_RESPONSE:
       event = 'push';
-        break;
+    } else {  // cat === NGHTTP2_HCAT_HEADERS:
-      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');
     }
     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')
+  // We should not have been able to get here at all if getTrailers
-    return [];
+  // 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') {
+  if (options == null || typeof options !== 'object') {
-    handler = options;
+    throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
-    options = Object.create(null);
+                               'options',
+                               'object');
   }
   debug('creating http2secureserver');
   return new Http2SecureServer(options, handler);

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',
+    const err = new errors.TypeError('ERR_INVALID_ARG_TYPE', name, types);
-                                     name, types || 'object');
     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 '<invalid>';
+  }
+}
+
 module.exports = {
   assertIsObject,
   assertValidPseudoHeaderResponse,
@@ -527,6 +540,7 @@ module.exports = {
   isPayloadMeaningless,
   mapToHeaders,
   NghttpError,
+  sessionName,
   toHeaderObject,
   updateOptionsBuffer,
   updateSettingsBuffer

src/node_http2.cc

@@ -842,6 +842,8 @@ void Http2Session::OnHeaders(Nghttp2Stream* stream,
   Local<Function> fn = env()->push_values_to_array_function();
   Local<Value> 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.

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
+  });

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), '<invalid>');
+});
+
+// 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');

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
+    });
+}