From 167382b1953698ba71e5a9f0d5b228d7abae15f2 Mon Sep 17 00:00:00 2001
From: Matteo Collina <hello@matteocollina.com>
Date: Wed, 23 Aug 2017 18:29:49 +0200
Subject: [PATCH] http2: refactor error handling

This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing

See: https://github.com/nodejs/node/issues/14963

PR-URL: https://github.com/nodejs/node/pull/14991
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
---
 doc/api/http2.md                              | 39 +++++++-
 lib/http2.js                                  |  4 +-
 lib/internal/http2/compat.js                  | 16 ++-
 lib/internal/http2/core.js                    | 43 ++++++--
 ...p2-client-stream-destroy-before-connect.js | 17 +---
 test/parallel/test-http2-compat-errors.js     | 52 ++++++++++
 test/parallel/test-http2-respond-file-404.js  | 47 +++++++++
 .../test-http2-respond-file-error-dir.js      | 46 +++++++++
 test/parallel/test-http2-server-errors.js     | 97 +++++++++++++++++++
 9 files changed, 328 insertions(+), 33 deletions(-)
 create mode 100644 test/parallel/test-http2-compat-errors.js
 create mode 100644 test/parallel/test-http2-respond-file-404.js
 create mode 100644 test/parallel/test-http2-respond-file-error-dir.js
 create mode 100644 test/parallel/test-http2-server-errors.js

diff --git a/doc/api/http2.md b/doc/api/http2.md
index d9b8aa97b0..57e1b74e7e 100755
--- a/doc/api/http2.md
+++ b/doc/api/http2.md
@@ -1118,6 +1118,8 @@ added: v8.4.0
 * `headers` {[Headers Object][]}
 * `options` {Object}
   * `statCheck` {Function}
+  * `onError` {Function} Callback function invoked in the case of an
+    Error before send
   * `getTrailers` {Function} Callback function invoked to collect trailer
     headers.
   * `offset` {number} The offset position at which to begin reading
@@ -1146,6 +1148,16 @@ server.on('stream', (stream) => {
   function statCheck(stat, headers) {
     headers['last-modified'] = stat.mtime.toUTCString();
   }
+
+  function onError(err) {
+    if (err.code === 'ENOENT') {
+      stream.respond({ ':status': 404 });
+    } else {
+      stream.respond({ ':status': 500 });
+    }
+    stream.end();
+  }
+
   stream.respondWithFile('/some/file',
                          { 'content-type': 'text/plain' },
                          { statCheck });
@@ -1178,6 +1190,10 @@ The `offset` and `length` options may be used to limit the response to a
 specific range subset. This can be used, for instance, to support HTTP Range
 requests.
 
+The `options.onError` function may also be used to handle all the errors
+that could happen before the delivery of the file is initiated. The
+default behavior is to destroy the stream.
+
 When set, the `options.getTrailers()` function is called immediately after
 queuing the last chunk of payload data to be sent. The callback is passed a
 single object (with a `null` prototype) that the listener may used to specify
@@ -1208,6 +1224,19 @@ added: v8.4.0
 
 * Extends: {net.Server}
 
+In `Http2Server`, there is no `'clientError'` event as there is in
+HTTP1. However, there are `'socketError'`, `'sessionError'`,  and
+`'streamError'`, for error happened on the socket, session or stream
+respectively.
+
+#### Event: 'socketError'
+<!-- YAML
+added: v8.4.0
+-->
+
+The `'socketError'` event is emitted when a `'socketError'` event is emitted by
+an `Http2Session` associated with the server.
+
 #### Event: 'sessionError'
 <!-- YAML
 added: v8.4.0
@@ -1217,13 +1246,15 @@ The `'sessionError'` event is emitted when an `'error'` event is emitted by
 an `Http2Session` object. If no listener is registered for this event, an
 `'error'` event is emitted.
 
-#### Event: 'socketError'
+#### Event: 'streamError'
 <!-- YAML
-added: v8.4.0
+added: REPLACEME
 -->
 
-The `'socketError'` event is emitted when a `'socketError'` event is emitted by
-an `Http2Session` associated with the server.
+* `socket` {http2.ServerHttp2Stream}
+
+If an `ServerHttp2Stream` emits an `'error'` event, it will be forwarded here.
+The stream will already be destroyed when this event is triggered.
 
 #### Event: 'stream'
 <!-- YAML
diff --git a/lib/http2.js b/lib/http2.js
index ab9e33b2a2..de06de1cc4 100644
--- a/lib/http2.js
+++ b/lib/http2.js
@@ -15,7 +15,7 @@ const {
   createSecureServer,
   connect,
   Http2ServerRequest,
-  Http2ServerResponse,
+  Http2ServerResponse
 } = require('internal/http2/core');
 
 module.exports = {
@@ -27,5 +27,5 @@ module.exports = {
   createSecureServer,
   connect,
   Http2ServerResponse,
-  Http2ServerRequest,
+  Http2ServerRequest
 };
diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js
index c2ada307dd..470a765b6b 100644
--- a/lib/internal/http2/compat.js
+++ b/lib/internal/http2/compat.js
@@ -58,8 +58,13 @@ function onStreamEnd() {
 }
 
 function onStreamError(error) {
-  const request = this[kRequest];
-  request.emit('error', error);
+  // this is purposefully left blank
+  //
+  // errors in compatibility mode are
+  // not forwarded to the request
+  // and response objects. However,
+  // they are forwarded to 'clientError'
+  // on the server by Http2Stream
 }
 
 function onRequestPause() {
@@ -82,11 +87,6 @@ function onStreamResponseDrain() {
   response.emit('drain');
 }
 
-function onStreamResponseError(error) {
-  const response = this[kResponse];
-  response.emit('error', error);
-}
-
 function onStreamClosedRequest() {
   const req = this[kRequest];
   req.push(null);
@@ -271,9 +271,7 @@ class Http2ServerResponse extends Stream {
     stream[kResponse] = this;
     this.writable = true;
     stream.on('drain', onStreamResponseDrain);
-    stream.on('error', onStreamResponseError);
     stream.on('close', onStreamClosedResponse);
-    stream.on('aborted', onAborted.bind(this));
     const onfinish = this[kFinish].bind(this);
     stream.on('streamClosed', onfinish);
     stream.on('finish', onfinish);
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index 92d8e6077c..d45acb29f1 100755
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -1506,6 +1506,13 @@ class Http2Stream extends Duplex {
       this.once('ready', this._destroy.bind(this, err, callback));
       return;
     }
+
+    const server = session[kServer];
+
+    if (err && server) {
+      server.emit('streamError', err, this);
+    }
+
     process.nextTick(() => {
       debug(`[${sessionName(session[kType])}] destroying stream ${this[kID]}`);
 
@@ -1529,9 +1536,8 @@ class Http2Stream extends Duplex {
       // All done
       const rst = this[kState].rst;
       const code = rst ? this[kState].rstCode : NGHTTP2_NO_ERROR;
-      if (code !== NGHTTP2_NO_ERROR) {
-        const err = new errors.Error('ERR_HTTP2_STREAM_ERROR', code);
-        process.nextTick(() => this.emit('error', err));
+      if (!err && code !== NGHTTP2_NO_ERROR) {
+        err = new errors.Error('ERR_HTTP2_STREAM_ERROR', code);
       }
       process.nextTick(emit.bind(this, 'streamClosed', code));
       debug(`[${sessionName(session[kType])}] stream ${this[kID]} destroyed`);
@@ -1634,13 +1640,24 @@ function doSendFileFD(session, options, fd, headers, getTrailers, err, stat) {
     abort(this);
     return;
   }
+  const onError = options.onError;
+
   if (err) {
-    process.nextTick(() => this.emit('error', err));
+    if (onError) {
+      onError(err);
+    } else {
+      this.destroy(err);
+    }
     return;
   }
+
   if (!stat.isFile()) {
     err = new errors.Error('ERR_HTTP2_SEND_FILE');
-    process.nextTick(() => this.emit('error', err));
+    if (onError) {
+      onError(err);
+    } else {
+      this.destroy(err);
+    }
     return;
   }
 
@@ -1677,12 +1694,17 @@ function doSendFileFD(session, options, fd, headers, getTrailers, err, stat) {
 
 function afterOpen(session, options, headers, getTrailers, err, fd) {
   const state = this[kState];
+  const onError = options.onError;
   if (this.destroyed || session.destroyed) {
     abort(this);
     return;
   }
   if (err) {
-    process.nextTick(() => this.emit('error', err));
+    if (onError) {
+      onError(err);
+    } else {
+      this.destroy(err);
+    }
     return;
   }
   state.fd = fd;
@@ -1691,6 +1713,12 @@ function afterOpen(session, options, headers, getTrailers, err, fd) {
            doSendFileFD.bind(this, session, options, fd, headers, getTrailers));
 }
 
+function streamOnError(err) {
+  // we swallow the error for parity with HTTP1
+  // all the errors that ends here are not critical for the project
+  debug('ServerHttp2Stream errored, avoiding uncaughtException', err);
+}
+
 
 class ServerHttp2Stream extends Http2Stream {
   constructor(session, id, options, headers) {
@@ -1698,6 +1726,7 @@ class ServerHttp2Stream extends Http2Stream {
     this[kInit](id);
     this[kProtocol] = headers[HTTP2_HEADER_SCHEME];
     this[kAuthority] = headers[HTTP2_HEADER_AUTHORITY];
+    this.on('error', streamOnError);
     debug(`[${sessionName(session[kType])}] created serverhttp2stream`);
   }
 
@@ -2556,6 +2585,8 @@ module.exports = {
   createServer,
   createSecureServer,
   connect,
+  Http2Session,
+  Http2Stream,
   Http2ServerRequest,
   Http2ServerResponse
 };
diff --git a/test/parallel/test-http2-client-stream-destroy-before-connect.js b/test/parallel/test-http2-client-stream-destroy-before-connect.js
index d68e6b33ec..05ba63f0b5 100644
--- a/test/parallel/test-http2-client-stream-destroy-before-connect.js
+++ b/test/parallel/test-http2-client-stream-destroy-before-connect.js
@@ -36,18 +36,11 @@ server.on('listening', common.mustCall(() => {
   req.destroy(err);
 
   req.on('error', common.mustCall((err) => {
-    const fn = err.code === 'ERR_HTTP2_STREAM_ERROR' ?
-      common.expectsError({
-        code: 'ERR_HTTP2_STREAM_ERROR',
-        type: Error,
-        message: 'Stream closed with error code 2'
-      }) :
-      common.expectsError({
-        type: Error,
-        message: 'test'
-      });
-    fn(err);
-  }, 2));
+    common.expectsError({
+      type: Error,
+      message: 'test'
+    })(err);
+  }));
 
   req.on('streamClosed', common.mustCall((code) => {
     assert.strictEqual(req.rstCode, NGHTTP2_INTERNAL_ERROR);
diff --git a/test/parallel/test-http2-compat-errors.js b/test/parallel/test-http2-compat-errors.js
new file mode 100644
index 0000000000..dacb39c6df
--- /dev/null
+++ b/test/parallel/test-http2-compat-errors.js
@@ -0,0 +1,52 @@
+// Flags: --expose-http2 --expose-internals
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const h2 = require('http2');
+const { Http2Stream } = require('internal/http2/core');
+
+// Errors should not be reported both in Http2ServerRequest
+// and Http2ServerResponse
+
+let expected = null;
+
+const server = h2.createServer(common.mustCall(function(req, res) {
+  req.on('error', common.mustNotCall());
+  res.on('error', common.mustNotCall());
+  req.on('aborted', common.mustCall());
+  res.on('aborted', common.mustNotCall());
+
+  res.write('hello');
+
+  expected = new Error('kaboom');
+  res.stream.destroy(expected);
+  server.close();
+}));
+
+server.on('streamError', common.mustCall(function(err, stream) {
+  assert.strictEqual(err, expected);
+  assert.strictEqual(stream instanceof Http2Stream, true);
+}));
+
+server.listen(0, common.mustCall(function() {
+  const port = server.address().port;
+
+  const url = `http://localhost:${port}`;
+  const client = h2.connect(url, common.mustCall(function() {
+    const headers = {
+      ':path': '/foobar',
+      ':method': 'GET',
+      ':scheme': 'http',
+      ':authority': `localhost:${port}`,
+    };
+    const request = client.request(headers);
+    request.on('data', common.mustCall(function(chunk) {
+      // cause an error on the server side
+      client.destroy();
+    }));
+    request.end();
+  }));
+}));
diff --git a/test/parallel/test-http2-respond-file-404.js b/test/parallel/test-http2-respond-file-404.js
new file mode 100644
index 0000000000..7158417f2b
--- /dev/null
+++ b/test/parallel/test-http2-respond-file-404.js
@@ -0,0 +1,47 @@
+// Flags: --expose-http2
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const http2 = require('http2');
+const assert = require('assert');
+
+const {
+  HTTP2_HEADER_CONTENT_TYPE
+} = http2.constants;
+
+const server = http2.createServer();
+server.on('stream', (stream) => {
+  const file = './not-a-file';
+  stream.respondWithFile('./not-a-file', {
+    [HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
+  }, {
+    onError(err) {
+      common.expectsError({
+        code: 'ENOENT',
+        type: Error,
+        message: `ENOENT: no such file or directory, open '${file}'`
+      })(err);
+
+      stream.respond({ ':status': 404 });
+      stream.end();
+    },
+    statCheck: common.mustNotCall()
+  });
+});
+server.listen(0, () => {
+
+  const client = http2.connect(`http://localhost:${server.address().port}`);
+  const req = client.request();
+
+  req.on('response', common.mustCall((headers) => {
+    assert.strictEqual(headers[':status'], 404);
+  }));
+  req.on('data', common.mustNotCall());
+  req.on('end', common.mustCall(() => {
+    client.destroy();
+    server.close();
+  }));
+  req.end();
+});
diff --git a/test/parallel/test-http2-respond-file-error-dir.js b/test/parallel/test-http2-respond-file-error-dir.js
new file mode 100644
index 0000000000..949ef2ffa4
--- /dev/null
+++ b/test/parallel/test-http2-respond-file-error-dir.js
@@ -0,0 +1,46 @@
+// Flags: --expose-http2
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const http2 = require('http2');
+const assert = require('assert');
+
+const {
+  HTTP2_HEADER_CONTENT_TYPE
+} = http2.constants;
+
+const server = http2.createServer();
+server.on('stream', (stream) => {
+  stream.respondWithFile('../', {
+    [HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
+  }, {
+    onError(err) {
+      common.expectsError({
+        code: 'ERR_HTTP2_SEND_FILE',
+        type: Error,
+        message: 'Only regular files can be sent'
+      })(err);
+
+      stream.respond({ ':status': 404 });
+      stream.end();
+    },
+    statCheck: common.mustNotCall()
+  });
+});
+server.listen(0, () => {
+
+  const client = http2.connect(`http://localhost:${server.address().port}`);
+  const req = client.request();
+
+  req.on('response', common.mustCall((headers) => {
+    assert.strictEqual(headers[':status'], 404);
+  }));
+  req.on('data', common.mustNotCall());
+  req.on('end', common.mustCall(() => {
+    client.destroy();
+    server.close();
+  }));
+  req.end();
+});
diff --git a/test/parallel/test-http2-server-errors.js b/test/parallel/test-http2-server-errors.js
new file mode 100644
index 0000000000..9280d63edb
--- /dev/null
+++ b/test/parallel/test-http2-server-errors.js
@@ -0,0 +1,97 @@
+// Flags: --expose-http2 --expose-internals
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const h2 = require('http2');
+const { Http2Stream } = require('internal/http2/core');
+
+// Errors should not be reported both in Http2ServerRequest
+// and Http2ServerResponse
+
+{
+  let expected = null;
+
+  const server = h2.createServer();
+
+  server.on('stream', common.mustCall(function(stream) {
+    stream.on('error', common.mustCall(function(err) {
+      assert.strictEqual(err, expected);
+    }));
+
+    stream.resume();
+    stream.write('hello');
+
+    expected = new Error('kaboom');
+    stream.destroy(expected);
+    server.close();
+  }));
+
+  server.on('streamError', common.mustCall(function(err, stream) {
+    assert.strictEqual(err, expected);
+    assert.strictEqual(stream instanceof Http2Stream, true);
+  }));
+
+  server.listen(0, common.mustCall(function() {
+    const port = server.address().port;
+
+    const url = `http://localhost:${port}`;
+    const client = h2.connect(url, common.mustCall(function() {
+      const headers = {
+        ':path': '/foobar',
+        ':method': 'GET',
+        ':scheme': 'http',
+        ':authority': `localhost:${port}`,
+      };
+      const request = client.request(headers);
+      request.on('data', common.mustCall(function(chunk) {
+        // cause an error on the server side
+        client.destroy();
+      }));
+      request.end();
+    }));
+  }));
+}
+
+{
+  let expected = null;
+
+  const server = h2.createServer();
+
+  server.on('stream', common.mustCall(function(stream) {
+    // there is no 'error'  handler, and this will not crash
+    stream.write('hello');
+    stream.resume();
+
+    expected = new Error('kaboom');
+    stream.destroy(expected);
+    server.close();
+  }));
+
+  server.on('streamError', common.mustCall(function(err, stream) {
+    assert.strictEqual(err, expected);
+    assert.strictEqual(stream instanceof Http2Stream, true);
+  }));
+
+  server.listen(0, common.mustCall(function() {
+    const port = server.address().port;
+
+    const url = `http://localhost:${port}`;
+    const client = h2.connect(url, common.mustCall(function() {
+      const headers = {
+        ':path': '/foobar',
+        ':method': 'GET',
+        ':scheme': 'http',
+        ':authority': `localhost:${port}`,
+      };
+      const request = client.request(headers);
+      request.on('data', common.mustCall(function(chunk) {
+        // cause an error on the server side
+        client.destroy();
+      }));
+      request.end();
+    }));
+  }));
+}