From 8a4826164b2ad7b0e919621309126df459516957 Mon Sep 17 00:00:00 2001 From: Kumamon38 Date: Tue, 21 May 2019 18:34:08 +0200 Subject: [PATCH] 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 | 4 +- ixwebsocket/IXWebSocketTransport.cpp | 51 +++++++++++------ test/CMakeLists.txt | 2 +- test/IXWebSocketCloseTest.cpp | 84 ++++++++++++++++++++++------ test/cmd_websocket_chat.cpp | 4 +- 5 files changed, 107 insertions(+), 38 deletions(-) diff --git a/ixwebsocket/IXWebSocket.cpp b/ixwebsocket/IXWebSocket.cpp index 3b232fac..2ded8be3 100644 --- a/ixwebsocket/IXWebSocket.cpp +++ b/ixwebsocket/IXWebSocket.cpp @@ -293,8 +293,8 @@ namespace ix break; } - // We cannot enter poll which might block forever if we are stopping - if (_stop) break; + // We can avoid to poll if we want to stop and are not closing + if (_stop && !isClosing()) break; // 2. Poll to see if there's any new data available WebSocketTransport::PollResult pollResult = _ws.poll(); diff --git a/ixwebsocket/IXWebSocketTransport.cpp b/ixwebsocket/IXWebSocketTransport.cpp index 12199bfe..6d88a3ce 100644 --- a/ixwebsocket/IXWebSocketTransport.cpp +++ b/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,22 +1042,41 @@ 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 lock(_closeDataMutex); - _closeCode = code; - _closeReason = reason; - _closeWireSize = closeWireSize; - _closeRemote = remote; - } - { - std::lock_guard lock(_closingTimePointMutex); - _closingTimePoint = std::chrono::steady_clock::now(); - } - setReadyState(ReadyState::CLOSING); + { + std::lock_guard lock(_closeDataMutex); + _closeCode = code; + _closeReason = reason; + _closeWireSize = closeWireSize; + _closeRemote = remote; + } + { + std::lock_guard lock(_closingTimePointMutex); + _closingTimePoint = std::chrono::steady_clock::now(); + } + setReadyState(ReadyState::CLOSING); - // wake up the poll, but do not close yet - _socket->wakeUpFromPoll(Socket::kSendRequest); + sendCloseFrame(code, reason); + // wake up the poll, but do not close yet + _socket->wakeUpFromPoll(Socket::kSendRequest); + } + else + { + { + std::lock_guard 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 diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 7ed21a1c..ba622404 100644 --- a/test/CMakeLists.txt +++ b/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}) diff --git a/test/IXWebSocketCloseTest.cpp b/test/IXWebSocketCloseTest.cpp index ac286cda..0e3fe018 100644 --- a/test/IXWebSocketCloseTest.cpp +++ b/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 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,12 +203,14 @@ namespace } else if (messageType == ix::WebSocketMessageType::Close) { - log("Server closed connection"); - - //Logger() << closeInfo.code; - //Logger() << closeInfo.reason; - //Logger() << closeInfo.remote; - + std::stringstream ss; + ss << "Server closed connection(" + << closeInfo.code + << "," + << closeInfo.reason + << ")"; + log(ss.str()); + std::lock_guard lck(mutexWrite); receivedCloseCode = closeInfo.code; @@ -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 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(); + } +} diff --git a/test/cmd_websocket_chat.cpp b/test/cmd_websocket_chat.cpp index 2b68e529..313d54b7 100644 --- a/test/cmd_websocket_chat.cpp +++ b/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 - ix::msleep(500); + // Give us 1000ms for the server to notice that clients went away + ix::msleep(1000); REQUIRE(server.getClients().size() == 0); ix::reportWebSocketTraffic();