From edc71ef008cb8ac68b86da3a3df8d826aae6a249 Mon Sep 17 00:00:00 2001
From: James M Snell <jasnell@gmail.com>
Date: Tue, 7 Jul 2020 12:47:58 -0700
Subject: [PATCH] quic: handle errors thrown / rejections in the session event

Errors thrown within the session event handler will be handled
by destroying the session (allowing a proper connection close
to be sent to the client peer). They will not crash the parent
QuicSocket by default. Instead, a `'sessionError'` event will
be emitted, allowing the error to be logged or handled.

PR-URL: https://github.com/nodejs/node/pull/34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
---
 doc/api/quic.md                               | 52 ++++++++++++++++-
 lib/internal/quic/core.js                     | 23 +++++++-
 test/parallel/test-quic-client-server.js      |  1 +
 ...t-quic-server-session-event-error-async.js | 58 +++++++++++++++++++
 .../test-quic-server-session-event-error.js   | 58 +++++++++++++++++++
 5 files changed, 190 insertions(+), 2 deletions(-)
 create mode 100644 test/parallel/test-quic-server-session-event-error-async.js
 create mode 100644 test/parallel/test-quic-server-session-event-error.js

diff --git a/doc/api/quic.md b/doc/api/quic.md
index 3667a05b394956..6b0f9ae41bb12e 100644
--- a/doc/api/quic.md
+++ b/doc/api/quic.md
@@ -1385,10 +1385,60 @@ The `'ready'` event will not be emitted multiple times.
 added: REPLACEME
 -->
 
-Emitted when a new `QuicServerSession` has been created.
+Emitted when a new `QuicServerSession` has been created. The callback is
+invoked with a single argument providing the newly created `QuicServerSession`
+object.
+
+```js
+const { createQuicSocket } = require('net');
+
+const options = getOptionsSomehow();
+const server = createQuicSocket({ server: options });
+server.listen();
+
+server.on('session', (session) => {
+  // Attach session event listeners.
+});
+```
 
 The `'session'` event will be emitted multiple times.
 
+The `'session'` event handler *may* be an async function.
+
+If the `'session'` event handler throws an error, or if it returns a `Promise`
+that is rejected, the error will be handled by destroying the `QuicServerSession`
+automatically and emitting a `'sessionError'` event on the `QuicSocket`.
+
+#### Event: `'sessionError'`
+<!--YAML
+added: REPLACEME
+-->
+
+Emitted when an error occurs processing an event related to a specific
+`QuicSession` instance. The callback is invoked with two arguments:
+
+* `error` {Error} The error that was either thrown or rejected.
+* `session` {QuicSession} The `QuicSession` instance that was destroyed.
+
+The `QuicSession` instance will have been destroyed by the time the
+`'sessionError'` event is emitted.
+
+```js
+const { createQuicSocket } = require('net');
+
+const options = getOptionsSomehow();
+const server = createQuicSocket({ server: options });
+server.listen();
+
+server.on('session', (session) => {
+  throw new Error('boom');
+});
+
+server.on('sessionError', (error, session) => {
+  console.log('error:', error.message);
+});
+```
+
 #### quicsocket.addEndpoint(options)
 <!-- YAML
 added: REPLACEME
diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js
index 8c06d69494279d..1118b300abd1db 100644
--- a/lib/internal/quic/core.js
+++ b/lib/internal/quic/core.js
@@ -246,6 +246,8 @@ const kUsePreferredAddress = Symbol('kUsePreferredAddress');
 const kVersionNegotiation = Symbol('kVersionNegotiation');
 const kWriteGeneric = Symbol('kWriteGeneric');
 
+const kRejections = Symbol.for('nodejs.rejection');
+
 const kSocketUnbound = 0;
 const kSocketPending = 1;
 const kSocketBound = 2;
@@ -278,7 +280,11 @@ function onSessionReady(handle) {
       socket,
       handle,
       socket[kGetStreamOptions]());
-  process.nextTick(emit.bind(socket, 'session', session));
+  try {
+    socket.emit('session', session);
+  } catch (error) {
+    socket[kRejections](error, 'session', session);
+  }
 }
 
 // Called when the C++ QuicSession::Close() method has been called.
@@ -937,6 +943,21 @@ class QuicSocket extends EventEmitter {
     });
   }
 
+  [kRejections](err, eventname, ...args) {
+    switch (eventname) {
+      case 'session':
+        const session = args[0];
+        session.destroy(err);
+        process.nextTick(() => {
+          this.emit('sessionError', err, session);
+        });
+        return;
+      default:
+        // Fall through
+    }
+    this.destroy(err);
+  }
+
   // Returns the default QuicStream options for peer-initiated
   // streams. These are passed on to new client and server
   // QuicSession instances when they are created.
diff --git a/test/parallel/test-quic-client-server.js b/test/parallel/test-quic-client-server.js
index 90df11bbb77314..82d55410c0f4cf 100644
--- a/test/parallel/test-quic-client-server.js
+++ b/test/parallel/test-quic-client-server.js
@@ -58,6 +58,7 @@ server.listen({
   rejectUnauthorized: false,
   alpn: kALPN,
 });
+
 server.on('session', common.mustCall((session) => {
   debug('QuicServerSession Created');
 
diff --git a/test/parallel/test-quic-server-session-event-error-async.js b/test/parallel/test-quic-server-session-event-error-async.js
new file mode 100644
index 00000000000000..01c80688a3623f
--- /dev/null
+++ b/test/parallel/test-quic-server-session-event-error-async.js
@@ -0,0 +1,58 @@
+// Flags: --expose-internals --no-warnings
+'use strict';
+
+const common = require('../common');
+if (!common.hasQuic)
+  common.skip('missing quic');
+
+const { internalBinding } = require('internal/test/binding');
+const {
+  constants: {
+    NGTCP2_CONNECTION_REFUSED
+  }
+} = internalBinding('quic');
+
+const assert = require('assert');
+const {
+  key,
+  cert,
+  ca,
+} = require('../common/quic');
+
+const { createQuicSocket } = require('net');
+
+const options = { key, cert, ca, alpn: 'zzz' };
+
+const server = createQuicSocket({ server: options });
+
+server.on('session', common.mustCall(async (session) => {
+  session.on('close', common.mustCall());
+  session.on('error', common.mustCall((err) => {
+    assert.strictEqual(err.message, 'boom');
+  }));
+  // Throwing inside the session event handler should cause
+  // the session to be destroyed immediately. This should
+  // cause the client side to be closed also.
+  throw new Error('boom');
+}));
+
+server.on('sessionError', common.mustCall((err, session) => {
+  assert.strictEqual(err.message, 'boom');
+  assert(session.destroyed);
+}));
+
+server.listen();
+
+server.once('listening', common.mustCall(() => {
+  const client = createQuicSocket({ client: options });
+  const req = client.connect({
+    address: 'localhost',
+    port: server.endpoints[0].address.port
+  });
+  req.on('close', common.mustCall(() => {
+    assert.strictEqual(req.closeCode.code, NGTCP2_CONNECTION_REFUSED);
+    assert.strictEqual(req.closeCode.silent, true);
+    server.close();
+    client.close();
+  }));
+}));
diff --git a/test/parallel/test-quic-server-session-event-error.js b/test/parallel/test-quic-server-session-event-error.js
new file mode 100644
index 00000000000000..60f846cb5f1b32
--- /dev/null
+++ b/test/parallel/test-quic-server-session-event-error.js
@@ -0,0 +1,58 @@
+// Flags: --expose-internals --no-warnings
+'use strict';
+
+const common = require('../common');
+if (!common.hasQuic)
+  common.skip('missing quic');
+
+const { internalBinding } = require('internal/test/binding');
+const {
+  constants: {
+    NGTCP2_CONNECTION_REFUSED
+  }
+} = internalBinding('quic');
+
+const assert = require('assert');
+const {
+  key,
+  cert,
+  ca,
+} = require('../common/quic');
+
+const { createQuicSocket } = require('net');
+
+const options = { key, cert, ca, alpn: 'zzz' };
+
+const server = createQuicSocket({ server: options });
+
+server.on('session', common.mustCall((session) => {
+  session.on('close', common.mustCall());
+  session.on('error', common.mustCall((err) => {
+    assert.strictEqual(err.message, 'boom');
+  }));
+  // Throwing inside the session event handler should cause
+  // the session to be destroyed immediately. This should
+  // cause the client side to be closed also.
+  throw new Error('boom');
+}));
+
+server.on('sessionError', common.mustCall((err, session) => {
+  assert.strictEqual(err.message, 'boom');
+  assert(session.destroyed);
+}));
+
+server.listen();
+
+server.once('listening', common.mustCall(() => {
+  const client = createQuicSocket({ client: options });
+  const req = client.connect({
+    address: 'localhost',
+    port: server.endpoints[0].address.port
+  });
+  req.on('close', common.mustCall(() => {
+    assert.strictEqual(req.closeCode.code, NGTCP2_CONNECTION_REFUSED);
+    assert.strictEqual(req.closeCode.silent, true);
+    server.close();
+    client.close();
+  }));
+}));