Fix concurrent socket teardown (#6602)

Closes #6600.
Closes #6599.

In #6110 I added a check to prevent teardown from continuing
if there was a new connection attempt in between. While this solved
the reported issue (#6103), it also means that in case someone
calls connect or disconnect in between the original teardown call
and the waitForBufferDone, we would not actually close the original
connection, leaving dangling connections.

The fix is to always use the original connection that was supposed
to be closed in teardown and the subsequent checks and only set
this.conn to null when it is still the same object.

Co-authored-by: satoren <satoreyo@hotmail.com>

Author: SteffenDE

assets/js/phoenix/socket.js

@@ -496,17 +496,16 @@ export default class Socket {
     if(!this.conn){
       return callback && callback()
     }
-    let connectClock = this.connectClock
 
-    this.waitForBufferDone(() => {
-      if(connectClock !== this.connectClock){ return }
-      if(this.conn){
-        if(code){ this.conn.close(code, reason || "") } else { this.conn.close() }
-      }
+    // If someone calls connect before we finish tearing down,
+    // we create a new connection, but we still want to finish tearing down the old one.
+    const connToClose = this.conn
 
-      this.waitForSocketClosed(() => {
-        if(connectClock !== this.connectClock){ return }
-        if(this.conn){
+    this.waitForBufferDone(connToClose, () => {
+      if(code){ connToClose.close(code, reason || "") } else { connToClose.close() }
+
+      this.waitForSocketClosed(connToClose, () => {
+        if(this.conn === connToClose){
           this.conn.onopen = function (){ } // noop
           this.conn.onerror = function (){ } // noop
           this.conn.onmessage = function (){ } // noop
@@ -519,25 +518,25 @@ export default class Socket {
     })
   }
 
-  waitForBufferDone(callback, tries = 1){
-    if(tries === 5 || !this.conn || !this.conn.bufferedAmount){
+  waitForBufferDone(conn, callback, tries = 1){
+    if(tries === 5 || !conn.bufferedAmount){
       callback()
       return
     }
 
     setTimeout(() => {
-      this.waitForBufferDone(callback, tries + 1)
+      this.waitForBufferDone(conn, callback, tries + 1)
     }, 150 * tries)
   }
 
-  waitForSocketClosed(callback, tries = 1){
-    if(tries === 5 || !this.conn || this.conn.readyState === SOCKET_STATES.closed){
+  waitForSocketClosed(conn, callback, tries = 1){
+    if(tries === 5 || conn.readyState === SOCKET_STATES.closed){
       callback()
       return
     }
 
     setTimeout(() => {
-      this.waitForSocketClosed(callback, tries + 1)
+      this.waitForSocketClosed(conn, callback, tries + 1)
     }, 150 * tries)
   }
 

assets/test/socket_test.js

@@ -2,6 +2,7 @@ import {jest} from "@jest/globals"
 import {WebSocket, Server as WebSocketServer} from "mock-socket"
 import {encode} from "./serializer"
 import {Socket, LongPoll} from "../js/phoenix"
+import {SOCKET_STATES} from "../js/phoenix/constants"
 
 let socket
 
@@ -285,6 +286,130 @@ describe("with transports", function (){
         socket.disconnect()
       }).not.toThrow()
     })
+
+    it("properly tears down old connection when immediately reconnecting", function (){
+      const connections = []
+      const mockWebSocket = function StubWebSocketNoAutoClose(_url){
+        const conn = {
+          readyState: SOCKET_STATES.open,
+          get bufferedAmount(){ return 1 },
+          binaryType: "arraybuffer",
+          timeout: 20000,
+          onopen: null,
+          onerror: null,
+          onmessage: null,
+          onclose: null,
+          close(_code, _reason){
+            this.readyState = SOCKET_STATES.closing
+            setTimeout(() => {
+              this.readyState = SOCKET_STATES.closed
+            }, 1000)
+          },
+          send(){},
+        }
+        connections.push(conn)
+        return conn
+      }
+
+      jest.useFakeTimers()
+
+      socket = new Socket("/socket", {
+        heartbeatIntervalMs: 30000,
+        heartbeatTimeoutMs: 30000,
+        reconnectAfterMs: () => 10,
+        transport: mockWebSocket
+      })
+      socket.connect()
+      const originalConn = socket.conn
+
+      // Disconnect triggers teardown, which waits for bufferedAmount to be zero or 2250ms,
+      // then awaits SOCKET_STATES.closed before calling the callback.
+      const disconnected = jest.fn()
+      socket.disconnect(disconnected)
+
+      // For now, the conn is still set.
+      expect(socket.conn).toBeTruthy()
+
+      // Advance time by > 2250ms, which means we are waiting for socket to transition to closed
+      jest.advanceTimersByTime(3000)
+
+      // Now we call connect, while the teardown is still running
+      socket.connect()
+      // By now, waitForSocketClosed should be done, but now there's a new conn!
+      jest.advanceTimersByTime(3000)
+      expect(socket.conn).not.toBe(originalConn)
+
+      const openConns = connections.filter(c => c.readyState === SOCKET_STATES.open)
+      expect(openConns.length).toBe(1)
+
+      // Late teardown must not overwrite this.conn with null when it is already connB
+      expect(socket.conn).not.toBeNull()
+
+      // the original disconnected should have been called
+      expect(disconnected).toHaveBeenCalled()
+
+      jest.useRealTimers()
+    })
+
+    it("properly tears down old connection when disconnecting twice", function (){
+      const connections = []
+      const mockWebSocket = function StubWebSocketNoAutoClose(_url){
+        const conn = {
+          readyState: SOCKET_STATES.open,
+          get bufferedAmount(){ return 1 },
+          binaryType: "arraybuffer",
+          timeout: 20000,
+          onopen: null,
+          onerror: null,
+          onmessage: null,
+          onclose: null,
+          close(_code, _reason){
+            this.readyState = SOCKET_STATES.closing
+            setTimeout(() => {
+              this.readyState = SOCKET_STATES.closed
+            }, 1000)
+          },
+          send(){},
+        }
+        connections.push(conn)
+        return conn
+      }
+
+      jest.useFakeTimers()
+
+      socket = new Socket("/socket", {
+        heartbeatIntervalMs: 30000,
+        heartbeatTimeoutMs: 30000,
+        reconnectAfterMs: () => 10,
+        transport: mockWebSocket
+      })
+      socket.connect()
+
+      const disconnected = jest.fn()
+      socket.disconnect(disconnected)
+
+      // For now, the conn is still set.
+      expect(socket.conn).toBeTruthy()
+
+      // Advance time by > 2250ms, which means we are waiting for socket to transition to closed
+      jest.advanceTimersByTime(3000)
+
+      // Now we call disconnect again, while the teardown is still running
+      const disconnected2 = jest.fn()
+      socket.disconnect(disconnected2)
+
+      jest.advanceTimersByTime(10000)
+
+      const openConns = connections.filter(c => c.readyState === SOCKET_STATES.open)
+      expect(openConns.length).toBe(0)
+      expect(socket.conn).toBeNull()
+
+      // both disconnected functions should have been called
+      expect(disconnected).toHaveBeenCalled()
+      expect(disconnected2).toHaveBeenCalled()
+
+      jest.useRealTimers()
+    })
   })
 
   describe("connectionState", function (){