Skip to content

Commit

Permalink
http2: fixes error handling
Browse files Browse the repository at this point in the history
There should be no default error handling when using Http2Stream.
All errors will end up in `'streamError'` on the server anyway,
but they are emitted on `'stream'` as well, otherwise some error
conditions are impossible to debug.

See: nodejs#14991
  • Loading branch information
mcollina committed Mar 8, 2018
1 parent 4d07434 commit 80f9390
Show file tree
Hide file tree
Showing 6 changed files with 36 additions and 12 deletions.
7 changes: 0 additions & 7 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -2073,19 +2073,12 @@ function afterOpen(session, options, headers, streamOptions, err, fd) {
headers, streamOptions));
}

function streamOnError(err) {
// we swallow the error for parity with HTTP1
// all the errors that ends here are not critical for the project
}


class ServerHttp2Stream extends Http2Stream {
constructor(session, handle, id, options, headers) {
super(session, options);
this[kInit](id, handle);
this[kProtocol] = headers[HTTP2_HEADER_SCHEME];
this[kAuthority] = headers[HTTP2_HEADER_AUTHORITY];
this.on('error', streamOnError);
}

// true if the remote peer accepts push streams
Expand Down
8 changes: 8 additions & 0 deletions test/parallel/test-http2-misbehaving-multiplex.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,15 @@ const server = h2.createServer();
server.on('stream', common.mustCall((stream) => {
stream.respond();
stream.end('ok');

// the error will be emitted asynchronously
stream.on('error', common.expectsError({
type: NghttpError,
code: 'ERR_HTTP2_ERROR',
message: 'Stream was already closed or invalid'
}));
}, 2));

server.on('session', common.mustCall((session) => {
session.on('error', common.expectsError({
code: 'ERR_HTTP2_ERROR',
Expand Down
8 changes: 6 additions & 2 deletions test/parallel/test-http2-server-errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,16 @@ const h2 = require('http2');

const server = h2.createServer();

process.on('uncaughtException', common.mustCall(function(err) {
assert.strictEqual(err.message, 'kaboom no handler');
}));

server.on('stream', common.mustCall(function(stream) {
// there is no 'error' handler, and this will not crash
// there is no 'error' handler, and this will crash
stream.write('hello');
stream.resume();

expected = new Error('kaboom');
expected = new Error('kaboom no handler');
stream.destroy(expected);
server.close();
}));
Expand Down
6 changes: 6 additions & 0 deletions test/parallel/test-http2-server-push-stream-head.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ server.on('stream', common.mustCall((stream, headers) => {
}, common.mustCall((err, push, headers) => {
assert.strictEqual(push._writableState.ended, true);
push.respond();
// cannot write to push() anymore
push.on('error', common.expectsError({
type: Error,
code: 'ERR_STREAM_WRITE_AFTER_END',
message: 'write after end'
}));
assert(!push.write('test'));
stream.end('test');
}));
Expand Down
14 changes: 11 additions & 3 deletions test/parallel/test-http2-server-rst-stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,22 @@ const {
const tests = [
[NGHTTP2_NO_ERROR, false],
[NGHTTP2_NO_ERROR, false],
[NGHTTP2_PROTOCOL_ERROR, true],
[NGHTTP2_PROTOCOL_ERROR, true, 'NGHTTP2_PROTOCOL_ERROR'],
[NGHTTP2_CANCEL, false],
[NGHTTP2_REFUSED_STREAM, true],
[NGHTTP2_INTERNAL_ERROR, true]
[NGHTTP2_REFUSED_STREAM, true, 'NGHTTP2_REFUSED_STREAM'],
[NGHTTP2_INTERNAL_ERROR, true, 'NGHTTP2_INTERNAL_ERROR']
];

const server = http2.createServer();
server.on('stream', (stream, headers) => {
const test = tests.find((t) => t[0] === Number(headers.rstcode));
if (test[1]) {
stream.on('error', common.expectsError({
type: Error,
code: 'ERR_HTTP2_STREAM_ERROR',
message: `Stream closed with error code ${test[2]}`
}));
}
stream.close(headers.rstcode | 0);
});

Expand Down
5 changes: 5 additions & 0 deletions test/parallel/test-http2-server-stream-session-destroy.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ server.on('stream', common.mustCall((stream) => {
type: Error
}
);
stream.on('error', common.expectsError({
type: Error,
code: 'ERR_STREAM_WRITE_AFTER_END',
message: 'write after end'
}));
assert.strictEqual(stream.write('data'), false);
}));

Expand Down

0 comments on commit 80f9390

Please sign in to comment.