From d3a6d5e61e999bbe5f63d63cc5b4b43bde77f5e2 Mon Sep 17 00:00:00 2001
From: HDegroote <75906619+HDegroote@users.noreply.github.com>
Date: Wed, 9 Oct 2024 16:16:09 +0200
Subject: [PATCH] (WIP) clarify server state machine + clean up edge cases

---
 lib/server.js | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/lib/server.js b/lib/server.js
index 36c05c2c..de4c6da7 100644
--- a/lib/server.js
+++ b/lib/server.js
@@ -16,6 +16,32 @@ const { ALREADY_LISTENING, NODE_DESTROYED } = require('./errors')
 const HANDSHAKE_CLEAR_WAIT = 10000
 const HANDSHAKE_INITIAL_TIMEOUT = 10000
 
+// Developer info
+// While DHT Server is open (after listen finished, and before close is called)
+//   it listens for new connections and passes them on to the main DHT object which owns it
+// The _router object of the main DHT object provides the entrypoint
+//   for new connections, through its onpeerhandshake callback
+// The lifecycle of a new connection is:
+// 1) onpeerhandshake is called by the router
+// 2) A new handshake is performed for that peer if none exists
+//      (if one exists, the next steps are skipped)
+// 3) _addHandshake runs:
+//    a) Verify the remote is not firewalled
+//    b) Ask the main DHT object to create a new raw stream
+//    c) define callbacks to run when the connection is established (when it has a socket).
+//          Note: to that end, it (ab)uses the firewall to detect when it's no longer a relayed stream, but has a direct connection
+// 4) When a connection is established (onsocket callback of the raw stream):
+//    a) Create a Hyperswarm secret stream on top of the raw connection
+//    b) trigger the onconnection callback, informing the main DHT object of the connection
+//    => From here on, the DHT object is responsible for the connection's lifecycle
+// An important takeaway here is that there is a period of time during which a connection
+// is being established, but does not yet exist for the DHT itself.
+//
+// On top of opening/opened/closing/closed states,
+// the Server also has suspend logic, adding suspended, suspending and resuming
+//   states (which can overlap with the open/close states)
+// Being suspended means it has no active connections and does not accept
+//   new connections until resumed again
 module.exports = class Server extends EventEmitter {
   constructor (dht, opts = {}) {
     super()
@@ -178,6 +204,10 @@ module.exports = class Server extends EventEmitter {
     }
 
     if (this._closing) return
+
+    // In case we start listening while already
+    // TODO: pass the suspended state in when creating the announcer instead?
+    // That way it never creates any queries (needs changes in _announcer)
     if (this.suspended) await this._announcer.suspend()
 
     if (this._closing) return
@@ -230,6 +260,8 @@ module.exports = class Server extends EventEmitter {
 
     let remotePayload
     try {
+      // TODO: figure out why this is awaited
+      // (default handshake.recv is sync, which makes sense since noise is just a buffer)
       remotePayload = await handshake.recv(noise)
     } catch (err) {
       safetyCatch(err)
@@ -283,6 +315,13 @@ module.exports = class Server extends EventEmitter {
       hs.rawStream.on('error', autoDestroy)
 
       hs.onsocket = (socket, port, host) => {
+        if (this._closing || this.suspended) {
+          // TODO: which error to use? There's a SUSPENDED error.
+          // Add a CLOSED/CLOSING error too?
+          socket.destroy(new Error('TODO: correct error message'))
+          return
+        }
+
         if (hs.rawStream === null) return // Already hole punched
 
         this._clearLater(hs, id, k)
@@ -442,6 +481,11 @@ module.exports = class Server extends EventEmitter {
   }
 
   async _onpeerhandshake ({ noise, peerAddress }, req) {
+    // TODO: consider alternatives
+    // (an alternative is destroying the router before the servers in index.js/destroy.
+    //  which solves the _closing case and leaves only the suspended case.)
+    if (this._closing || this.suspended) return
+
     const k = b4a.toString(noise, 'hex')
 
     // The next couple of statements MUST run within the same tick to prevent