From cc89aac5f7d5443e0be9d037f067391ab144244e Mon Sep 17 00:00:00 2001
From: James M Snell <jasnell@gmail.com>
Date: Tue, 7 Jul 2020 13:42:25 -0700
Subject: [PATCH] quic: refactor/improve error handling for busy event

Also, change setServerBusy into a setter

PR-URL: https://github.com/nodejs/node/pull/34247
Reviewed-By: Anna Henningsen <anna@addaleax.net>
---
 doc/api/quic.md                               | 33 +++++++++----------
 lib/internal/quic/core.js                     | 17 +++++++---
 ...test-quic-server-busy-event-error-async.js | 32 ++++++++++++++++++
 .../test-quic-server-busy-event-error.js      | 32 ++++++++++++++++++
 4 files changed, 93 insertions(+), 21 deletions(-)
 create mode 100644 test/parallel/test-quic-server-busy-event-error-async.js
 create mode 100644 test/parallel/test-quic-server-busy-event-error.js

diff --git a/doc/api/quic.md b/doc/api/quic.md
index 6b0f9ae41bb12e..a847ac621aa268 100644
--- a/doc/api/quic.md
+++ b/doc/api/quic.md
@@ -1330,8 +1330,8 @@ added: REPLACEME
 -->
 
 Emitted when the server busy state has been toggled using
-`quicSocket.setServerBusy()`. The callback is invoked with a single
-boolean argument indicating `true` if busy status is enabled,
+`quicSocket.serverBusy = true | false`. The callback is invoked with a
+single boolean argument indicating `true` if busy status is enabled,
 `false` otherwise. This event is strictly informational.
 
 ```js
@@ -1346,8 +1346,8 @@ socket.on('busy', (busy) => {
     console.log('Server is not busy');
 });
 
-socket.setServerBusy(true);
-socket.setServerBusy(false);
+socket.serverBusy = true;
+socket.serverBusy = false;
 ```
 
 This `'busy'` event may be emitted multiple times.
@@ -1874,6 +1874,18 @@ Set to `true` if the socket is not yet bound to the local UDP port.
 added: REPLACEME
 -->
 
+#### quicsocket.serverBusy
+<!-- YAML
+added: REPLACEME
+-->
+
+* Type: {boolean} When `true`, the `QuicSocket` will reject new connections.
+
+Setting `quicsocket.serverBusy` to `true` will tell the `QuicSocket`
+to reject all new incoming connection requests using the `SERVER_BUSY` QUIC
+error code. To begin receiving connections again, disable busy mode by setting
+`quicsocket.serverBusy = false`.
+
 #### quicsocket.serverBusyCount
 <!-- YAML
 added: REPLACEME
@@ -1911,19 +1923,6 @@ by artificially dropping received or transmitted packets.
 
 This method is *not* to be used in production applications.
 
-#### quicsocket.setServerBusy(\[on\])
-<!-- YAML
-added: REPLACEME
--->
-
-* `on` {boolean} When `true`, the `QuicSocket` will reject new connections.
-  **Defaults**: `true`.
-
-Calling `setServerBusy()` or `setServerBusy(true)` will tell the `QuicSocket`
-to reject all new incoming connection requests using the `SERVER_BUSY` QUIC
-error code. To begin receiving connections again, disable busy mode by calling
-`setServerBusy(false)`.
-
 #### quicsocket.statelessResetCount
 <!-- YAML
 added: REPLACEME
diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js
index 1118b300abd1db..d70b99378a24a4 100644
--- a/lib/internal/quic/core.js
+++ b/lib/internal/quic/core.js
@@ -851,7 +851,7 @@ class QuicSocket extends EventEmitter {
     highWaterMark: undefined,
     lookup: undefined,
     server: undefined,
-    serverBusy: undefined,
+    serverBusy: false,
     serverListening: false,
     serverSecureContext: undefined,
     sessions: new Set(),
@@ -1075,7 +1075,16 @@ class QuicSocket extends EventEmitter {
   // Called by the C++ internals to notify when server busy status is toggled.
   [kServerBusy](on) {
     this[kInternalState].serverBusy = on;
-    process.nextTick(emit.bind(this, 'busy', on));
+    // In a nextTick because the event ends up being
+    // emitted synchronously when quicSocket.serverBusy
+    // is called.
+    process.nextTick(() => {
+      try {
+        this.emit('busy', on);
+      } catch (error) {
+        this[kRejections](error, 'busy', on);
+      }
+    });
   }
 
   addEndpoint(options = {}) {
@@ -1490,10 +1499,10 @@ class QuicSocket extends EventEmitter {
 
   // Marking a server as busy will cause all new
   // connection attempts to fail with a SERVER_BUSY CONNECTION_CLOSE.
-  setServerBusy(on = true) {
+  set serverBusy(on) {
     const state = this[kInternalState];
     if (state.state === kSocketDestroyed)
-      throw new ERR_QUICSOCKET_DESTROYED('setServerBusy');
+      throw new ERR_QUICSOCKET_DESTROYED('serverBusy');
     validateBoolean(on, 'on');
     if (state.serverBusy !== on)
       this[kHandle].setServerBusy(on);
diff --git a/test/parallel/test-quic-server-busy-event-error-async.js b/test/parallel/test-quic-server-busy-event-error-async.js
new file mode 100644
index 00000000000000..7e8f86b68988d0
--- /dev/null
+++ b/test/parallel/test-quic-server-busy-event-error-async.js
@@ -0,0 +1,32 @@
+// Flags: --no-warnings
+'use strict';
+
+const common = require('../common');
+if (!common.hasQuic)
+  common.skip('missing 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('busy', common.mustCall(async () => {
+  throw new Error('boom');
+}));
+
+server.on('close', common.mustCall());
+
+server.on('error', common.mustCall((err) => {
+  assert.strictEqual(err.message, 'boom');
+}));
+
+assert.strictEqual(server.serverBusy, false);
+server.serverBusy = true;
diff --git a/test/parallel/test-quic-server-busy-event-error.js b/test/parallel/test-quic-server-busy-event-error.js
new file mode 100644
index 00000000000000..d71e9d25e70ca8
--- /dev/null
+++ b/test/parallel/test-quic-server-busy-event-error.js
@@ -0,0 +1,32 @@
+// Flags: --no-warnings
+'use strict';
+
+const common = require('../common');
+if (!common.hasQuic)
+  common.skip('missing 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('busy', common.mustCall(() => {
+  throw new Error('boom');
+}));
+
+server.on('close', common.mustCall());
+
+server.on('error', common.mustCall((err) => {
+  assert.strictEqual(err.message, 'boom');
+}));
+
+assert.strictEqual(server.serverBusy, false);
+server.serverBusy = true;