Fix #882. Don't automatically remove sockets from nsps since they might be used again

Author: nuclearace

CHANGELOG.md

@@ -3,6 +3,9 @@
 - Allow setting `SocketEngineSpec.extraHeaders` after init.
 - Deprecate `SocketEngineSpec.websocket` in favor of just using the `SocketEngineSpec.polling` property.
 - Enable bitcode for most platforms.
+- Fix [#882](https://github.com/socketio/socket.io-client-swift/issues/882). This adds a new method
+`SocketManger.removeSocket(_:)` that should be called if when you no longer wish to use a socket again.
+This will cause the engine to no longer keep a strong reference to the socket and no longer track it.
 
 # v13.0.1
 

Source/SocketIO/Client/SocketIOClient.swift

@@ -127,7 +127,14 @@ open class SocketIOClient : NSObject, SocketIOClientSpec {
 
         status = .connecting
 
-        manager.connectSocket(self)
+        joinNamespace()
+
+        if manager.status == .connected && nsp == "/" {
+            // We might not get a connect event for the default nsp, fire immediately
+            didConnect(toNamespace: nsp)
+
+            return
+        }
 
         guard timeoutAfter != 0 else { return }
 
@@ -183,7 +190,6 @@ open class SocketIOClient : NSObject, SocketIOClientSpec {
         DefaultSocketLogger.Logger.log("Closing socket", type: logType)
 
         leaveNamespace()
-        didDisconnect(reason: "Disconnect")
     }
 
     /// Send an event to the server, with optional data items.
@@ -366,21 +372,15 @@ open class SocketIOClient : NSObject, SocketIOClientSpec {
     /// Call when you wish to leave a namespace and disconnect this socket.
     @objc
     open func leaveNamespace() {
-        guard nsp != "/" else { return }
-
-        status = .disconnected
-
         manager?.disconnectSocket(self)
     }
 
     /// Joins `nsp`.
     @objc
     open func joinNamespace() {
-        guard nsp != "/" else { return }
-
         DefaultSocketLogger.Logger.log("Joining namespace \(nsp)", type: logType)
 
-        manager?.engine?.send("0\(nsp)", withData: [])
+        manager?.connectSocket(self)
     }
 
     /// Removes handler(s) for a client event.

Source/SocketIO/Manager/SocketManager.swift

@@ -233,9 +233,6 @@ open class SocketManager : NSObject, SocketManagerSpec, SocketParsable, SocketDa
     ///
     /// - parameter socket: The socket to disconnect.
     open func disconnectSocket(_ socket: SocketIOClient) {
-        // Make sure we remove socket from nsps
-        nsps.removeValue(forKey: socket.nsp)
-
         engine?.send("1\(socket.nsp)", withData: [])
         socket.didDisconnect(reason: "Namespace leave")
     }
@@ -424,6 +421,18 @@ open class SocketManager : NSObject, SocketManagerSpec, SocketParsable, SocketDa
         engine?.disconnect(reason: "manual reconnect")
     }
 
+    /// Removes the socket from the manager's control. One of the disconnect methods should be called before calling this
+    /// method.
+    ///
+    /// After calling this method the socket should no longer be considered usable.
+    ///
+    /// - parameter socket: The socket to remove.
+    /// - returns: The socket removed, if it was owned by the manager.
+    @discardableResult
+    open func removeSocket(_ socket: SocketIOClient) -> SocketIOClient? {
+        return nsps.removeValue(forKey: socket.nsp)
+    }
+
     private func tryReconnect(reason: String) {
         guard reconnecting else { return }
 

Source/SocketIO/Manager/SocketManagerSpec.swift

@@ -63,6 +63,9 @@ public protocol SocketManagerSpec : class, SocketEngineClient {
     /// called on.
     var handleQueue: DispatchQueue { get set }
 
+    /// The sockets in this manager indexed by namespace.
+    var nsps: [String: SocketIOClient] { get set }
+
     /// If `true`, this manager will try and reconnect on any disconnects.
     var reconnects: Bool { get set }
 
@@ -114,6 +117,14 @@ public protocol SocketManagerSpec : class, SocketEngineClient {
     /// This will cause a `disconnect` event to be emitted, as well as an `reconnectAttempt` event.
     func reconnect()
 
+    /// Removes the socket from the manager's control.
+    /// After calling this method the socket should no longer be considered usable.
+    ///
+    /// - parameter socket: The socket to remove.
+    /// - returns: The socket removed, if it was owned by the manager.
+    @discardableResult
+    func removeSocket(_ socket: SocketIOClient) -> SocketIOClient?
+
     /// Returns a `SocketIOClient` for the given namespace. This socket shares a transport with the manager.
     ///
     /// Calling multiple times returns the same socket.

Tests/TestSocketIO/SocketMangerTest.swift

@@ -88,6 +88,14 @@ class SocketMangerTest : XCTestCase {
         XCTAssertEqual(manager.reconnectWait, 5)
     }
 
+    func testManagerRemovesSocket() {
+        setUpSockets()
+
+        manager.removeSocket(socket)
+
+        XCTAssertNil(manager.nsps[socket.nsp])
+    }
+
     private func setUpSockets() {
         socket = manager.testSocket(forNamespace: "/")
         socket2 = manager.testSocket(forNamespace: "/swift")

Tests/TestSocketIO/SocketSideEffectTest.swift

@@ -250,6 +250,22 @@ class SocketSideEffectTest: XCTestCase {
         waitForExpectations(timeout: 0.8)
     }
 
+    func testConnectCallsConnectEventImmediatelyIfManagerAlreadyConnected() {
+        let expect = expectation(description: "The client should call the connect handler")
+
+        socket = manager.defaultSocket
+
+        socket.setTestStatus(.notConnected)
+        manager.setTestStatus(.connected)
+
+        socket.on(clientEvent: .connect) {data, ack in
+            expect.fulfill()
+        }
+        socket.connect(timeoutAfter: 0.3, withHandler: nil)
+
+        waitForExpectations(timeout: 0.8)
+    }
+
     func testConnectDoesNotTimeOutIfConnected() {
         let expect = expectation(description: "The client should not call the timeout function")
 

Tests/TestSocketIOObjc/ManagerObjectiveCTest.m

@@ -106,6 +106,14 @@ - (void)testManagerEmitAll {
     [self waitForExpectationsWithTimeout:0.3 handler:nil];
 }
 
+- (void)testMangerRemoveSocket {
+    [self setUpSockets];
+
+    [self.manager removeSocket:self.socket];
+
+    XCTAssertNil(self.manager.nsps[self.socket.nsp]);
+}
+
 - (void)setUpSockets {
     self.socket = [self.manager testSocketForNamespace:@"/"];
     self.socket2 = [self.manager testSocketForNamespace:@"/swift"];