Revert "http2: refactor error handling"

This reverts commit 4ca8ff2.

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

PR-URL: #15047
Reviewed-By: James M Snell <[email protected]>

Author: Trott

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}
-
-If an `ServerHttp2Stream` emits an `'error'` event, it will be forwarded here.
-The stream will already be destroyed when this event is triggered.
+The `'socketError'` event is emitted when a `'socketError'` event is emitted by
+an `Http2Session` associated with the server.
 
 #### 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
-  //
-  // errors in compatibility mode are
-  // not forwarded to the request
-  // and response objects. However,
-  // they are forwarded to 'clientError'
-  // on the server by Http2Stream
+  const request = this[kRequest];
+  request.emit('error', error);
 }
 
 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) {
-        err = new errors.Error('ERR_HTTP2_STREAM_ERROR', code);
+      if (code !== NGHTTP2_NO_ERROR) {
+        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) {
-      onError(err);
-    } else {
-      this.destroy(err);
-    }
+    process.nextTick(() => this.emit('error', err));
     return;
   }
-
   if (!stat.isFile()) {
     err = new errors.Error('ERR_HTTP2_SEND_FILE');
-    if (onError) {
-      onError(err);
-    } else {
-      this.destroy(err);
-    }
+    process.nextTick(() => this.emit('error', 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) {
-      onError(err);
-    } else {
-      this.destroy(err);
-    }
+    process.nextTick(() => this.emit('error', err));
     return;
   }
   state.fd = fd;
@@ -1713,20 +1691,13 @@ 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) {
     super(session, options);
     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) => {
-    common.expectsError({
-      type: Error,
-      message: 'test'
-    })(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));
 
   req.on('streamClosed', common.mustCall((code) => {
     assert.strictEqual(req.rstCode, NGHTTP2_INTERNAL_ERROR);