fix close bug and tests : let poll do his job when closing (#79)

* let poll do his job when closing

* try fix test

* try fix test

* Update IXWebSocketTransport.cpp

* add log to find issue on CI

* add log to find issue on CI

* add log to find issue on CI

* add log to find issue on CI

* add log to find issue on CI

* change state immediately, and send close frame after

* add immediate close test

* disable test for windows

ixwebsocket/IXWebSocket.cpp

@@ -293,8 +293,8 @@ namespace ix
                 break;
             }
 
-            // We cannot enter poll which might block forever if we are stopping
+            // We can avoid to poll if we want to stop and are not closing
-            if (_stop) break;
+            if (_stop && !isClosing()) break;
 
             // 2. Poll to see if there's any new data available
             WebSocketTransport::PollResult pollResult = _ws.poll();

ixwebsocket/IXWebSocketTransport.cpp

@@ -71,7 +71,7 @@ namespace ix
     const int WebSocketTransport::kDefaultPingIntervalSecs(-1);
     const int WebSocketTransport::kDefaultPingTimeoutSecs(-1);
     const bool WebSocketTransport::kDefaultEnablePong(true);
-    const int WebSocketTransport::kClosingMaximumWaitingDelayInMs(200);
+    const int WebSocketTransport::kClosingMaximumWaitingDelayInMs(300);
     constexpr size_t WebSocketTransport::kChunkSize;
 
     WebSocketTransport::WebSocketTransport() :
@@ -748,7 +748,7 @@ namespace ix
         bool compress,
         const OnProgressCallback& onProgressCallback)
     {
-        if (_readyState != ReadyState::OPEN)
+        if (_readyState != ReadyState::OPEN && _readyState != ReadyState::CLOSING)
         {
             return WebSocketSendInfo();
         }
@@ -1042,7 +1042,9 @@ namespace ix
 
         if (_readyState == ReadyState::CLOSING || _readyState == ReadyState::CLOSED) return;
 
-        sendCloseFrame(code, reason);
+        // connection is opened, so close without sending close frame
+        if (_readyState == ReadyState::OPEN)
+        {
             {
                 std::lock_guard<std::mutex> lock(_closeDataMutex);
                 _closeCode = code;
@@ -1056,9 +1058,26 @@ namespace ix
             }
             setReadyState(ReadyState::CLOSING);
 
+            sendCloseFrame(code, reason);
             // wake up the poll, but do not close yet
             _socket->wakeUpFromPoll(Socket::kSendRequest);
         }
+        else
+        {
+            {
+                std::lock_guard<std::mutex> lock(_closeDataMutex);
+                _closeCode = code;
+                _closeReason = reason;
+                _closeWireSize = closeWireSize;
+                _closeRemote = remote;
+            }
+
+            setReadyState(ReadyState::CLOSED);
+
+            // wake up the poll, and close
+            _socket->wakeUpFromPoll(Socket::kCloseRequest);
+        }
+    }
 
     size_t WebSocketTransport::bufferedAmount() const
     {

test/CMakeLists.txt

@@ -44,13 +44,13 @@ if (UNIX)
   list(APPEND SOURCES
     IXDNSLookupTest.cpp
     cmd_websocket_chat.cpp
+    IXWebSocketCloseTest.cpp
   )
 endif()
 
 # Disable tests for now that are failing or not reliable
 # IXWebSocketPingTest.cpp
 # IXWebSocketPingTimeoutTest.cpp
-# IXWebSocketCloseTest.cpp
 # IXWebSocketMessageQTest.cpp (trigger a segfault on Linux)
 
 add_executable(ixwebsocket_unittest ${SOURCES})

test/IXWebSocketCloseTest.cpp

@@ -102,6 +102,7 @@ namespace
         }
 
         _webSocket.setUrl(url);
+        _webSocket.disableAutomaticReconnection();
 
         std::stringstream ss;
         log(std::string("Connecting to url: ") + url);
@@ -118,27 +119,27 @@ namespace
                 if (messageType == ix::WebSocketMessageType::Open)
                 {
                     log("client connected");
-
-                    _webSocket.disableAutomaticReconnection();
                 }
                 else if (messageType == ix::WebSocketMessageType::Close)
                 {
-                    log("client disconnected");
+                    std::stringstream ss;
+                    ss << "client disconnected("
+                       << closeInfo.code
+                       << ","
+                       << closeInfo.reason
+                       << ")";
+                    log(ss.str());
 
                     std::lock_guard<std::mutex> lck(_mutexCloseData);
 
                     _closeCode = closeInfo.code;
                     _closeReason = std::string(closeInfo.reason);
                     _closeRemote = closeInfo.remote;
-
-                    _webSocket.disableAutomaticReconnection();
                 }
                 else if (messageType == ix::WebSocketMessageType::Error)
                 {
                     ss << "Error ! " << error.reason;
                     log(ss.str());
-
-                    _webSocket.disableAutomaticReconnection();
                 }
                 else if (messageType == ix::WebSocketMessageType::Pong)
                 {
@@ -202,11 +203,13 @@ namespace
                         }
                         else if (messageType == ix::WebSocketMessageType::Close)
                         {
-                            log("Server closed connection");
+                            std::stringstream ss;
-
+                            ss << "Server closed connection("
-                            //Logger() << closeInfo.code;
+                               << closeInfo.code
-                            //Logger() << closeInfo.reason;
+                               << ","
-                            //Logger() << closeInfo.remote;
+                               << closeInfo.reason
+                               << ")";
+                            log(ss.str());
                             
                             std::lock_guard<std::mutex> lck(mutexWrite);
 
@@ -261,11 +264,11 @@ TEST_CASE("Websocket_client_close_default", "[close]")
 
         REQUIRE(server.getClients().size() == 1);
 
-        ix::msleep(100);
+        ix::msleep(500);
 
         webSocketClient.stop();
 
-        ix::msleep(200);
+        ix::msleep(500);
 
         // ensure client close is the same as values given
         REQUIRE(webSocketClient.getCloseCode() == 1000);
@@ -319,7 +322,7 @@ TEST_CASE("Websocket_client_close_params_given", "[close]")
 
         REQUIRE(server.getClients().size() == 1);
 
-        ix::msleep(100);
+        ix::msleep(500);
 
         webSocketClient.stop(4000, "My reason");
 
@@ -377,7 +380,7 @@ TEST_CASE("Websocket_server_close", "[close]")
 
         REQUIRE(server.getClients().size() == 1);
 
-        ix::msleep(200);
+        ix::msleep(500);
 
         server.stop();
 
@@ -404,3 +407,50 @@ TEST_CASE("Websocket_server_close", "[close]")
         ix::reportWebSocketTraffic();
     }
 }
+
+TEST_CASE("Websocket_server_close_immediatly", "[close]")
+{
+    SECTION("Make sure that close code and reason was read from server.")
+    {
+        ix::setupWebSocketTrafficTrackerCallback();
+
+        int port = getFreePort();
+        ix::WebSocketServer server(port);
+
+        uint16_t serverReceivedCloseCode(0);
+        bool serverReceivedCloseRemote(false);
+        std::string serverReceivedCloseReason("");
+        std::mutex mutexWrite;
+
+        REQUIRE(startServer(server, serverReceivedCloseCode, serverReceivedCloseReason, serverReceivedCloseRemote, mutexWrite));
+
+        std::string session = ix::generateSessionId();
+        WebSocketClient webSocketClient(port);
+
+        webSocketClient.start();
+
+        server.stop();
+
+        ix::msleep(500);
+
+        // ensure client close hasn't been called
+        REQUIRE(webSocketClient.getCloseCode() == 0);
+        REQUIRE(webSocketClient.getCloseReason() == "");
+        REQUIRE(webSocketClient.getCloseRemote() == false);
+
+        {
+            std::lock_guard<std::mutex> lck(mutexWrite);
+
+            // Here we ensure that the code/reason wasn't received by the server
+            REQUIRE(serverReceivedCloseCode == 0);
+            REQUIRE(serverReceivedCloseReason == "");
+            REQUIRE(serverReceivedCloseRemote == false);
+        }
+
+        // Give us 1000ms for the server to notice that clients went away
+        ix::msleep(1000);
+        REQUIRE(server.getClients().size() == 0);
+
+        ix::reportWebSocketTraffic();
+    }
+}

test/cmd_websocket_chat.cpp

@@ -336,8 +336,8 @@ TEST_CASE("Websocket_chat", "[websocket_chat]")
         REQUIRE(chatA.getReceivedMessages()[1] == "from B2");
         REQUIRE(chatA.getReceivedMessages()[2].size() == bigMessage.size());
 
-        // Give us 500ms for the server to notice that clients went away
+        // Give us 1000ms for the server to notice that clients went away
-        ix::msleep(500);
+        ix::msleep(1000);
         REQUIRE(server.getClients().size() == 0);
 
         ix::reportWebSocketTraffic();