http2: fixes error handling

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: https://github.com/nodejs/node/pull/14991

PR-URL: https://github.com/nodejs/node/pull/19232
Reviewed-By: James M Snell <jasnell@gmail.com>
This commit is contained in:
Matteo Collina 2018-03-08 16:22:23 +01:00
parent 33e63fe64f
commit 8403f00dc3
6 changed files with 36 additions and 12 deletions

View File

@ -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

View File

@ -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',

View File

@ -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();
}));

View File

@ -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');
}));

View File

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

View File

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