From f36e5b413a8813de6b3a699850eede936c0af451 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 8 Mar 2018 16:22:23 +0100 Subject: [PATCH] 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 --- lib/internal/http2/core.js | 7 ------- test/parallel/test-http2-misbehaving-multiplex.js | 7 +++++++ test/parallel/test-http2-server-errors.js | 8 ++++++-- .../parallel/test-http2-server-push-stream-head.js | 6 ++++++ test/parallel/test-http2-server-rst-stream.js | 14 +++++++++++--- .../test-http2-server-stream-session-destroy.js | 5 +++++ 6 files changed, 35 insertions(+), 12 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 4aee67b825605c..86e33d9cda0da8 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -2041,19 +2041,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 diff --git a/test/parallel/test-http2-misbehaving-multiplex.js b/test/parallel/test-http2-misbehaving-multiplex.js index 7d5a7a2f552d49..757e66b100e435 100644 --- a/test/parallel/test-http2-misbehaving-multiplex.js +++ b/test/parallel/test-http2-misbehaving-multiplex.js @@ -14,7 +14,14 @@ 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({ + 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', diff --git a/test/parallel/test-http2-server-errors.js b/test/parallel/test-http2-server-errors.js index a3586bd64d46e7..bbdf312fc153d4 100644 --- a/test/parallel/test-http2-server-errors.js +++ b/test/parallel/test-http2-server-errors.js @@ -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(); })); diff --git a/test/parallel/test-http2-server-push-stream-head.js b/test/parallel/test-http2-server-push-stream-head.js index cd2276746f4bdd..35e22c0ce3b561 100644 --- a/test/parallel/test-http2-server-push-stream-head.js +++ b/test/parallel/test-http2-server-push-stream-head.js @@ -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'); })); diff --git a/test/parallel/test-http2-server-rst-stream.js b/test/parallel/test-http2-server-rst-stream.js index d19704509a5e5f..6e4e9ccb88e611 100644 --- a/test/parallel/test-http2-server-rst-stream.js +++ b/test/parallel/test-http2-server-rst-stream.js @@ -18,14 +18,22 @@ const { const tests = [ [NGHTTP2_NO_ERROR, false], [NGHTTP2_NO_ERROR, false], - [NGHTTP2_PROTOCOL_ERROR, true], + [NGHTTP2_PROTOCOL_ERROR, true, 1], [NGHTTP2_CANCEL, false], - [NGHTTP2_REFUSED_STREAM, true], - [NGHTTP2_INTERNAL_ERROR, true] + [NGHTTP2_REFUSED_STREAM, true, 7], + [NGHTTP2_INTERNAL_ERROR, true, 2] ]; 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); }); diff --git a/test/parallel/test-http2-server-stream-session-destroy.js b/test/parallel/test-http2-server-stream-session-destroy.js index 5eb04a8d376635..989c72ec959679 100644 --- a/test/parallel/test-http2-server-stream-session-destroy.js +++ b/test/parallel/test-http2-server-stream-session-destroy.js @@ -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); }));