From 076a86d3da5b78aaa54ad96689680d8cb7fdfeca Mon Sep 17 00:00:00 2001
From: James M Snell <jasnell@gmail.com>
Date: Fri, 10 Jul 2020 13:48:45 -0700
Subject: [PATCH] quic: eliminate "ready"/"not ready" states for QuicSession

QuicClientSession and QuicServerSessions are now always immediately
ready for use when they are created, so no more need to track
ready state.

PR-URL: https://github.com/nodejs/node/pull/34283
Reviewed-By: Anna Henningsen <anna@addaleax.net>
---
 doc/api/quic.md                               |  10 --
 lib/internal/quic/core.js                     | 116 +++---------------
 test/parallel/test-quic-quicsession-resume.js |  29 +++--
 3 files changed, 35 insertions(+), 120 deletions(-)

diff --git a/doc/api/quic.md b/doc/api/quic.md
index cb3180e0cdf66a..671a4c681be3c1 100644
--- a/doc/api/quic.md
+++ b/doc/api/quic.md
@@ -1269,16 +1269,6 @@ empty object when the key exchange is not ephemeral. The supported types are
 
 For example: `{ type: 'ECDH', name: 'prime256v1', size: 256 }`.
 
-#### quicclientsession.ready
-<!-- YAML
-added: REPLACEME
--->
-
-* Type: {boolean}
-
-Set to `true` if the `QuicClientSession` is ready for use. False if the
-`QuicSocket` has not yet been bound.
-
 #### quicclientsession.setSocket(socket])
 <!-- YAML
 added: REPLACEME
diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js
index 9dffd0893b4fe4..633a27b1c034f0 100644
--- a/lib/internal/quic/core.js
+++ b/lib/internal/quic/core.js
@@ -212,7 +212,6 @@ const kBind = Symbol('kBind');
 const kClose = Symbol('kClose');
 const kCert = Symbol('kCert');
 const kClientHello = Symbol('kClientHello');
-const kContinueConnect = Symbol('kContinueConnect');
 const kCompleteListen = Symbol('kCompleteListen');
 const kDestroy = Symbol('kDestroy');
 const kEndpointBound = Symbol('kEndpointBound');
@@ -226,11 +225,9 @@ const kInternalServerState = Symbol('kInternalServerState');
 const kListen = Symbol('kListen');
 const kMakeStream = Symbol('kMakeStream');
 const kMaybeBind = Symbol('kMaybeBind');
-const kMaybeReady = Symbol('kMaybeReady');
 const kOnFileOpened = Symbol('kOnFileOpened');
 const kOnFileUnpipe = Symbol('kOnFileUnpipe');
 const kOnPipedFileHandleRead = Symbol('kOnPipedFileHandleRead');
-const kSocketReady = Symbol('kSocketReady');
 const kRemoveSession = Symbol('kRemove');
 const kRemoveStream = Symbol('kRemoveStream');
 const kServerBusy = Symbol('kServerBusy');
@@ -1148,9 +1145,6 @@ class QuicSocket extends EventEmitter {
 
       state.state = kSocketBound;
 
-      for (const session of state.sessions)
-        session[kSocketReady]();
-
       process.nextTick(() => {
         // User code may have run before this so we need to check the
         // destroyed state. If it has been destroyed, do nothing.
@@ -1175,13 +1169,6 @@ class QuicSocket extends EventEmitter {
       return;
     state.state = kSocketBound;
 
-    // Once the QuicSocket has been bound, we notify all currently
-    // existing QuicSessions. QuicSessions created after this
-    // point will automatically be notified that the QuicSocket
-    // is ready.
-    for (const session of state.sessions)
-      session[kSocketReady]();
-
     // The ready event indicates that the QuicSocket is ready to be
     // used to either listen or connect. No QuicServerSession should
     // exist before this event, and all QuicClientSession will remain
@@ -1380,16 +1367,7 @@ class QuicSocket extends EventEmitter {
     if (this.closing)
       throw new ERR_INVALID_STATE('QuicSocket is closing');
 
-    const session = new QuicClientSession(this, options);
-
-    try {
-      session[kContinueConnect](type, ip);
-    } catch (err) {
-      session.destroy();
-      throw err;
-    }
-
-    return session;
+    return new QuicClientSession(this, options, type, ip);
   }
 
   [kEndpointClose](endpoint, error) {
@@ -2207,10 +2185,7 @@ class QuicSession extends EventEmitter {
     // signaling the completion of the TLS handshake.
     const makeStream = QuicSession[kMakeStream].bind(this, stream, halfOpen);
     let deferred = false;
-    if (this.allowEarlyData && !this.ready) {
-      deferred = true;
-      this.once('ready', makeStream);
-    } else if (!this.handshakeComplete) {
+    if (!this.handshakeComplete) {
       deferred = true;
       this.once('secure', makeStream);
     }
@@ -2348,10 +2323,6 @@ class QuicServerSession extends QuicSession {
     } = options;
     super(socket, { highWaterMark, defaultEncoding });
     this[kSetHandle](handle);
-
-    // Both the handle and socket are immediately usable
-    // at this point so trigger the ready event.
-    this[kSocketReady]();
   }
 
   // Called only when a clientHello event handler is registered.
@@ -2388,12 +2359,6 @@ class QuicServerSession extends QuicSession {
       callback.bind(this[kHandle]));
   }
 
-  [kSocketReady]() {
-    process.nextTick(emit.bind(this, 'ready'));
-  }
-
-  get ready() { return true; }
-
   get allowEarlyData() { return false; }
 
   addContext(servername, context = {}) {
@@ -2418,7 +2383,6 @@ class QuicClientSession extends QuicSession {
     handshakeStarted: false,
     minDHSize: undefined,
     port: undefined,
-    ready: 0,
     remoteTransportParams: undefined,
     requestOCSP: undefined,
     secureContext: undefined,
@@ -2429,7 +2393,7 @@ class QuicClientSession extends QuicSession {
     qlogEnabled: false,
   };
 
-  constructor(socket, options) {
+  constructor(socket, options, type, ip) {
     const sc_options = {
       ...options,
       minVersion: 'TLSv1.3',
@@ -2486,33 +2450,8 @@ class QuicClientSession extends QuicSession {
       remoteTransportParams !== undefined &&
       sessionTicket !== undefined;
 
-    if (socket.bound)
-      this[kSocketReady]();
-  }
-
-  [kHandshakePost]() {
-    const { type, size } = this.ephemeralKeyInfo;
-    if (type === 'DH' && size < this[kInternalClientState].minDHSize) {
-      this.destroy(new ERR_TLS_DH_PARAM_SIZE(size));
-      return false;
-    }
-    return true;
-  }
-
-  [kCert](response) {
-    this.emit('OCSPResponse', response);
-  }
-
-  [kContinueConnect](type, ip) {
-    const state = this[kInternalClientState];
     setTransportParams(state.transportParams);
 
-    const options =
-      (state.verifyHostnameIdentity ?
-        QUICCLIENTSESSION_OPTION_VERIFY_HOSTNAME_IDENTITY : 0) |
-      (state.requestOCSP ?
-        QUICCLIENTSESSION_OPTION_REQUEST_OCSP : 0);
-
     const handle =
       _createClientSession(
         this.socket[kHandle],
@@ -2526,7 +2465,10 @@ class QuicClientSession extends QuicSession {
         state.dcid,
         state.preferredAddressPolicy,
         this.alpnProtocol,
-        options,
+        (state.verifyHostnameIdentity ?
+          QUICCLIENTSESSION_OPTION_VERIFY_HOSTNAME_IDENTITY : 0) |
+        (state.requestOCSP ?
+          QUICCLIENTSESSION_OPTION_REQUEST_OCSP : 0),
         state.qlogEnabled,
         state.autoStart);
 
@@ -2556,45 +2498,25 @@ class QuicClientSession extends QuicSession {
     }
 
     this[kSetHandle](handle);
-
-    // Listeners may have been added before the handle was created.
-    // Ensure that we toggle those listeners in the handle state.
-
-    const internalState = this[kInternalState];
-    if (this.listenerCount('keylog') > 0) {
-      toggleListeners(internalState.state, 'keylog', true);
-    }
-
-    if (this.listenerCount('pathValidation') > 0)
-      toggleListeners(internalState.state, 'pathValidation', true);
-
-    if (this.listenerCount('usePreferredAddress') > 0)
-      toggleListeners(internalState.state, 'usePreferredAddress', true);
-
-    this[kMaybeReady](0x2);
   }
 
-  [kSocketReady]() {
-    this[kMaybeReady](0x1);
+  [kHandshakePost]() {
+    const { type, size } = this.ephemeralKeyInfo;
+    if (type === 'DH' && size < this[kInternalClientState].minDHSize) {
+      this.destroy(new ERR_TLS_DH_PARAM_SIZE(size));
+      return false;
+    }
+    return true;
   }
 
-  // The QuicClientSession is ready for use only after
-  // (a) The QuicSocket has been bound and
-  // (b) The internal handle has been created
-  [kMaybeReady](flag) {
-    this[kInternalClientState].ready |= flag;
-    if (this.ready)
-      process.nextTick(emit.bind(this, 'ready'));
+  [kCert](response) {
+    this.emit('OCSPResponse', response);
   }
 
   get allowEarlyData() {
     return this[kInternalClientState].allowEarlyData;
   }
 
-  get ready() {
-    return this[kInternalClientState].ready === 0x3;
-  }
-
   get handshakeStarted() {
     return this[kInternalClientState].handshakeStarted;
   }
@@ -2608,11 +2530,7 @@ class QuicClientSession extends QuicSession {
     if (state.handshakeStarted)
       return;
     state.handshakeStarted = true;
-    if (!this.ready) {
-      this.once('ready', () => this[kHandle].startHandshake());
-    } else {
-      this[kHandle].startHandshake();
-    }
+    this[kHandle].startHandshake();
   }
 
   get ephemeralKeyInfo() {
diff --git a/test/parallel/test-quic-quicsession-resume.js b/test/parallel/test-quic-quicsession-resume.js
index 5acd0c16dd5893..75c42316750589 100644
--- a/test/parallel/test-quic-quicsession-resume.js
+++ b/test/parallel/test-quic-quicsession-resume.js
@@ -42,29 +42,36 @@ const countdown = new Countdown(2, () => {
 
   await server.listen();
 
+  let storedTicket;
+  let storedParams;
+
   const req = await client.connect({
     address: common.localhostIPv4,
     port: server.endpoints[0].address.port,
   });
 
-  const stream = req.openStream({ halfOpen: true });
-  stream.end('hello');
-  stream.resume();
-  stream.on('close', () => countdown.dec());
-
   req.on('sessionTicket', common.mustCall((ticket, params) => {
     assert(ticket instanceof Buffer);
     assert(params instanceof Buffer);
     debug('  Ticket: %s', ticket.toString('hex'));
     debug('  Params: %s', params.toString('hex'));
-
-    // Destroy this initial client session...
-    req.destroy();
-
-    // Wait a tick then start a new one.
-    setImmediate(newSession, ticket, params);
+    storedTicket = ticket;
+    storedParams = params;
   }, 1));
 
+  req.on('secure', () => {
+    const stream = req.openStream({ halfOpen: true });
+    stream.end('hello');
+    stream.resume();
+    stream.on('close', () => {
+      req.close();
+      countdown.dec();
+      // Wait a turn then start a new session using the stored
+      // ticket and transportParameters
+      setImmediate(newSession, storedTicket, storedParams);
+    });
+  });
+
   async function newSession(sessionTicket, remoteTransportParams) {
     const req = await client.connect({
       address: common.localhostIPv4,