Revert "http2: refactor error handling"

This reverts commit 4ca8ff264f368c301827e07956f313cebd1b8de8.

That commit was landed without a green CI and is failing on Windows.

PR-URL: https://github.com/nodejs/node/pull/15047
Reviewed-By: James M Snell <jasnell@gmail.com>

doc/api/http2.md

@@ -1118,8 +1118,6 @@ 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
@@ -1148,16 +1146,6 @@ 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 });
@@ -1190,10 +1178,6 @@ 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
@@ -1224,19 +1208,6 @@ 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
@@ -1246,15 +1217,13 @@ 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: 'streamError'
+#### Event: 'socketError'
 <!-- YAML
-added: REPLACEME
+added: v8.4.0
 -->
 
-* `socket` {http2.ServerHttp2Stream}
+The `'socketError'` event is emitted when a `'socketError'` event is emitted by
-
+an `Http2Session` associated with the server.
-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

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,
 };

lib/internal/http2/compat.js

@@ -58,13 +58,8 @@ function onStreamEnd() {
 }
 
 function onStreamError(error) {
-  // this is purposefully left blank
+  const request = this[kRequest];
-  //
+  request.emit('error', error);
-  // 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() {
@@ -87,6 +82,11 @@ 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,7 +271,9 @@ 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);

lib/internal/http2/core.js

@@ -1506,13 +1506,6 @@ 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]}`);
 
@@ -1536,8 +1529,9 @@ class Http2Stream extends Duplex {
       // All done
       const rst = this[kState].rst;
       const code = rst ? this[kState].rstCode : NGHTTP2_NO_ERROR;
-      if (!err && code !== NGHTTP2_NO_ERROR) {
+      if (code !== NGHTTP2_NO_ERROR) {
-        err = new errors.Error('ERR_HTTP2_STREAM_ERROR', code);
+        const err = new errors.Error('ERR_HTTP2_STREAM_ERROR', code);
+        process.nextTick(() => this.emit('error', err));
       }
       process.nextTick(emit.bind(this, 'streamClosed', code));
       debug(`[${sessionName(session[kType])}] stream ${this[kID]} destroyed`);
@@ -1640,24 +1634,13 @@ function doSendFileFD(session, options, fd, headers, getTrailers, err, stat) {
     abort(this);
     return;
   }
-  const onError = options.onError;
-
   if (err) {
-    if (onError) {
+    process.nextTick(() => this.emit('error', err));
-      onError(err);
-    } else {
-      this.destroy(err);
-    }
     return;
   }
-
   if (!stat.isFile()) {
     err = new errors.Error('ERR_HTTP2_SEND_FILE');
-    if (onError) {
+    process.nextTick(() => this.emit('error', err));
-      onError(err);
-    } else {
-      this.destroy(err);
-    }
     return;
   }
 
@@ -1694,17 +1677,12 @@ 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) {
-    if (onError) {
+    process.nextTick(() => this.emit('error', err));
-      onError(err);
-    } else {
-      this.destroy(err);
-    }
     return;
   }
   state.fd = fd;
@@ -1713,12 +1691,6 @@ 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) {
@@ -1726,7 +1698,6 @@ 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`);
   }
 
@@ -2585,8 +2556,6 @@ module.exports = {
   createServer,
   createSecureServer,
   connect,
-  Http2Session,
-  Http2Stream,
   Http2ServerRequest,
   Http2ServerResponse
 };

test/parallel/test-http2-client-stream-destroy-before-connect.js

@@ -36,11 +36,18 @@ 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'
-    })(err);
+      });
-  }));
+    fn(err);
+  }, 2));
 
   req.on('streamClosed', common.mustCall((code) => {
     assert.strictEqual(req.rstCode, NGHTTP2_INTERNAL_ERROR);

test/parallel/test-http2-compat-errors.js

@@ -1,52 +0,0 @@
-// 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();
-  }));
-}));

test/parallel/test-http2-respond-file-404.js

@@ -1,47 +0,0 @@
-// 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();
-});

test/parallel/test-http2-respond-file-error-dir.js

@@ -1,46 +0,0 @@
-// 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();
-});

test/parallel/test-http2-server-errors.js

@@ -1,97 +0,0 @@
-// 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();
-    }));
-  }));
-}