From 96587142c1983f234980765ef4a66ad576907a29 Mon Sep 17 00:00:00 2001 From: Malcolm Bechard Date: Wed, 15 Jan 2025 13:06:55 -0500 Subject: [PATCH] Fix missing increment of the instance count on every call to startup() Change how the implicit startup is tracked. The extra change is needed since the startup() calls in srt::CUDT::socket() and srt::CUDT::createGroup would endlessly increment the startup counter as sockets were created otherwise. Force a clean() when the singleton is getting destructed. Previously it just did a single decrement of the instance count, which could result in attempting to exit without actually cleaning up. fixes #3098 --- srtcore/api.cpp | 38 ++++++++++++++++++++------------ srtcore/api.h | 15 +++++++++++-- test/test_bonding.cpp | 6 ++++- test/test_common.cpp | 37 +++++++++++++++++++++++++++++++ test/test_connection_timeout.cpp | 4 +--- 5 files changed, 80 insertions(+), 20 deletions(-) diff --git a/srtcore/api.cpp b/srtcore/api.cpp index bb5dd64fe..46eb4fe12 100644 --- a/srtcore/api.cpp +++ b/srtcore/api.cpp @@ -198,13 +198,8 @@ srt::CUDTUnited::CUDTUnited() srt::CUDTUnited::~CUDTUnited() { - // Call it if it wasn't called already. - // This will happen at the end of main() of the application, - // when the user didn't call srt_cleanup(). - if (m_bGCStatus) - { - cleanup(); - } + // force cleanup, even if the instance count is > 1 + cleanup(true); releaseMutex(m_GlobControlLock); releaseMutex(m_IDLock); @@ -233,13 +228,20 @@ string srt::CUDTUnited::CONID(SRTSOCKET sock) return os.str(); } -int srt::CUDTUnited::startup() +int srt::CUDTUnited::startup(bool implicit) { ScopedLock gcinit(m_InitLock); - if (m_bGCStatus) + + if (implicit && m_iInstanceCount > 0) + // [reinstate in 1.6.0] return m_iInstanceCount; return 1; if (m_iInstanceCount++ > 0) + // [reinstate in 1.6.0] return m_iInstanceCount; + return 1; + + if (m_bGCStatus) + // [reinstate in 1.6.0] return m_iInstanceCount; return 1; // Global initialization code @@ -268,7 +270,7 @@ int srt::CUDTUnited::startup() return 0; } -int srt::CUDTUnited::cleanup() +int srt::CUDTUnited::cleanup(bool force) { // IMPORTANT!!! // In this function there must be NO LOGGING AT ALL. This function may @@ -280,9 +282,14 @@ int srt::CUDTUnited::cleanup() // executing this procedure. ScopedLock gcinit(m_InitLock); - if (--m_iInstanceCount > 0) - return 0; + if (!force) + { + if (m_iInstanceCount == 0 || --m_iInstanceCount > 0) + // [reinstate in 1.6.0] return m_iInstanceCount; + return 0; + } + // Cleanup done before already if (!m_bGCStatus) return 0; @@ -298,6 +305,8 @@ int srt::CUDTUnited::cleanup() CSync::notify_one_relaxed(m_GCStopCond); m_GCThread.join(); + if (force) + m_iInstanceCount = 0; m_bGCStatus = false; // Global destruction code @@ -305,6 +314,7 @@ int srt::CUDTUnited::cleanup() WSACleanup(); #endif + // Cleanup done at all. return 0; } @@ -3469,7 +3479,7 @@ int srt::CUDT::cleanup() SRTSOCKET srt::CUDT::socket() { - uglobal().startup(); + uglobal().startup(true); try { @@ -3519,7 +3529,7 @@ srt::CUDTGroup& srt::CUDT::newGroup(const int type) SRTSOCKET srt::CUDT::createGroup(SRT_GROUP_TYPE gt) { // Doing the same lazy-startup as with srt_create_socket() - uglobal().startup(); + uglobal().startup(true); try { diff --git a/srtcore/api.h b/srtcore/api.h index 48e7827f8..1eefcb2bc 100644 --- a/srtcore/api.h +++ b/srtcore/api.h @@ -257,12 +257,14 @@ class CUDTUnited static std::string CONID(SRTSOCKET sock); /// initialize the UDT library. + /// @param [in] implicit should be set to true for internal implicit startup() calls. /// @return 0 if success, otherwise -1 is returned. - int startup(); + int startup(bool implicit = false); /// release the UDT library. + /// @param [in] force cleanup regardless of the instance count. /// @return 0 if success, otherwise -1 is returned. - int cleanup(); + int cleanup(bool force = false); /// Create a new UDT socket. /// @param [out] pps Variable (optional) to which the new socket will be written, if succeeded @@ -542,6 +544,15 @@ class CUDTUnited int m_iInstanceCount; // number of startup() called by application SRT_ATTR_GUARDED_BY(m_InitLock) bool m_bGCStatus; // if the GC thread is working (true) +public: + + std::pair getInstanceStatus() + { + sync::ScopedLock lk(m_InitLock); + return std::make_pair(m_iInstanceCount, m_bGCStatus); + } + +private: SRT_ATTR_GUARDED_BY(m_InitLock) sync::CThread m_GCThread; diff --git a/test/test_bonding.cpp b/test/test_bonding.cpp index 4ec4a84f5..d50895567 100644 --- a/test/test_bonding.cpp +++ b/test/test_bonding.cpp @@ -724,7 +724,6 @@ TEST(Bonding, ConnectNonBlocking) EXPECT_NE(srt_bind(g_listen_socket, (sockaddr*)&bind_sa, sizeof bind_sa), -1); const int yes = 1; srt_setsockflag(g_listen_socket, SRTO_GROUPCONNECT, &yes, sizeof yes); - EXPECT_NE(srt_listen(g_listen_socket, 5), -1); int lsn_eid = srt_epoll_create(); int lsn_events = SRT_EPOLL_IN | SRT_EPOLL_ERR | SRT_EPOLL_UPDATE; @@ -761,6 +760,11 @@ TEST(Bonding, ConnectNonBlocking) cout << "[A] Waiting for accept\n"; + // Yield to allow the "too early" sending to fail + std::this_thread::yield(); + + EXPECT_NE(srt_listen(g_listen_socket, 5), -1); + // This can wait in infinity; worst case it will be killed in process. int uwait_res = srt_epoll_uwait(lsn_eid, ev, 3, -1); EXPECT_EQ(uwait_res, 1); diff --git a/test/test_common.cpp b/test/test_common.cpp index 1a8cca061..a029b160a 100644 --- a/test/test_common.cpp +++ b/test/test_common.cpp @@ -5,9 +5,44 @@ #include "test_env.h" #include "utilities.h" #include "common.h" +#include "api.h" using namespace srt; +TEST(General, Startup) +{ + // Should return 0 if it was run for the first time + // and actually performed the initialization. + EXPECT_EQ(srt_startup(), 0); + + EXPECT_EQ(srt::CUDT::uglobal().getInstanceStatus(), std::make_pair(1, true)); + + // Every next one should return the number of nested instances + // [reinstate in 1.6.0] EXPECT_EQ(srt_startup(), 2); + srt_startup(); + EXPECT_EQ(srt::CUDT::uglobal().getInstanceStatus(), std::make_pair(2, true)); + + // Now let's pair the first instance, should NOT execute + // [reinstate in 1.6.0] EXPECT_EQ(srt_cleanup(), 1); + srt_cleanup(); + + EXPECT_EQ(srt_cleanup(), 0); + + // Second cleanup, should report successful cleanup even if nothing is done. + EXPECT_EQ(srt_cleanup(), 0); + + // Now let's start with getting the number of instances + // from implicitly created ones. + SRTSOCKET sock = srt_create_socket(); + + EXPECT_EQ(srt::CUDT::uglobal().getInstanceStatus(), std::make_pair(1, true)); + + EXPECT_EQ(srt_close(sock), 0); + + // Do the cleanup again, to not leave it up to the global destructor. + EXPECT_EQ(srt_cleanup(), 0); +} + void test_cipaddress_pton(const char* peer_ip, int family, const uint32_t (&ip)[4]) { const int port = 4200; @@ -44,6 +79,8 @@ void test_cipaddress_pton(const char* peer_ip, int family, const uint32_t (&ip)[ // Example IPv4 address: 192.168.0.1 TEST(CIPAddress, IPv4_pton) { + // Check the NEXT TEST of General/Startup, if it has done the cleanup. + EXPECT_EQ(srt::CUDT::uglobal().getInstanceStatus(), std::make_pair(0, false)); srt::TestInit srtinit; const char* peer_ip = "192.168.0.1"; const uint32_t ip[4] = {htobe32(0xC0A80001), 0, 0, 0}; diff --git a/test/test_connection_timeout.cpp b/test/test_connection_timeout.cpp index ec52d9dd8..c8333d89c 100644 --- a/test/test_connection_timeout.cpp +++ b/test/test_connection_timeout.cpp @@ -211,7 +211,7 @@ TEST(TestConnectionAPI, Accept) using namespace std::chrono; using namespace srt; - srt_startup(); + TestInit srtinit; const SRTSOCKET caller_sock = srt_create_socket(); const SRTSOCKET listener_sock = srt_create_socket(); @@ -266,8 +266,6 @@ TEST(TestConnectionAPI, Accept) } srt_close(caller_sock); srt_close(listener_sock); - - srt_cleanup(); }