From 105e6eac3ca803df0093ad3484eafe40444e3c05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sven=20G=C3=B6thel?= Date: Fri, 25 Oct 2024 11:48:57 +0200 Subject: [PATCH] Remove disabled `StreamSocket::checkRemoval` criteria `MinBytesPerSec` minimum throughput MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #9916 added `MinBytesPerSec` minimum throughput criteria for checkRemoval to limit connections, i.e. cleaning up unresponsiveness or lagging sockets. Due to its unreliable nature per request, the criteria has been disabled per default but remained in the code test w/ UT UnitTimeoutSocket. Turns out that even the UT may fail on certain test setups. We decided to remove this criteria altogether, as it will nor be used. Signed-off-by: Sven Göthel Change-Id: I9212cdf8b87c1c7b0df5761db956cbaa4fd44f58 --- net/NetUtil.hpp | 3 - net/Socket.cpp | 11 +- net/Socket.hpp | 3 - test/Makefile.am | 3 - test/UnitTimeoutConnections.cpp | 1 - test/UnitTimeoutNone.cpp | 1 - test/UnitTimeoutSocket.cpp | 192 -------------------------------- test/UnitTimeoutWSPing.cpp | 1 - wsd/COOLWSD.cpp | 1 - 9 files changed, 2 insertions(+), 214 deletions(-) delete mode 100644 test/UnitTimeoutSocket.cpp diff --git a/net/NetUtil.hpp b/net/NetUtil.hpp index f2a873f765e8..62458ae181c0 100644 --- a/net/NetUtil.hpp +++ b/net/NetUtil.hpp @@ -41,8 +41,6 @@ class Defaults /// Maximum total connections (9999 or MAX_CONNECTIONS). Zero disables metric. size_t MaxConnections; - /// Socket minimum bits per seconds throughput (0). Zero disables metric. - double MinBytesPerSec; /// Socket poll timeout in us (64s), useful to increase for debugging. std::chrono::microseconds SocketPollTimeout; @@ -53,7 +51,6 @@ class Defaults , WSPingPeriod(std::chrono::microseconds(3000000)) , HTTPTimeout(std::chrono::microseconds(30000000)) , MaxConnections(9999) - , MinBytesPerSec(0.0) , SocketPollTimeout(std::chrono::microseconds(64000000)) { } diff --git a/net/Socket.cpp b/net/Socket.cpp index 762939551ab9..b03f7ac3822d 100644 --- a/net/Socket.cpp +++ b/net/Socket.cpp @@ -1502,24 +1502,17 @@ bool StreamSocket::checkRemoval(std::chrono::steady_clock::time_point now) if( isIPType() ) { // Forced removal on outside-facing IPv[46] network connections only - const auto durTotal = - std::chrono::duration_cast(now - getCreationTime()); const auto durLast = std::chrono::duration_cast(now - getLastSeenTime()); - const double bytesPerSecIn = durTotal.count() > 0 ? (double)bytesRcvd() / ((double)durTotal.count() / 1000.0) : 0.0; /// TO Criteria: Violate maximum idle (_pollTimeout default 64s) const bool isIDLE = _pollTimeout > std::chrono::microseconds::zero() && durLast > _pollTimeout; - /// TO Criteria: Violate minimum bytes-per-sec throughput? (_minBytesPerSec default 0, disabled) - const bool isMinThroughput = _minBytesPerSec > std::numeric_limits::epsilon() && - bytesPerSecIn > std::numeric_limits::epsilon() && - bytesPerSecIn < _minBytesPerSec; /// TO Criteria: Shall terminate? const bool isTermination = SigUtil::getTerminationFlag(); - if (isIDLE || isMinThroughput || isTermination ) + if (isIDLE || isTermination ) { LOG_WRN("CheckRemoval: Timeout: {IDLE " << isIDLE - << ", MinThroughput " << isMinThroughput << ", Termination " << isTermination << "}, " + << ", Termination " << isTermination << "}, " << getStatsString(now) << ", " << *this); if (_socketHandler) diff --git a/net/Socket.hpp b/net/Socket.hpp index 16d2e48696fe..95de312c074d 100644 --- a/net/Socket.hpp +++ b/net/Socket.hpp @@ -1069,7 +1069,6 @@ class StreamSocket : public Socket, Socket(fd, type, creationTime), _pollTimeout( net::Defaults::get().SocketPollTimeout ), _httpTimeout( net::Defaults::get().HTTPTimeout ), - _minBytesPerSec( net::Defaults::get().MinBytesPerSec ), _hostname(std::move(host)), _wsState(WSState::HTTP), _isLocalHost(hostType == LocalHost), @@ -1739,8 +1738,6 @@ class StreamSocket : public Socket, const std::chrono::microseconds _pollTimeout; /// defaults to 30s, see net::Defaults::HTTPTimeout const std::chrono::microseconds _httpTimeout; - /// defaults to 0 (disabled), see net::Defaults::MinBytesPerSec - const double _minBytesPerSec; /// The hostname (or IP) of the peer we are connecting to. const std::string _hostname; diff --git a/test/Makefile.am b/test/Makefile.am index d5a779f64a75..f3339adf7fe8 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -91,7 +91,6 @@ all_la_unit_tests = \ unit-join-disconnect.la \ unit-initial-load-fail.la \ unit-timeout.la \ - unit-timeout_socket.la \ unit-timeout_wsping.la \ unit-timeout_conn.la \ unit-timeout_none.la \ @@ -227,8 +226,6 @@ unit_join_disconnect_la_SOURCES = UnitJoinDisconnect.cpp unit_join_disconnect_la_LIBADD = $(CPPUNIT_LIBS) unit_timeout_la_SOURCES = UnitTimeout.cpp unit_timeout_la_LIBADD = $(CPPUNIT_LIBS) -unit_timeout_socket_la_SOURCES = UnitTimeoutSocket.cpp -unit_timeout_socket_la_LIBADD = $(CPPUNIT_LIBS) unit_timeout_wsping_la_SOURCES = UnitTimeoutWSPing.cpp unit_timeout_wsping_la_LIBADD = $(CPPUNIT_LIBS) unit_timeout_conn_la_SOURCES = UnitTimeoutConnections.cpp diff --git a/test/UnitTimeoutConnections.cpp b/test/UnitTimeoutConnections.cpp index c55fb62b2555..5d6f1839ce8b 100644 --- a/test/UnitTimeoutConnections.cpp +++ b/test/UnitTimeoutConnections.cpp @@ -39,7 +39,6 @@ class UnitTimeoutConnections : public UnitTimeoutBase1 // defaults.HTTPTimeout = std::chrono::microseconds(30000000); // defaults.MaxConnections = 9999; defaults.MaxConnections = ConnectionLimit; - // defaults.MinBytesPerSec = 0.0; // defaults.SocketPollTimeout = std::chrono::microseconds(64000000); } diff --git a/test/UnitTimeoutNone.cpp b/test/UnitTimeoutNone.cpp index 8b02cc4f93ae..8ce994e75b52 100644 --- a/test/UnitTimeoutNone.cpp +++ b/test/UnitTimeoutNone.cpp @@ -39,7 +39,6 @@ class UnitTimeoutNone : public UnitTimeoutBase1 // defaults.HTTPTimeout = std::chrono::microseconds(30000000); // defaults.MaxConnections = 9999; // defaults.MaxConnections = ConnectionLimit; - // defaults.MinBytesPerSec = 0.0; // defaults.SocketPollTimeout = std::chrono::microseconds(64000000); } diff --git a/test/UnitTimeoutSocket.cpp b/test/UnitTimeoutSocket.cpp deleted file mode 100644 index 01ed3a69d19b..000000000000 --- a/test/UnitTimeoutSocket.cpp +++ /dev/null @@ -1,192 +0,0 @@ -/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */ -/* - * Copyright the Collabora Online contributors. - * - * SPDX-License-Identifier: MPL-2.0 - * - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. - */ - -#include - -#include -#include - -#include -#include - -#include -#include - -#include -#include -#include -#include -#include - -#include "UnitTimeoutBase.hpp" - -/// Test suite class for Socket max-duration timeout limit using HTTP and WS sessions. -class UnitTimeoutSocket : public UnitTimeoutBase0 -{ - TestResult testHttp(); - TestResult testWSPing(); - TestResult testWSDChatPing(); - - void configNet(net::Defaults& defaults) override - { - // defaults.WSPingTimeout = std::chrono::microseconds(2000000); - // defaults.WSPingPeriod = std::chrono::microseconds(3000000); - // defaults.HTTPTimeout = std::chrono::microseconds(30000000); - // defaults.HTTPTimeout = std::chrono::microseconds(1); - // defaults.MaxConnections = 9999; - // defaults.MinBytesPerSec = 0.0; - defaults.MinBytesPerSec = 100000.0; // 100kBps - // defaults.SocketPollTimeout = std::chrono::microseconds(64000000); - } - -public: - UnitTimeoutSocket() - : UnitTimeoutBase0("UnitTimeoutSocket") - { - } - - void invokeWSDTest() override; -}; - -UnitBase::TestResult UnitTimeoutSocket::testHttp() -{ - setTestname(__func__); - TST_LOG("Starting Test: " << testname); - - const std::string documentURL = "/favicon.ico"; - - // Keep alive socket, avoid forced socket disconnect via dtor - TerminatingPoll socketPoller(testname); - socketPoller.runOnClientThread(); - - const int sockCount = 4; - // Reused http session, keep-alive - std::vector> sessions; - - try - { - for(int sockIdx = 0; sockIdx < sockCount; ++sockIdx) { - sessions.push_back( http::Session::create(helpers::getTestServerURI()) ); - TST_LOG("Test: " << testname << "[" << sockIdx << "]: `" << documentURL << "`"); - http::Request request(documentURL, http::Request::VERB_GET); - const std::shared_ptr response = - sessions[sockIdx]->syncRequest(request, socketPoller); - TST_LOG("Response: " << response->header().toString()); - TST_LOG("Response size: " << testname << "[" << sockIdx << "]: `" << documentURL << "`: " << response->header().getContentLength()); - LOK_ASSERT_EQUAL(http::StatusCode::OK, response->statusCode()); - LOK_ASSERT_EQUAL(true, sessions[sockIdx]->isConnected()); - LOK_ASSERT(http::Header::ConnectionToken::None == - response->header().getConnectionToken()); - LOK_ASSERT(0 < response->header().getContentLength()); - } - } - catch (const Poco::Exception& exc) - { - LOK_ASSERT_FAIL(exc.displayText()); - } - LOK_ASSERT_EQUAL(true, pollDisconnected(std::chrono::microseconds(1000000), sessions, &socketPoller)); - - TST_LOG("Clearing Sessions: " << testname); - sessions.clear(); - TST_LOG("Clearing Poller: " << testname); - socketPoller.closeAllSockets(); - TST_LOG("Ending Test: " << testname); - return TestResult::Ok; -} - -/// Test the native WebSocket control-frame ping/pong facility -> No Timeout! -UnitBase::TestResult UnitTimeoutSocket::testWSPing() -{ - setTestname(__func__); - TST_LOG("Starting Test: " << testname); - - std::string documentPath, documentURL; - helpers::getDocumentPathAndURL("hello.odt", documentPath, documentURL, testname); - - std::shared_ptr session = http::WebSocketSession::create(helpers::getTestServerURI()); - http::Request req(documentURL); - session->asyncRequest(req, socketPoll()); - - // wsd/ClientSession.cpp:709 sendTextFrameAndLogError("error: cmd=" + tokens[0] + " kind=nodocloaded"); - constexpr const bool loadDoc = true; // Required for WSD chat -> wsd/ClientSession.cpp:709, common/Session.hpp:160 - if( loadDoc ) { - session->sendMessage("load url=" + documentURL); - } - - LOK_ASSERT_EQUAL(true, session->isConnected()); - - assertMessage(*session, "progress:", "find"); - assertMessage(*session, "progress:", "connect"); - assertMessage(*session, "progress:", "ready"); - - LOK_ASSERT_EQUAL(true, pollDisconnected(std::chrono::microseconds(1000000), *session)); - - TST_LOG("Ending Test: " << testname); - return TestResult::Ok; -} - -/// Tests the WSD chat ping/pong facility, where client sends the ping. -/// See: https://github.com/CollaboraOnline/online/blob/master/wsd/protocol.txt/ -UnitBase::TestResult UnitTimeoutSocket::testWSDChatPing() -{ - setTestname(__func__); - TST_LOG("Starting Test: " << testname); - - std::string documentPath, documentURL; - helpers::getDocumentPathAndURL("hello.odt", documentPath, documentURL, testname); - - std::shared_ptr session = http::WebSocketSession::create(helpers::getTestServerURI()); - http::Request req(documentURL); - session->asyncRequest(req, socketPoll()); - - // wsd/ClientSession.cpp:709 sendTextFrameAndLogError("error: cmd=" + tokens[0] + " kind=nodocloaded"); - constexpr const bool loadDoc = true; // Required for WSD chat -> wsd/ClientSession.cpp:709, common/Session.hpp:160 - if( loadDoc ) { - session->sendMessage("load url=" + documentURL); - } - - LOK_ASSERT_EQUAL(true, session->isConnected()); - - assertMessage(*session, "progress:", "find"); - assertMessage(*session, "progress:", "connect"); - assertMessage(*session, "progress:", "ready"); - - session->sendMessage("ping"); - assertMessage(*session, "", "pong"); - - LOK_ASSERT_EQUAL(true, pollDisconnected(std::chrono::microseconds(1000000), *session)); - - TST_LOG("Ending Test: " << testname); - return TestResult::Ok; -} - -void UnitTimeoutSocket::invokeWSDTest() -{ - UnitBase::TestResult result = TestResult::Ok; - - result = testHttp(); - if (result != TestResult::Ok) - exitTest(result); - - result = testWSPing(); - if (result != TestResult::Ok) - exitTest(result); - - result = testWSDChatPing(); - if (result != TestResult::Ok) - exitTest(result); - - exitTest(TestResult::Ok); -} - -UnitBase* unit_create_wsd(void) { return new UnitTimeoutSocket(); } - -/* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/test/UnitTimeoutWSPing.cpp b/test/UnitTimeoutWSPing.cpp index e6bfb1a6831f..0f2fe34378bd 100644 --- a/test/UnitTimeoutWSPing.cpp +++ b/test/UnitTimeoutWSPing.cpp @@ -40,7 +40,6 @@ class UnitTimeoutWSPing : public UnitTimeoutBase0 defaults.WSPingPeriod = std::chrono::microseconds(10000); // defaults.HTTPTimeout = std::chrono::microseconds(30000000); // defaults.MaxConnections = 9999; - // defaults.MinBytesPerSec = 0.0; // defaults.SocketPollTimeout = std::chrono::microseconds(64000000); } diff --git a/wsd/COOLWSD.cpp b/wsd/COOLWSD.cpp index 184503055235..89e09ad13104 100644 --- a/wsd/COOLWSD.cpp +++ b/wsd/COOLWSD.cpp @@ -2771,7 +2771,6 @@ void COOLWSD::innerInitialize(Poco::Util::Application& self) LOG_DBG("net::Defaults: WSPing[Timeout " << netDefaults.WSPingTimeout << ", Period " << netDefaults.WSPingPeriod << "], HTTP[Timeout " << netDefaults.HTTPTimeout << "], Socket[MaxConnections " << netDefaults.MaxConnections - << ", MinBytesPerSec " << netDefaults.MinBytesPerSec << "], SocketPoll[Timeout " << netDefaults.SocketPollTimeout << "]"); }