From 9308bc1f2d2abc6947531ef52ed3c1fc2da59119 Mon Sep 17 00:00:00 2001
From: Alex Potsides <alex@achingbrain.net>
Date: Fri, 27 Sep 2024 09:50:10 +0100
Subject: [PATCH] fix: make tcp closing error throwing consistent (#2729)

In the first close attempt we wrap the `closePromise` with a try/catch
and call `.abort` if it throws, and we don't propagate the error.

In the second invocation we return the `closePromise` which can reject
making the code non-deterministic.
---
 packages/transport-tcp/src/listener.ts       |  2 +
 packages/transport-tcp/src/socket-to-conn.ts | 47 +++++++++++---------
 2 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/packages/transport-tcp/src/listener.ts b/packages/transport-tcp/src/listener.ts
index 3f4808ea1d..ffc18bfb82 100644
--- a/packages/transport-tcp/src/listener.ts
+++ b/packages/transport-tcp/src/listener.ts
@@ -167,8 +167,10 @@ export class TCPListener extends TypedEventEmitter<ListenerEvents> implements Li
 
   private onSocket (socket: net.Socket): void {
     if (this.status.code !== TCPListenerStatusCode.ACTIVE) {
+      socket.destroy()
       throw new NotStartedError('Server is not listening yet')
     }
+
     // Avoid uncaught errors caused by unstable connections
     socket.on('error', err => {
       this.log('socket error', err)
diff --git a/packages/transport-tcp/src/socket-to-conn.ts b/packages/transport-tcp/src/socket-to-conn.ts
index 3c8f971cba..09dea95986 100644
--- a/packages/transport-tcp/src/socket-to-conn.ts
+++ b/packages/transport-tcp/src/socket-to-conn.ts
@@ -127,47 +127,48 @@ export const toMultiaddrConnection = (socket: Socket, options: ToConnectionOptio
     timeline: { open: Date.now() },
 
     async close (options: AbortOptions = {}) {
-      if (socket.destroyed) {
-        log('The %s socket is destroyed', lOptsStr)
+      if (socket.closed) {
+        log('The %s socket is already closed', lOptsStr)
         return
       }
 
-      if (closePromise != null) {
-        log('The %s socket is closed or closing', lOptsStr)
-        return closePromise
-      }
-
-      if (options.signal == null) {
-        const signal = AbortSignal.timeout(closeTimeout)
-
-        options = {
-          ...options,
-          signal
-        }
+      if (socket.destroyed) {
+        log('The %s socket is already destroyed', lOptsStr)
+        return
       }
 
       const abortSignalListener = (): void => {
         socket.destroy(new AbortError('Destroying socket after timeout'))
       }
 
-      options.signal?.addEventListener('abort', abortSignalListener)
-
       try {
+        if (closePromise != null) {
+          log('The %s socket is already closing', lOptsStr)
+          await closePromise
+          return
+        }
+
+        if (options.signal == null) {
+          const signal = AbortSignal.timeout(closeTimeout)
+
+          options = {
+            ...options,
+            signal
+          }
+        }
+
+        options.signal?.addEventListener('abort', abortSignalListener)
+
         log('%s closing socket', lOptsStr)
         closePromise = new Promise<void>((resolve, reject) => {
           socket.once('close', () => {
             // socket completely closed
             log('%s socket closed', lOptsStr)
-
             resolve()
           })
           socket.once('error', (err: Error) => {
             log('%s socket error', lOptsStr, err)
 
-            // error closing socket
-            if (maConn.timeline.close == null) {
-              maConn.timeline.close = Date.now()
-            }
             if (!socket.destroyed) {
               reject(err)
             }
@@ -210,6 +211,10 @@ export const toMultiaddrConnection = (socket: Socket, options: ToConnectionOptio
         socket.destroy(err)
       }
 
+      // closing a socket is always asynchronous (must wait for "close" event)
+      // but the tests expect this to be a synchronous operation so we have to
+      // set the close time here. the tests should be refactored to reflect
+      // reality.
       if (maConn.timeline.close == null) {
         maConn.timeline.close = Date.now()
       }