Skip to content

Commit

Permalink
http2: improve http2 coverage
Browse files Browse the repository at this point in the history
Improve http2 coverage through refactoring and tests

PR-URL: nodejs/node#15210
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
jasnell authored and addaleax committed Sep 17, 2017
1 parent 4c11362 commit a07a910
Show file tree
Hide file tree
Showing 6 changed files with 196 additions and 68 deletions.
106 changes: 41 additions & 65 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const {
isPayloadMeaningless,
mapToHeaders,
NghttpError,
sessionName,
toHeaderObject,
updateOptionsBuffer,
updateSettingsBuffer
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand All @@ -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));
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
20 changes: 17 additions & 3 deletions lib/internal/http2/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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,
Expand All @@ -527,6 +540,7 @@ module.exports = {
isPayloadMeaningless,
mapToHeaders,
NghttpError,
sessionName,
toHeaderObject,
updateOptionsBuffer,
updateSettingsBuffer
Expand Down
2 changes: 2 additions & 0 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
45 changes: 45 additions & 0 deletions test/parallel/test-http2-createsecureserver-nooptions.js
Original file line number Diff line number Diff line change
@@ -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
});
49 changes: 49 additions & 0 deletions test/parallel/test-http2-misc-util.js
Original file line number Diff line number Diff line change
@@ -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');
42 changes: 42 additions & 0 deletions test/parallel/test-http2-server-settimeout-no-callback.js
Original file line number Diff line number Diff line change
@@ -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
});
}

0 comments on commit a07a910

Please sign in to comment.