http2: refinement and test for socketError

Fixes: nodejs/http2#184

Refines the `'socketError'` event a bit and adds a test for the
emission of the `'socketError'` event on the server. Client side
is tested separately

Backport-PR-URL: #14813
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Timothy Gu <timothygu99@gmail.com>

PR-URL: #14239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Author: jasnell

doc/api/http2.md

@@ -254,9 +254,9 @@ The `'socketError'` event is emitted when an `'error'` is emitted on the
 `Socket` instance bound to the `Http2Session`. If this event is not handled,
 the `'error'` event will be re-emitted on the `Socket`.
 
-Likewise, when an `'error'` event is emitted on the `Http2Session`, a
-`'sessionError'` event will be emitted on the `Socket`. If that event is
-not handled, the `'error'` event will be re-emitted on the `Http2Session`.
+For `ServerHttp2Session` instances, a `'socketError'` event listener is always
+registered that will, by default, forward the event on to the owning
+`Http2Server` instance if no additional handlers are registered.
 
 #### Event: 'timeout'
 <!-- YAML
@@ -1117,9 +1117,8 @@ an `Http2Session` object. If no listener is registered for this event, an
 added: REPLACEME
 -->
 
-The `'socketError'` event is emitted when an `'error'` event is emitted by
-a `Socket` associated with the server. If no listener is registered for this
-event, an `'error'` event is emitted.
+The `'socketError'` event is emitted when a `'socketError'` event is emitted by
+an `Http2Session` associated with the server.
 
 #### Event: 'stream'
 <!-- YAML
@@ -1181,9 +1180,8 @@ an `Http2Session` object. If no listener is registered for this event, an
 added: REPLACEME
 -->
 
-The `'socketError'` event is emitted when an `'error'` event is emitted by
-a `Socket` associated with the server. If no listener is registered for this
-event, an `'error'` event is emitted on the `Socket` instance instead.
+The `'socketError'` event is emitted when a `'socketError'` event is emitted by
+an `Http2Session` associated with the server.
 
 #### Event: 'unknownProtocol'
 <!-- YAML

lib/internal/http2/core.js

@@ -2032,17 +2032,15 @@ function sessionOnError(error) {
   this.emit('error', error);
 }
 
-// When a Socket emits an error, first try to forward it to the server
-// as a socketError; failing that, forward it to the session as a
+// When a Socket emits an error, forward it to the session as a
 // socketError; failing that, remove the listener and call destroy
 function socketOnError(error) {
   const type = this[kSession] && this[kSession][kType];
   debug(`[${sessionName(type)}] server socket error: ${error.message}`);
   if (kRenegTest.test(error.message))
     return this.destroy();
-  if (this[kServer] !== undefined && this[kServer].emit('socketError', error))
-    return;
-  if (this[kSession] !== undefined && this[kSession].emit('socketError', error))
+  if (this[kSession] !== undefined &&
+      this[kSession].emit('socketError', error, this))
     return;
   this.removeListener('error', socketOnError);
   this.destroy(error);
@@ -2076,6 +2074,11 @@ function sessionOnPriority(stream, parent, weight, exclusive) {
   this[kServer].emit('priority', stream, parent, weight, exclusive);
 }
 
+function sessionOnSocketError(error, socket) {
+  if (this.listenerCount('socketError') <= 1 && this[kServer] !== undefined)
+    this[kServer].emit('socketError', error, socket, this);
+}
+
 function connectionListener(socket) {
   debug('[server] received a connection');
   const options = this[kOptions] || {};
@@ -2109,6 +2112,7 @@ function connectionListener(socket) {
   session.on('error', sessionOnError);
   session.on('stream', sessionOnStream);
   session.on('priority', sessionOnPriority);
+  session.on('socketError', sessionOnSocketError);
 
   socket[kServer] = this;
 
@@ -2193,27 +2197,14 @@ function setupCompat(ev) {
   }
 }
 
-// If the socket emits an error, forward it to the session as a socketError;
-// failing that, remove the listener and destroy the socket
-function clientSocketOnError(error) {
-  const type = this[kSession] && this[kSession][kType];
-  debug(`[${sessionName(type)}] client socket error: ${error.message}`);
-  if (kRenegTest.test(error.message))
-    return this.destroy();
-  if (this[kSession] !== undefined && this[kSession].emit('socketError', error))
-    return;
-  this.removeListener('error', clientSocketOnError);
-  this.destroy(error);
-}
-
 // If the session emits an error, forward it to the socket as a sessionError;
 // failing that, destroy the session, remove the listener and re-emit the error
 function clientSessionOnError(error) {
   debug(`client session error: ${error.message}`);
   if (this[kSocket] !== undefined && this[kSocket].emit('sessionError', error))
     return;
   this.destroy();
-  this.removeListener('error', clientSocketOnError);
+  this.removeListener('error', socketOnError);
   this.removeListener('error', clientSessionOnError);
 }
 
@@ -2249,7 +2240,7 @@ function connect(authority, options, listener) {
       throw new errors.Error('ERR_HTTP2_UNSUPPORTED_PROTOCOL', protocol);
   }
 
-  socket.on('error', clientSocketOnError);
+  socket.on('error', socketOnError);
   socket.on('resume', socketOnResume);
   socket.on('pause', socketOnPause);
   socket.on('drain', socketOnDrain);

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

@@ -0,0 +1,52 @@
+// Flags: --expose-http2
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const http2 = require('http2');
+
+const server = http2.createServer();
+server.on('stream', common.mustCall((stream) => {
+  stream.respond();
+  stream.end('ok');
+}));
+server.on('session', common.mustCall((session) => {
+  // First, test that the socketError event is forwarded to the session object
+  // and not the server object.
+  const handler = common.mustCall((error, socket) => {
+    common.expectsError({
+      type: Error,
+      message: 'test'
+    })(error);
+    assert.strictEqual(socket, session.socket);
+  });
+  const isNotCalled = common.mustNotCall();
+  session.on('socketError', handler);
+  server.on('socketError', isNotCalled);
+  session.socket.emit('error', new Error('test'));
+  session.removeListener('socketError', handler);
+  server.removeListener('socketError', isNotCalled);
+
+  // Second, test that the socketError is forwarded to the server object when
+  // no socketError listener is registered for the session
+  server.on('socketError', common.mustCall((error, socket, session) => {
+    common.expectsError({
+      type: Error,
+      message: 'test'
+    })(error);
+    assert.strictEqual(socket, session.socket);
+    assert.strictEqual(session, session);
+  }));
+  session.socket.emit('error', new Error('test'));
+}));
+
+server.listen(0, common.mustCall(() => {
+  const client = http2.connect(`http://localhost:${server.address().port}`);
+  const req = client.request();
+  req.resume();
+  req.on('end', common.mustCall());
+  req.on('streamClosed', common.mustCall(() => {
+    client.destroy();
+    server.close();
+  }));
+}));