guard against double-connect attempts on socket (#150)

CHANGELOG.md

@@ -4,6 +4,10 @@ All notable changes to this project are documented below.
 
 The format is based on [keep a changelog](http://keepachangelog.com/) and this project uses [semantic versioning](http://semver.org/).
 
+### [2.8.3] - [2023-10-31]
+### Fixed
+- `NRtClient` will now guard against multiple connection attempts without disconnecting from the socket.
+
 ### [2.8.2] - [2023-10-31]
 ### Fixed
 - Fixed a race condition that could happen during socket connection which would cause `std::future_error` to be thrown.

core/core-rt/NRtClient.cpp

@@ -77,6 +77,11 @@ void NRtClient::setListener(NRtClientListenerInterface * listener)
 
 void NRtClient::connect(NSessionPtr session, bool createStatus, NRtClientProtocol protocol)
 {
+    if (_transport->isConnected() || _transport->isConnecting())
+    {
+        return;
+    }
+
     std::string url;
     NRtTransportType transportType;
 
@@ -109,15 +114,11 @@ void NRtClient::connect(NSessionPtr session, bool createStatus, NRtClientProtoco
 
 std::future<void> NRtClient::connectAsync(NSessionPtr session, bool createStatus, NRtClientProtocol protocol)
 {
-    try
-    {
-        // if old promise not ready, just complete it for the user.
-        _connectPromise->set_value();
-    }
-    catch(const std::future_error&)
+    if (_transport->isConnected() || _transport->isConnecting())
     {
-        // if we get an exception here, it means the connect promise has completed already from a previous connect.
-        // this is very expected. there is no way to check from the promise itself if it has completed already.
+        std::promise<void> emptyPromise = std::promise<void>();
+        emptyPromise.set_value();
+        return emptyPromise.get_future();
     }
 
     // stomp the old promise
@@ -135,6 +136,11 @@ std::future<void> NRtClient::connectAsync(NSessionPtr session, bool createStatus
     return _connectPromise->get_future();
 }
 
+bool NRtClient::isConnecting() const
+{
+    return _transport->isConnecting();
+}
+
 bool NRtClient::isConnected() const
 {
     return _transport->isConnected();
@@ -150,9 +156,9 @@ std::future<void> NRtClient::disconnectAsync()
 {
     // currently, disconnect callback is invoked immediately by client here, so we just return a completed future.
     disconnect();
-    std::promise<void> promise = std::promise<void>();
-    promise.set_value();
-    return promise.get_future();
+    std::promise<void> emptyPromise = std::promise<void>();
+    emptyPromise.set_value();
+    return emptyPromise.get_future();
 }
 
 void NRtClient::disconnect(const NRtClientDisconnectInfo& info)

core/core-rt/NRtClient.h

@@ -59,6 +59,7 @@ namespace Nakama {
         void connect(NSessionPtr session, bool createStatus, NRtClientProtocol protocol) override;
         std::future<void> connectAsync(NSessionPtr session, bool createStatus, NRtClientProtocol protocol) override;
 
+        bool isConnecting() const override;
         bool isConnected() const override;
 
         void disconnect() override;

impl/wsWslay/NWebsocketWslay.cpp

@@ -341,4 +341,9 @@ namespace Nakama {
             return;
         }
     }
+
+    bool NWebsocketWslay::isConnecting()
+    {
+        return _state == State::Connecting || _state == State::Handshake_Receiving || _state == State::Handshake_Sending;
+    }
 }

impl/wsWslay/NWebsocketWslay.h

@@ -46,6 +46,9 @@ class NWebsocketWslay : public NRtTransportInterface
     void disconnect() override;
     bool send(const NBytes& data) override;
 
+protected:
+    bool isConnecting() override;
+
 private:
     static ssize_t recv_callback(wslay_event_context_ptr ctx, uint8_t* data, size_t len, int flags, void* user_data);
     static ssize_t send_callback(wslay_event_context_ptr ctx, const uint8_t* data, size_t len, int flags, void* user_data);

interface/include/nakama-cpp/realtime/NRtClientInterface.h

@@ -151,6 +151,11 @@ NAKAMA_NAMESPACE_BEGIN
          */
         virtual std::future<void> connectAsync(NSessionPtr session, bool createStatus, NRtClientProtocol protocol = NRtClientProtocol::Protobuf) = 0;
 
+        /**
+         * @return True if connecting to server.
+         */
+        virtual bool isConnecting() const = 0;
+
         /**
          * @return True if connected to server.
          */

interface/include/nakama-cpp/realtime/NRtTransportInterface.h

@@ -87,6 +87,11 @@ NAKAMA_NAMESPACE_BEGIN
          */
         virtual bool isConnected() const { return _connected; }
 
+        /**
+         * @return True if connecting to server.
+         */
+        virtual bool isConnecting() = 0;
+
         /**
          * Close the connection with the server.
          *

test/README.md

@@ -6,7 +6,7 @@ If you are building for Mac/iOS, you'll need to set your NAKAMA_TEST_DEVELOPMENT
 
 In-tree example:
 ```
-cd example
+cd test
 cmake --list-presets
 cmake --preset <configure-preset>
 cmake --build build/<build-preset> --target install run

test/src/realtime/test_lifecycle.cpp

@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+#include <condition_variable>
 #include <thread>
 #include "NTest.h"
 #include "globals.h"
@@ -116,5 +117,58 @@ namespace Nakama {
             test.stopTest(connected);
 
         }
+
+        void test_rt_double_connect_async()
+        {
+             bool threadedTick = true;
+
+            NTest test(__func__, true);
+            test.runTest();
+
+            NSessionPtr session = test.client->authenticateCustomAsync(TestGuid::newGuid(), std::string(), true).get();
+
+            bool connected = false;
+
+            test.listener.setConnectCallback([&connected](){
+                connected = true;
+            });
+
+            // should not throw any errors.
+            test.rtClient->connectAsync(session, true).get();
+            test.rtClient->connectAsync(session, true).get();
+
+            test.stopTest(connected);
+        }
+
+        void test_rt_double_connect()
+        {
+            bool threadedTick = true;
+
+            NTest test(__func__, true);
+            test.runTest();
+
+            NSessionPtr session = test.client->authenticateCustomAsync(TestGuid::newGuid(), std::string(), true).get();
+
+            bool connected = false;
+            std::mutex mtx;
+            std::condition_variable cv;
+
+            test.listener.setConnectCallback([&](){
+                std::unique_lock<std::mutex> lock(mtx);
+                connected = true;
+                cv.notify_one();
+            });
+
+            // should not throw any errors.
+            test.rtClient->connect(session, true);
+            test.rtClient->connect(session, true);
+
+            {
+                std::unique_lock<std::mutex> lock(mtx);
+                cv.wait(lock, [&](){ return connected; }); // Wait until `connected` becomes true.
+            }
+
+            test.stopTest(connected);
+        }
     }
 }

test/src/realtime/test_realtime.cpp

@@ -35,6 +35,8 @@ void test_rt_quickdestroy();
 void test_rt_rapiddisconnect();
 void test_rt_reconnect();
 void test_rt_connect_callback();
+void test_rt_double_connect();
+void test_rt_double_connect_async();
 
 void run_realtime_tests()
 {
@@ -53,6 +55,9 @@ void test_realtime()
     // These tests are not protocol specific
     test_rt_rapiddisconnect();
     test_rt_connect_callback();
+    test_rt_double_connect();
+    test_rt_double_connect_async();
+
     /// change to 10 iterations to trigger https://github.com/microsoft/libHttpClient/issues/698 bug
     for (int i = 0; i < 1; i++) {
         test_rt_reconnect();

version.cmake

@@ -1,4 +1,4 @@
 # Easy to update by CI scripts file containing our version as well as
 # dependent git repos to achieve reproducible builds
 
-set(LIBNAKAMA_VERSION 2.8.2)
+set(LIBNAKAMA_VERSION 2.8.3)