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: nodejs#14991 PR-URL: nodejs#19232 Reviewed-By: James M Snell <[email protected]>
mcollina · Mar 20, 2018 · c2bf08e · c2bf08e
1 parent d2b99e3
commit c2bf08e
Show file tree

Hide file tree

Showing 6 changed files with 35 additions and 12 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);
 }));