From e9160f9228fdea4629e25a2f94b3a2ec2669b764 Mon Sep 17 00:00:00 2001 From: Sektor van Skijlen Date: Tue, 19 Aug 2025 11:45:07 +0200 Subject: [PATCH 01/19] [MAINT] Added optional test discovery. Added improvements for attributes. Enabled std shared mutex if compiling with C++17 (#3190) * [MAINT] Added optional test discovery. Added improvements for attributes * Enabled C++17 version of shared mutex when compiling in C++17 mode * Added UT discovery option to configure * Changed enabled std::shared_mutex only if compiling with stdc++-sync * Fixed problem with not updated sync.cpp SharedMutex * Fixed review findings --------- Co-authored-by: Mikolaj Malecki --- CMakeLists.txt | 5 +++- configure-data.tcl | 1 + srtcore/srt.h | 4 +++ srtcore/srt_attr_defs.h | 61 +++++++++++++++++++++-------------------- srtcore/sync.cpp | 9 +++++- srtcore/sync.h | 12 ++++++-- test/test_sync.cpp | 3 ++ 7 files changed, 62 insertions(+), 33 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index a5547454f..0acd39b41 100755 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -164,6 +164,7 @@ option(ENABLE_PKTINFO "Enable using IP_PKTINFO to allow the listener extracting option(ENABLE_RELATIVE_LIBPATH "Should application contain relative library paths, like ../lib" OFF) option(ENABLE_GETNAMEINFO "In-logs sockaddr-to-string should do rev-dns" OFF) option(ENABLE_UNITTESTS "Enable unit tests" OFF) +option(ENABLE_UNITTESTS_DISCOVERY "Do unit test discovery when unit tests enabled" ON) option(ENABLE_ENCRYPTION "Enable encryption in SRT" ON) option(ENABLE_AEAD_API_PREVIEW "Enable AEAD API preview in SRT" Off) option(ENABLE_MAXREXMITBW "Enable SRTO_MAXREXMITBW (v1.6.0 API preview)" Off) @@ -1585,7 +1586,9 @@ if (ENABLE_UNITTESTS AND ENABLE_CXX11) #set_tests_properties(test-srt PROPERTIES RUN_SERIAL TRUE) else() set_tests_properties(${tests_srt} PROPERTIES RUN_SERIAL TRUE) - gtest_discover_tests(test-srt) + if (ENABLE_UNITTESTS_DISCOVERY) + gtest_discover_tests(test-srt) + endif() endif() enable_testing() diff --git a/configure-data.tcl b/configure-data.tcl index 3510a9645..d7b9d4937 100644 --- a/configure-data.tcl +++ b/configure-data.tcl @@ -49,6 +49,7 @@ set cmake_options { enable-relative-libpath "Should applications contain relative library paths, like ../lib (default: OFF)" enable-getnameinfo "In-logs sockaddr-to-string should do rev-dns (default: OFF)" enable-unittests "Enable Unit Tests (will download Google UT) (default: OFF)" + enable-unittests-discovery "Enable UT Discovery (will run when building) (default: ON)" enable-encryption "Should encryption features be enabled (default: ON)" enable-c++-deps "Extra library dependencies in srt.pc for C language (default: ON)" use-static-libstdc++ "Should use static rather than shared libstdc++ (default: OFF)" diff --git a/srtcore/srt.h b/srtcore/srt.h index 71ac2c3af..2d8ba3997 100644 --- a/srtcore/srt.h +++ b/srtcore/srt.h @@ -109,17 +109,21 @@ written by #define SRT_ATR_DEPRECATED #define SRT_ATR_DEPRECATED_PX [[deprecated]] +#define SRT_ATR_NODISCARD [[nodiscard]] // GNUG is GNU C/C++; this syntax is also supported by Clang #elif defined(__GNUC__) #define SRT_ATR_DEPRECATED_PX #define SRT_ATR_DEPRECATED __attribute__((deprecated)) +#define SRT_ATR_NODISCARD __attribute__((warn_unused_result)) #elif defined(_MSC_VER) #define SRT_ATR_DEPRECATED_PX __declspec(deprecated) #define SRT_ATR_DEPRECATED // no postfix-type modifier +#define SRT_ATR_NODISCARD _Check_return_ #else #define SRT_ATR_DEPRECATED_PX #define SRT_ATR_DEPRECATED +#define SRT_ATR_NODISCARD #endif #ifdef __cplusplus diff --git a/srtcore/srt_attr_defs.h b/srtcore/srt_attr_defs.h index 726c4a03b..1c489658f 100644 --- a/srtcore/srt_attr_defs.h +++ b/srtcore/srt_attr_defs.h @@ -17,26 +17,17 @@ used by SRT library internally. // ATTRIBUTES: // -// SRT_ATR_UNUSED: declare an entity ALLOWED to be unused (prevents warnings) -// ATR_DEPRECATED: declare an entity deprecated (compiler should warn when used) // ATR_NOEXCEPT: The true `noexcept` from C++11, or nothing if compiling in pre-C++11 mode // ATR_NOTHROW: In C++11: `noexcept`. In pre-C++11: `throw()`. Required for GNU libstdc++. // ATR_CONSTEXPR: In C++11: `constexpr`. Otherwise empty. // ATR_OVERRIDE: In C++11: `override`. Otherwise empty. // ATR_FINAL: In C++11: `final`. Otherwise empty. -#ifdef __GNUG__ -#define ATR_DEPRECATED __attribute__((deprecated)) -#else -#define ATR_DEPRECATED -#endif -#if HAVE_CXX11 -#define SRT_ATR_ALIGNAS(n) alignas(n) -#elif HAVE_GCC -#define SRT_ATR_ALIGNAS(n) __attribute__((aligned(n))) +#ifdef __GNUG__ +#define HAVE_GCC 1 #else -#define SRT_ATR_ALIGNAS(n) +#define HAVE_GCC 0 #endif #if defined(__cplusplus) && __cplusplus > 199711L @@ -44,18 +35,15 @@ used by SRT library internally. // For gcc 4.7, claim C++11 is supported, as long as experimental C++0x is on, // however it's only the "most required C++11 support". #if defined(__GXX_EXPERIMENTAL_CXX0X__) && __GNUC__ == 4 && __GNUC_MINOR__ >= 7 // 4.7 only! -#define ATR_NOEXCEPT -#define ATR_NOTHROW throw() -#define ATR_CONSTEXPR -#define ATR_OVERRIDE -#define ATR_FINAL #else #define HAVE_FULL_CXX11 1 -#define ATR_NOEXCEPT noexcept -#define ATR_NOTHROW noexcept -#define ATR_CONSTEXPR constexpr -#define ATR_OVERRIDE override -#define ATR_FINAL final + +#if __cplusplus >= 201703L +#define HAVE_CXX17 1 +#else +#define HAVE_CXX17 0 +#endif + #endif #elif defined(_MSC_VER) && _MSC_VER >= 1800 // Microsoft Visual Studio supports C++11, but not fully, @@ -65,26 +53,41 @@ used by SRT library internally. #define HAVE_CXX11 1 #if defined(_MSC_FULL_VER) && _MSC_FULL_VER >= 190023026 #define HAVE_FULL_CXX11 1 + +#if __cplusplus >= 201703L +#define HAVE_CXX17 1 +#else +#define HAVE_CXX17 0 +#endif + +#endif +#else +#define HAVE_CXX11 0 +#define HAVE_CXX17 0 +#endif // __cplusplus + +#if HAVE_FULL_CXX11 #define ATR_NOEXCEPT noexcept #define ATR_NOTHROW noexcept #define ATR_CONSTEXPR constexpr #define ATR_OVERRIDE override #define ATR_FINAL final #else +// These are both for HAVE_CXX11 == 1 and 0. #define ATR_NOEXCEPT #define ATR_NOTHROW throw() #define ATR_CONSTEXPR #define ATR_OVERRIDE #define ATR_FINAL #endif + +#if HAVE_CXX11 +#define SRT_ATR_ALIGNAS(n) alignas(n) +#elif HAVE_GCC +#define SRT_ATR_ALIGNAS(n) __attribute__((aligned(n))) #else -#define HAVE_CXX11 0 -#define ATR_NOEXCEPT -#define ATR_NOTHROW throw() -#define ATR_CONSTEXPR -#define ATR_OVERRIDE -#define ATR_FINAL -#endif // __cplusplus +#define SRT_ATR_ALIGNAS(n) +#endif #if !HAVE_CXX11 && defined(REQUIRE_CXX11) && REQUIRE_CXX11 == 1 #error "The currently compiled application required C++11, but your compiler doesn't support it." diff --git a/srtcore/sync.cpp b/srtcore/sync.cpp index bfe153657..96cb1a9d9 100644 --- a/srtcore/sync.cpp +++ b/srtcore/sync.cpp @@ -357,6 +357,11 @@ int srt::sync::genRandomInt(int minVal, int maxVal) #endif // HAVE_CXX11 } +#if defined(ENABLE_STDCXX_SYNC) && HAVE_CXX17 + +// Shared mutex imp not required - aliased from C++17 + +#else //////////////////////////////////////////////////////////////////////////////// // @@ -451,4 +456,6 @@ int srt::sync::SharedMutex::getReaderCount() const { ScopedLock lk(m_Mutex); return m_iCountRead; -} \ No newline at end of file +} +#endif // C++17 for shared_mutex + diff --git a/srtcore/sync.h b/srtcore/sync.h index 45285828c..7e11c29cf 100644 --- a/srtcore/sync.h +++ b/srtcore/sync.h @@ -12,6 +12,7 @@ #define INC_SRT_SYNC_H #include "platform_sys.h" +#include "srt_attr_defs.h" #include #include @@ -21,6 +22,9 @@ #include #include #include +#if HAVE_CXX17 +#include +#endif #define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_STDCXX_STEADY #define SRT_SYNC_CLOCK_STR "STDCXX_STEADY" #else @@ -54,7 +58,6 @@ #include "srt.h" #include "utilities.h" -#include "srt_attr_defs.h" namespace srt @@ -491,11 +494,15 @@ inline void releaseCond(Condition& cv) { cv.destroy(); } // /////////////////////////////////////////////////////////////////////////////// +#if defined(ENABLE_STDCXX_SYNC) && HAVE_CXX17 +using SharedMutex = std::shared_mutex; +#else + /// Implementation of a read-write mutex. /// This allows multiple readers at a time, or a single writer. /// TODO: The class can be improved if needed to give writer a preference /// by adding additional m_iWritersWaiting member variable (counter). -/// TODO: The m_iCountRead could be made atomic to make unlok_shared() faster and lock-free. +/// TODO: The m_iCountRead could be made atomic to make unlock_shared() faster and lock-free. class SharedMutex { public: @@ -526,6 +533,7 @@ class SharedMutex int m_iCountRead; bool m_bWriterLocked; }; +#endif /// A version of std::scoped_lock (or lock_guard for C++11). /// We could have used the srt::sync::ScopedLock making it a template-based class. diff --git a/test/test_sync.cpp b/test/test_sync.cpp index d06b5e5f5..9fb1b3db3 100644 --- a/test/test_sync.cpp +++ b/test/test_sync.cpp @@ -609,6 +609,8 @@ TEST(SyncThread, Joinable) EXPECT_FALSE(foo.joinable()); } +#if !HAVE_CXX17 + /*****************************************************************************/ /* * SharedMutex @@ -693,6 +695,7 @@ TEST(SharedMutex, LockedReadCount) EXPECT_TRUE(mut.try_lock()); } +#endif /*****************************************************************************/ /* From 7497397023cddd72a54ea9ed79b60e96f1085640 Mon Sep 17 00:00:00 2001 From: Mikolaj Malecki Date: Tue, 19 Aug 2025 18:39:37 +0200 Subject: [PATCH 02/19] [core] Fixed bug in srt_epoll_uwait: wrong number of sockets returned --- srtcore/epoll.cpp | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/srtcore/epoll.cpp b/srtcore/epoll.cpp index 8cd8440c7..c8c3c74b7 100644 --- a/srtcore/epoll.cpp +++ b/srtcore/epoll.cpp @@ -522,22 +522,17 @@ int srt::CEPoll::uwait(const int eid, SRT_EPOLL_EVENT* fdsSet, int fdsSize, int6 throw CUDTException(MJ_NOTSUP, MN_INVAL); } - int total = 0; // This is a list, so count it during iteration - CEPollDesc::enotice_t::iterator i = ed.enotice_begin(); - while (i != ed.enotice_end()) + CEPollDesc::enotice_t::iterator i = ed.enotice_begin(), inext; + int pos = 0; // This is a list, so count it during iteration + for (inext = i ; i != ed.enotice_end() && pos < fdsSize ; ++pos, i = inext) { - int pos = total; // previous past-the-end position - ++total; - - if (total > fdsSize) - break; + ++inext; // deletion-safe list loop fdsSet[pos] = *i; - - ed.checkEdge(i++); // NOTE: potentially deletes `i` + ed.checkEdge(i); // NOTE: potentially deletes `i` } - if (total) - return total; + if (pos) // pos is increased by 1 towards the last used position + return pos; } if ((msTimeOut >= 0) && (count_microseconds(srt::sync::steady_clock::now() - entertime) >= msTimeOut * int64_t(1000))) From d720b8c9a47c265f60eaf5a7bf6a1990423d039f Mon Sep 17 00:00:00 2001 From: Sektor van Skijlen Date: Tue, 26 Aug 2025 09:36:12 +0200 Subject: [PATCH 03/19] [core] Rework m_bListening to lock-free and m_GlobControlLock to use shared mutex (#3189) * [MAINT] Added optional test discovery. Added improvements for attributes * Enabled C++17 version of shared mutex when compiling in C++17 mode * [core] Rework m_bListening to lock-free and m_GlobControlLock to use shared mutex * Added UT discovery option to configure * Changed enabled std::shared_mutex only if compiling with stdc++-sync * Fixed problem with not updated sync.cpp SharedMutex * Removed dead code * Fixed review findings * Demoted some locks on m_GlobControlLock to shared. Added false positive TSan info * Merged changes from #3200 * Added a fix that should repair the problem reported in 3196. Still needs rechecks * Fixed incorrect shared lock applied for a case when a new group is to be created (required exclusive) --------- Co-authored-by: Mikolaj Malecki --- srtcore/api.cpp | 65 +++++++++------ srtcore/api.h | 2 +- srtcore/core.cpp | 200 +++++++++++++++++++++++++++++----------------- srtcore/core.h | 1 - srtcore/group.cpp | 6 +- srtcore/queue.cpp | 22 +++-- srtcore/queue.h | 5 +- srtcore/sync.h | 26 +++--- 8 files changed, 206 insertions(+), 121 deletions(-) diff --git a/srtcore/api.cpp b/srtcore/api.cpp index 14d7a4652..da929ef5a 100644 --- a/srtcore/api.cpp +++ b/srtcore/api.cpp @@ -281,7 +281,8 @@ void srt::CUDTUnited::closeAllSockets() HLOGC(inlog.Debug, log << "GC: GLOBAL EXIT - releasing all pending sockets. Acquring control lock..."); { - ScopedLock glock(m_GlobControlLock); + // Pre-closing: run over all open sockets and close them. + SharedLock glock(m_GlobControlLock); for (sockets_t::iterator i = m_Sockets.begin(); i != m_Sockets.end(); ++i) { @@ -297,6 +298,16 @@ void srt::CUDTUnited::closeAllSockets() s->removeFromGroup(false); } #endif + } + } + + { + ExclusiveLock glock(m_GlobControlLock); + + for (sockets_t::iterator i = m_Sockets.begin(); i != m_Sockets.end(); ++i) + { + CUDTSocket* s = i->second; + m_ClosedSockets[i->first] = s; // remove from listener's queue @@ -518,7 +529,7 @@ SRTSOCKET srt::CUDTUnited::newSocket(CUDTSocket** pps) HLOGC(smlog.Debug, log << CONID(ns->m_SocketID) << "newSocket: mapping socket " << ns->m_SocketID); // protect the m_Sockets structure. - ScopedLock cs(m_GlobControlLock); + ExclusiveLock cs(m_GlobControlLock); m_Sockets[ns->m_SocketID] = ns; } catch (...) @@ -536,6 +547,12 @@ SRTSOCKET srt::CUDTUnited::newSocket(CUDTSocket** pps) return ns->m_SocketID; } +// XXX NOTE: TSan reports here false positive against the call +// to CRcvQueue::removeListener. This here will apply shared +// lock on m_GlobControlLock in the call of locateSocket, while +// having applied a shared lock on CRcvQueue::m_pListener in +// CRcvQueue::worker_ProcessConnectionRequest. As this thread +// locks both mutexes as shared, it doesn't form a deadlock. int srt::CUDTUnited::newConnection(const SRTSOCKET listen, const sockaddr_any& peer, const CPacket& hspkt, @@ -675,7 +692,7 @@ int srt::CUDTUnited::newConnection(const SRTSOCKET listen, HLOGC(cnlog.Debug, log << "newConnection: incoming " << peer.str() << ", mapping socket " << ns->m_SocketID); { - ScopedLock cg(m_GlobControlLock); + ExclusiveLock cg(m_GlobControlLock); m_Sockets[ns->m_SocketID] = ns; } @@ -724,7 +741,7 @@ int srt::CUDTUnited::newConnection(const SRTSOCKET listen, { // protect the m_PeerRec structure (and group existence) - ScopedLock glock(m_GlobControlLock); + ExclusiveLock glock(m_GlobControlLock); try { HLOGC(cnlog.Debug, log << "newConnection: mapping peer " << ns->m_PeerID @@ -898,7 +915,7 @@ int srt::CUDTUnited::newConnection(const SRTSOCKET listen, // connect() in UDT code) may fail, in which case this socket should not be // further processed and should be removed. { - ScopedLock cg(m_GlobControlLock); + ExclusiveLock cg(m_GlobControlLock); #if ENABLE_BONDING if (ns->m_GroupOf) @@ -973,7 +990,7 @@ int srt::CUDTUnited::installConnectHook(const SRTSOCKET u, srt_connect_callback_ SRT_SOCKSTATUS srt::CUDTUnited::getStatus(const SRTSOCKET u) { // protects the m_Sockets structure - ScopedLock cg(m_GlobControlLock); + SharedLock cg(m_GlobControlLock); sockets_t::const_iterator i = m_Sockets.find(u); @@ -1247,7 +1264,7 @@ SRTSOCKET srt::CUDTUnited::accept(const SRTSOCKET listen, sockaddr* pw_addr, int { // Put a lock to protect the group against accidental deletion // in the meantime. - ScopedLock glock(m_GlobControlLock); + SharedLock glock(m_GlobControlLock); // Check again; it's unlikely to happen, but // it's a theoretically possible scenario if (s->m_GroupOf) @@ -1527,7 +1544,7 @@ int srt::CUDTUnited::groupConnect(CUDTGroup* pg, SRT_SOCKGROUPCONFIG* targets, i } { - ScopedLock cs(m_GlobControlLock); + ExclusiveLock cs(m_GlobControlLock); if (m_Sockets.count(sid) == 0) { HLOGC(aclog.Debug, log << "srt_connect_group: socket @" << sid << " deleted in process"); @@ -1633,7 +1650,7 @@ int srt::CUDTUnited::groupConnect(CUDTGroup* pg, SRT_SOCKGROUPCONFIG* targets, i targets[tii].errorcode = e.getErrorCode(); targets[tii].id = CUDT::INVALID_SOCK; - ScopedLock cl(m_GlobControlLock); + ExclusiveLock cl(m_GlobControlLock); ns->removeFromGroup(false); m_Sockets.erase(ns->m_SocketID); // Intercept to delete the socket on failure. @@ -1645,7 +1662,7 @@ int srt::CUDTUnited::groupConnect(CUDTGroup* pg, SRT_SOCKGROUPCONFIG* targets, i LOGC(aclog.Fatal, log << "groupConnect: IPE: UNKNOWN EXCEPTION from connectIn"); targets[tii].errorcode = SRT_ESYSOBJ; targets[tii].id = CUDT::INVALID_SOCK; - ScopedLock cl(m_GlobControlLock); + ExclusiveLock cl(m_GlobControlLock); ns->removeFromGroup(false); m_Sockets.erase(ns->m_SocketID); // Intercept to delete the socket on failure. @@ -2019,7 +2036,7 @@ void srt::CUDTUnited::deleteGroup(CUDTGroup* g) { using srt_logging::gmlog; - srt::sync::ScopedLock cg(m_GlobControlLock); + srt::sync::ExclusiveLock cg(m_GlobControlLock); return deleteGroup_LOCKED(g); } @@ -2110,7 +2127,7 @@ int srt::CUDTUnited::close(CUDTSocket* s) HLOGC(smlog.Debug, log << "@" << u << "U::close done. GLOBAL CLOSE: " << s->core().CONID() << "Acquiring GLOBAL control lock"); - ScopedLock manager_cg(m_GlobControlLock); + ExclusiveLock manager_cg(m_GlobControlLock); // since "s" is located before m_GlobControlLock, locate it again in case // it became invalid // XXX This is very weird; if we state that the CUDTSocket object @@ -2181,7 +2198,7 @@ int srt::CUDTUnited::close(CUDTSocket* s) // Done the other way, but still done. You can stop waiting. bool isgone = false; { - ScopedLock manager_cg(m_GlobControlLock); + SharedLock manager_cg(m_GlobControlLock); isgone = m_ClosedSockets.count(u) == 0; } if (!isgone) @@ -2583,7 +2600,7 @@ int srt::CUDTUnited::epoll_release(const int eid) srt::CUDTSocket* srt::CUDTUnited::locateSocket(const SRTSOCKET u, ErrorHandling erh) { - ScopedLock cg(m_GlobControlLock); + SharedLock cg(m_GlobControlLock); CUDTSocket* s = locateSocket_LOCKED(u); if (!s) { @@ -2611,7 +2628,7 @@ srt::CUDTSocket* srt::CUDTUnited::locateSocket_LOCKED(SRTSOCKET u) #if ENABLE_BONDING srt::CUDTGroup* srt::CUDTUnited::locateAcquireGroup(SRTSOCKET u, ErrorHandling erh) { - ScopedLock cg(m_GlobControlLock); + SharedLock cg(m_GlobControlLock); const groups_t::iterator i = m_Groups.find(u); if (i == m_Groups.end()) @@ -2628,7 +2645,7 @@ srt::CUDTGroup* srt::CUDTUnited::locateAcquireGroup(SRTSOCKET u, ErrorHandling e srt::CUDTGroup* srt::CUDTUnited::acquireSocketsGroup(CUDTSocket* s) { - ScopedLock cg(m_GlobControlLock); + SharedLock cg(m_GlobControlLock); CUDTGroup* g = s->m_GroupOf; if (!g) return NULL; @@ -2642,7 +2659,7 @@ srt::CUDTGroup* srt::CUDTUnited::acquireSocketsGroup(CUDTSocket* s) srt::CUDTSocket* srt::CUDTUnited::locateAcquireSocket(SRTSOCKET u, ErrorHandling erh) { - ScopedLock cg(m_GlobControlLock); + SharedLock cg(m_GlobControlLock); CUDTSocket* s = locateSocket_LOCKED(u); if (!s) @@ -2665,7 +2682,7 @@ bool srt::CUDTUnited::acquireSocket(CUDTSocket* s) // directly from m_Sockets, or even better, has been acquired // by some other functionality already, which is only about to // be released earlier than you need. - ScopedLock cg(m_GlobControlLock); + SharedLock cg(m_GlobControlLock); s->apiAcquire(); // Keep the lock so that no one changes anything in the meantime. // If the socket m_Status == SRTS_CLOSED (set by setClosed()), then @@ -2681,7 +2698,7 @@ bool srt::CUDTUnited::acquireSocket(CUDTSocket* s) srt::CUDTSocket* srt::CUDTUnited::locatePeer(const sockaddr_any& peer, const SRTSOCKET id, int32_t isn) { - ScopedLock cg(m_GlobControlLock); + SharedLock cg(m_GlobControlLock); map >::iterator i = m_PeerRec.find(CUDTSocket::getPeerSpec(id, isn)); if (i == m_PeerRec.end()) @@ -2705,7 +2722,7 @@ srt::CUDTSocket* srt::CUDTUnited::locatePeer(const sockaddr_any& peer, const SRT void srt::CUDTUnited::checkBrokenSockets() { - ScopedLock cg(m_GlobControlLock); + ExclusiveLock cg(m_GlobControlLock); #if ENABLE_BONDING vector delgids; @@ -3059,7 +3076,7 @@ bool srt::CUDTUnited::channelSettingsMatch(const CSrtMuxerConfig& cfgMuxer, cons void srt::CUDTUnited::updateMux(CUDTSocket* s, const sockaddr_any& reqaddr, const UDPSOCKET* udpsock /*[[nullable]]*/) { - ScopedLock cg(m_GlobControlLock); + ExclusiveLock cg(m_GlobControlLock); // If udpsock is provided, then this socket will be simply // taken for binding as a good deal. It would be nice to make @@ -3366,7 +3383,7 @@ void srt::CUDTUnited::updateMux(CUDTSocket* s, const sockaddr_any& reqaddr, cons // multiplexer wasn't found by id, the search by port number continues. bool srt::CUDTUnited::updateListenerMux(CUDTSocket* s, const CUDTSocket* ls) { - ScopedLock cg(m_GlobControlLock); + ExclusiveLock cg(m_GlobControlLock); const int port = ls->m_SelfAddr.hport(); HLOGC(smlog.Debug, @@ -3534,7 +3551,7 @@ SRTSOCKET srt::CUDT::createGroup(SRT_GROUP_TYPE gt) { try { - srt::sync::ScopedLock globlock(uglobal().m_GlobControlLock); + srt::sync::ExclusiveLock globlock(uglobal().m_GlobControlLock); return newGroup(gt).id(); // Note: potentially, after this function exits, the group // could be deleted, immediately, from a separate thread (tho @@ -3590,7 +3607,7 @@ SRTSOCKET srt::CUDT::getGroupOfSocket(SRTSOCKET socket) { // Lock this for the whole function as we need the group // to persist the call. - ScopedLock glock(uglobal().m_GlobControlLock); + SharedLock glock(uglobal().m_GlobControlLock); CUDTSocket* s = uglobal().locateSocket_LOCKED(socket); if (!s || !s->m_GroupOf) return APIError(MJ_NOTSUP, MN_INVAL, 0); diff --git a/srtcore/api.h b/srtcore/api.h index d0249ca10..d3f8427d9 100644 --- a/srtcore/api.h +++ b/srtcore/api.h @@ -407,7 +407,7 @@ class CUDTUnited groups_t m_Groups; #endif - sync::Mutex m_GlobControlLock; // used to synchronize UDT API + sync::SharedMutex m_GlobControlLock; // used to synchronize UDT API sync::Mutex m_IDLock; // used to synchronize ID generation diff --git a/srtcore/core.cpp b/srtcore/core.cpp index d2118c555..344f21ac1 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -1012,8 +1012,6 @@ void srt::CUDT::open() void srt::CUDT::setListenState() { - ScopedLock cg(m_ConnectionLock); - if (!m_bOpened) throw CUDTException(MJ_NOTSUP, MN_NONE, 0); @@ -1021,14 +1019,42 @@ void srt::CUDT::setListenState() throw CUDTException(MJ_NOTSUP, MN_ISCONNECTED, 0); // listen can be called more than once - if (m_bListening) - return; - - // if there is already another socket listening on the same port - if (m_pRcvQueue->setListener(this) < 0) - throw CUDTException(MJ_NOTSUP, MN_BUSY, 0); - - m_bListening = true; + // If two threads call srt_listen at the same time, only + // one will pass this condition; others will be rejected. + // If it was called ever once, none will pass. + for (;;) + { + if (m_bListening.compare_exchange(false, true)) + { + // if there is already another socket listening on the same port + if (!m_pRcvQueue->setListener(this)) + { + // Failed here, so + m_bListening = false; + throw CUDTException(MJ_NOTSUP, MN_BUSY, 0); + } + } + else + { + // Ok, this thread could have been blocked access, + // but still the other thread that attempted to set + // the listener could have failed. Therefore check + // again if the listener was set successfully, and + // if the listening point is still free, try again. + CUDT* current = m_pRcvQueue->getListener(); + if (current == NULL) + { + continue; + } + else if (current != this) + { + // Some other listener already set it + throw CUDTException(MJ_NOTSUP, MN_BUSY, 0); + } + // If it was you who set this, just return with no exception. + } + break; + } } size_t srt::CUDT::fillSrtHandshake(uint32_t *aw_srtdata, size_t srtlen, int msgtype, int hs_version) @@ -1734,7 +1760,7 @@ bool srt::CUDT::createSrtHandshake( if (have_group) { // NOTE: See information about mutex ordering in api.h - ScopedLock gdrg (uglobal().m_GlobControlLock); + SharedLock gdrg (uglobal().m_GlobControlLock); if (!m_parent->m_GroupOf) { // This may only happen if since last check of m_GroupOf pointer the socket was removed @@ -3188,10 +3214,19 @@ bool srt::CUDT::interpretGroup(const int32_t groupdata[], size_t data_size SRT_A return false; } - ScopedLock guard_group_existence (uglobal().m_GlobControlLock); - if (m_SrtHsSide == HSD_INITIATOR) { + // Here we'll be only using a pre-existing group, so shared is enough. + SharedLock guard_group_existence (uglobal().m_GlobControlLock); + + // Recheck broken flags after acquisition + if (m_bClosing || m_bBroken) + { + m_RejectReason = SRT_REJ_CLOSE; + LOGC(cnlog.Error, log << CONID() << "interpretGroup: closure during handshake, interrupting"); + return false; + } + // This is a connection initiator that has requested the peer to make a // mirror group and join it, then respond its mirror group id. The // `grpid` variable contains this group ID; map this as your peer @@ -3210,64 +3245,81 @@ bool srt::CUDT::interpretGroup(const int32_t groupdata[], size_t data_size SRT_A return false; } - // Group existence is guarded, so we can now lock the group as well. - ScopedLock gl(*pg->exp_groupLock()); - - // Now we know the group exists, but it might still be closed - if (pg->closing()) { - LOGC(cnlog.Error, log << CONID() << "HS/RSP: group was closed in the process, can't continue connecting"); - m_RejectReason = SRT_REJ_IPE; - return false; - } + // XXX Consider moving this to another function to avoid + // exposing group lock. - SRTSOCKET peer = pg->peerid(); - if (peer == -1) - { - // This is the first connection within this group, so this group - // has just been informed about the peer membership. Accept it. - pg->set_peerid(grpid); - HLOGC(cnlog.Debug, - log << CONID() << "HS/RSP: group $" << pg->id() << " -> peer $" << pg->peerid() - << ", copying characteristic data"); + // Group existence is guarded, so we can now lock the group as well. + ScopedLock gl(*pg->exp_groupLock()); - // The call to syncWithSocket is copying - // some interesting data from the first connected - // socket. This should be only done for the first successful connection. - pg->syncWithSocket(*this, HSD_INITIATOR); - } - // Otherwise the peer id must be the same as existing, otherwise - // this group is considered already bound to another peer group. - // (Note that the peer group is peer-specific, and peer id numbers - // may repeat among sockets connected to groups established on - // different peers). - else if (peer != grpid) - { - LOGC(cnlog.Error, - log << CONID() << "IPE: HS/RSP: group membership responded for peer $" << grpid - << " but the current socket's group $" << pg->id() << " has already a peer $" << peer); - m_RejectReason = SRT_REJ_GROUP; - return false; - } - else - { - HLOGC(cnlog.Debug, - log << CONID() << "HS/RSP: group $" << pg->id() << " ALREADY MAPPED to peer mirror $" - << pg->peerid()); + // Now we know the group exists, but it might still be closed + if (pg->closing()) + { + LOGC(cnlog.Error, log << CONID() << "HS/RSP: group was closed in the process, can't continue connecting"); + m_RejectReason = SRT_REJ_IPE; + return false; + } + + SRTSOCKET peer = pg->peerid(); + if (peer == SRT_INVALID_SOCK) + { + // This is the first connection within this group, so this group + // has just been informed about the peer membership. Accept it. + pg->set_peerid(grpid); + HLOGC(cnlog.Debug, + log << CONID() << "HS/RSP: group $" << pg->id() << " -> peer $" << pg->peerid() + << ", copying characteristic data"); + + // The call to syncWithSocket is copying + // some interesting data from the first connected + // socket. This should be only done for the first successful connection. + pg->syncWithSocket(*this, HSD_INITIATOR); + } + // Otherwise the peer id must be the same as existing, otherwise + // this group is considered already bound to another peer group. + // (Note that the peer group is peer-specific, and peer id numbers + // may repeat among sockets connected to groups established on + // different peers). + else if (peer != grpid) + { + LOGC(cnlog.Error, + log << CONID() << "IPE: HS/RSP: group membership responded for peer $" << grpid + << " but the current socket's group $" << pg->id() << " has already a peer $" << peer); + m_RejectReason = SRT_REJ_GROUP; + return false; + } + else + { + HLOGC(cnlog.Debug, + log << CONID() << "HS/RSP: group $" << pg->id() << " ALREADY MAPPED to peer mirror $" + << pg->peerid()); + } } + m_parent->m_GroupOf->debugGroup(); } else { + // Here we'll be potentially creating a new group, hence exclusive is needed. + ExclusiveLock guard_group_existence (uglobal().m_GlobControlLock); + + // Recheck broken flags after acquisition + if (m_bClosing || m_bBroken) + { + m_RejectReason = SRT_REJ_CLOSE; + LOGC(cnlog.Error, log << CONID() << "interpretGroup: closure during handshake, interrupting"); + return false; + } + // This is a connection responder that has been requested to make a // mirror group and join it. Later on, the HS response will be sent // and its group ID will be added to the HS extensions as mirror group // ID to the peer. SRTSOCKET lgid = makeMePeerOf(grpid, gtp, link_flags); - if (!lgid) + if (lgid == 0) return true; // already done - if (lgid == -1) + if (lgid == SRT_INVALID_SOCK) { // NOTE: This error currently isn't reported by makeMePeerOf, // so this is left to handle a possible error introduced in future. @@ -3288,10 +3340,9 @@ bool srt::CUDT::interpretGroup(const int32_t groupdata[], size_t data_size SRT_A f->weight = link_weight; f->agent = m_parent->m_SelfAddr; f->peer = m_PeerAddr; + m_parent->m_GroupOf->debugGroup(); } - m_parent->m_GroupOf->debugGroup(); - // That's all. For specific things concerning group // types, this will be later. return true; @@ -4843,7 +4894,7 @@ EConnectStatus srt::CUDT::postConnect(const CPacket* pResponse, bool rendezvous, { #if ENABLE_BONDING - ScopedLock cl (uglobal().m_GlobControlLock); + SharedLock cl (uglobal().m_GlobControlLock); CUDTGroup* g = m_parent->m_GroupOf; if (g) { @@ -4950,10 +5001,11 @@ EConnectStatus srt::CUDT::postConnect(const CPacket* pResponse, bool rendezvous, //int token = -1; #if ENABLE_BONDING { - ScopedLock cl (uglobal().m_GlobControlLock); + SharedLock cl (uglobal().m_GlobControlLock); CUDTGroup* g = m_parent->m_GroupOf; if (g) { + ScopedLock gl (*g->exp_groupLock()); // XXX this might require another check of group type. // For redundancy group, at least, update the status in the group. @@ -6380,7 +6432,11 @@ bool srt::CUDT::closeInternal() ATR_NOEXCEPT if (m_bListening) { m_bListening = false; - m_pRcvQueue->removeListener(this); + bool removed SRT_ATR_UNUSED = m_pRcvQueue->removeListener(this); + if (!removed) + { + LOGC(smlog.Error, log << CONID() << "CLOSING: IPE: listening=true but listener removal failed!"); + } } else if (m_bConnecting) { @@ -8213,7 +8269,7 @@ int srt::CUDT::sendCtrlAck(CPacket& ctrlpkt, int size) // can be either never set, already reset, or ever set // and possibly dangling. The re-check after lock eliminates // the dangling case. - ScopedLock glock (uglobal().m_GlobControlLock); + SharedLock glock (uglobal().m_GlobControlLock); // Note that updateLatestRcv will lock m_GroupOf->m_GroupLock, // but this is an intended order. @@ -8274,7 +8330,7 @@ int srt::CUDT::sendCtrlAck(CPacket& ctrlpkt, int size) if (group_read_seq != SRT_SEQNO_NONE && m_parent->m_GroupOf) { // See above explanation for double-checking - ScopedLock glock (uglobal().m_GlobControlLock); + SharedLock glock (uglobal().m_GlobControlLock); if (m_parent->m_GroupOf) { @@ -8431,7 +8487,7 @@ void srt::CUDT::updateSndLossListOnACK(int32_t ackdata_seqno) { // m_RecvAckLock is ordered AFTER m_GlobControlLock, so this can only // be done now that m_RecvAckLock is unlocked. - ScopedLock glock (uglobal().m_GlobControlLock); + SharedLock glock (uglobal().m_GlobControlLock); if (m_parent->m_GroupOf) { HLOGC(inlog.Debug, log << CONID() << "ACK: acking group sender buffer for #" << msgno_at_last_acked_seq); @@ -8591,7 +8647,7 @@ void srt::CUDT::processCtrlAck(const CPacket &ctrlpkt, const steady_clock::time_ #if ENABLE_BONDING if (m_parent->m_GroupOf) { - ScopedLock glock (uglobal().m_GlobControlLock); + SharedLock glock (uglobal().m_GlobControlLock); if (m_parent->m_GroupOf) { // Will apply m_GroupLock, ordered after m_GlobControlLock. @@ -8812,7 +8868,7 @@ void srt::CUDT::processCtrlAckAck(const CPacket& ctrlpkt, const time_point& tsAr if (m_config.bDriftTracer) { #if ENABLE_BONDING - ScopedLock glock(uglobal().m_GlobControlLock); // XXX not too excessive? + ExclusiveLock glock(uglobal().m_GlobControlLock); // XXX not too excessive? const bool drift_updated = #endif m_pRcvBuffer->addRcvTsbPdDriftSample(ctrlpkt.getMsgTimeStamp(), tsArrival, rtt); @@ -9358,7 +9414,7 @@ void srt::CUDT::updateAfterSrtHandshake(int hsv) if (m_parent->m_GroupOf) { - ScopedLock glock (uglobal().m_GlobControlLock); + SharedLock glock (uglobal().m_GlobControlLock); grpspec = m_parent->m_GroupOf ? " group=$" + Sprint(m_parent->m_GroupOf->id()) : string(); @@ -10512,7 +10568,7 @@ int srt::CUDT::processData(CUnit* in_unit) // reception sequence pointer stating that this link is not receiving. if (m_parent->m_GroupOf) { - ScopedLock protect_group_existence (uglobal().m_GlobControlLock); + ExclusiveLock protect_group_existence (uglobal().m_GlobControlLock); groups::SocketData* gi = m_parent->m_GroupMemberData; // This check is needed as after getting the lock the socket @@ -11720,7 +11776,7 @@ void srt::CUDT::checkTimers() #if ENABLE_BONDING if (m_parent->m_GroupOf) { - ScopedLock glock (uglobal().m_GlobControlLock); + SharedLock glock (uglobal().m_GlobControlLock); if (m_parent->m_GroupOf) { // Pass socket ID because it's about changing group socket data @@ -11752,7 +11808,7 @@ void srt::CUDT::completeBrokenConnectionDependencies(int errorcode) #if ENABLE_BONDING bool pending_broken = false; { - ScopedLock guard_group_existence (uglobal().m_GlobControlLock); + SharedLock guard_group_existence (uglobal().m_GlobControlLock); if (m_parent->m_GroupOf) { token = m_parent->m_GroupMemberData->token; @@ -11787,7 +11843,7 @@ void srt::CUDT::completeBrokenConnectionDependencies(int errorcode) // existence of the group will not be changed during // the operation. The attempt of group deletion will // have to wait until this operation completes. - ScopedLock lock(uglobal().m_GlobControlLock); + ExclusiveLock lock(uglobal().m_GlobControlLock); CUDTGroup* pg = m_parent->m_GroupOf; if (pg) { @@ -12045,7 +12101,7 @@ void srt::CUDT::processKeepalive(const CPacket& ctrlpkt, const time_point& tsArr // existence of the group will not be changed during // the operation. The attempt of group deletion will // have to wait until this operation completes. - ScopedLock lock(uglobal().m_GlobControlLock); + ExclusiveLock lock(uglobal().m_GlobControlLock); CUDTGroup* pg = m_parent->m_GroupOf; if (pg) { diff --git a/srtcore/core.h b/srtcore/core.h index 6f7bd6b19..00fd44923 100644 --- a/srtcore/core.h +++ b/srtcore/core.h @@ -436,7 +436,6 @@ class CUDT // immediately to free the socket void notListening() { - sync::ScopedLock cg(m_ConnectionLock); m_bListening = false; m_pRcvQueue->removeListener(this); } diff --git a/srtcore/group.cpp b/srtcore/group.cpp index dc3c1a9be..339512573 100644 --- a/srtcore/group.cpp +++ b/srtcore/group.cpp @@ -1044,7 +1044,7 @@ void CUDTGroup::close() vector ids; { - ScopedLock glob(CUDT::uglobal().m_GlobControlLock); + ExclusiveLock glob(CUDT::uglobal().m_GlobControlLock); ScopedLock g(m_GroupLock); m_bClosing = true; @@ -3409,7 +3409,7 @@ void CUDTGroup::send_CloseBrokenSockets(vector& w_wipeme) // With unlocked GroupLock, we can now lock GlobControlLock. // This is needed to prevent any of them deleted from the container // at the same time. - ScopedLock globlock(CUDT::uglobal().m_GlobControlLock); + SharedLock globlock(CUDT::uglobal().m_GlobControlLock); for (vector::iterator p = w_wipeme.begin(); p != w_wipeme.end(); ++p) { @@ -3446,7 +3446,7 @@ void CUDTGroup::sendBackup_CloseBrokenSockets(SendBackupCtx& w_sendBackupCtx) // With unlocked GroupLock, we can now lock GlobControlLock. // This is needed prevent any of them be deleted from the container // at the same time. - ScopedLock globlock(CUDT::uglobal().m_GlobControlLock); + SharedLock globlock(CUDT::uglobal().m_GlobControlLock); typedef vector::const_iterator const_iter_t; for (const_iter_t member = w_sendBackupCtx.memberStates().begin(); member != w_sendBackupCtx.memberStates().end(); ++member) diff --git a/srtcore/queue.cpp b/srtcore/queue.cpp index 6cb4faeb1..f8b782174 100644 --- a/srtcore/queue.cpp +++ b/srtcore/queue.cpp @@ -1429,7 +1429,7 @@ srt::EConnectStatus srt::CRcvQueue::worker_ProcessConnectionRequest(CUnit* unit, bool have_listener = false; { SharedLock shl(m_pListener); - CUDT* pListener = m_pListener.getPtrNoLock(); + CUDT* pListener = m_pListener.get_locked(shl); if (pListener) { @@ -1720,17 +1720,25 @@ int srt::CRcvQueue::recvfrom(int32_t id, CPacket& w_packet) return (int)w_packet.getLength(); } -int srt::CRcvQueue::setListener(CUDT* u) +bool srt::CRcvQueue::setListener(CUDT* u) { - if (!m_pListener.set(u)) - return -1; + return m_pListener.compare_exchange(NULL, u); +} - return 0; +srt::CUDT* srt::CRcvQueue::getListener() +{ + SharedLock lkl (m_pListener); + return m_pListener.get_locked(lkl); } -void srt::CRcvQueue::removeListener(const CUDT* u) +// XXX NOTE: TSan reports here false positive against the call +// to locateSocket in CUDTUnited::newConnection. This here will apply +// exclusive lock on m_pListener, while keeping shared lock on +// CUDTUnited::m_GlobControlLock in CUDTUnited::closeAllSockets. +// As the other thread locks both as shared, this is no deadlock risk. +bool srt::CRcvQueue::removeListener(CUDT* u) { - m_pListener.clearIf(u); + return m_pListener.compare_exchange(u, NULL); } void srt::CRcvQueue::registerConnector(const SRTSOCKET& id, diff --git a/srtcore/queue.h b/srtcore/queue.h index 48bedd9af..0d23790e9 100644 --- a/srtcore/queue.h +++ b/srtcore/queue.h @@ -538,8 +538,9 @@ class CRcvQueue #endif private: - int setListener(CUDT* u); - void removeListener(const CUDT* u); + bool setListener(CUDT* u); + CUDT* getListener(); + bool removeListener(CUDT* u); void registerConnector(const SRTSOCKET& id, CUDT* u, diff --git a/srtcore/sync.h b/srtcore/sync.h index 7e11c29cf..100a11eed 100644 --- a/srtcore/sync.h +++ b/srtcore/sync.h @@ -535,6 +535,15 @@ class SharedMutex }; #endif +inline void enterCS(SharedMutex& m) SRT_ATTR_EXCLUDES(m) SRT_ATTR_ACQUIRE(m) { m.lock(); } + +inline bool tryEnterCS(SharedMutex& m) SRT_ATTR_EXCLUDES(m) SRT_ATTR_TRY_ACQUIRE(true, m) { return m.try_lock(); } + +inline void leaveCS(SharedMutex& m) SRT_ATTR_REQUIRES(m) SRT_ATTR_RELEASE(m) { m.unlock(); } + +inline void setupMutex(SharedMutex&, const char*) {} +inline void releaseMutex(SharedMutex&) {} + /// A version of std::scoped_lock (or lock_guard for C++11). /// We could have used the srt::sync::ScopedLock making it a template-based class. /// But in that case all usages would have to be specificed like ScopedLock in C++03. @@ -585,26 +594,21 @@ class CSharedObjectPtr : public SharedMutex { } - bool set(T* pObj) + bool compare_exchange(T* expected, T* newobj) { ExclusiveLock lock(*this); - if (m_pObj) + if (m_pObj != expected) return false; - m_pObj = pObj; + m_pObj = newobj; return true; } - bool clearIf(const T* pObj) + T* get_locked(SharedLock& /*wholocked*/) { - ExclusiveLock lock(*this); - if (m_pObj != pObj) - return false; - m_pObj = NULL; - return true; + // XXX Here you can assert that `wholocked` locked *this. + return m_pObj; } - T* getPtrNoLock() const { return m_pObj; } - private: T* m_pObj; }; From 725e2d472115c4ba745822441b79125ec9efdd58 Mon Sep 17 00:00:00 2001 From: cl-ment Date: Wed, 27 Aug 2025 10:56:20 +0200 Subject: [PATCH 04/19] [build] Update Ubuntu runners to Lastest. (#3180) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [build] Update Ubuntu runners to 22.04. * Update to googletest-1.12.1 * Remove Unit Tests from the ABI workflow. * Use latest version of ubuntu * Remove build-wrapper. --------- Authored-by: Clément Gérouville --- .github/workflows/abi.yml | 6 +++--- .github/workflows/android.yaml | 2 +- .github/workflows/cxx11-ubuntu.yaml | 4 ++-- scripts/googletest-download.cmake | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/workflows/abi.yml b/.github/workflows/abi.yml index 2c05cc06b..51d22a516 100644 --- a/.github/workflows/abi.yml +++ b/.github/workflows/abi.yml @@ -12,7 +12,7 @@ env: jobs: build: name: ABI checks - runs-on: ubuntu-20.04 + runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 @@ -22,7 +22,7 @@ jobs: run: | cd pull_request mkdir _build && cd _build - cmake -DCMAKE_BUILD_TYPE=Debug -DENABLE_UNITTESTS=ON ../ + cmake -DCMAKE_BUILD_TYPE=Debug ../ - name: build run: | sudo apt install -y abi-dumper @@ -41,7 +41,7 @@ jobs: echo $SRT_TAG_VERSION cd tag mkdir _build && cd _build - cmake -DCMAKE_BUILD_TYPE=Debug -DENABLE_UNITTESTS=ON ../ + cmake -DCMAKE_BUILD_TYPE=Debug ../ - name: build_tag run: | cd tag diff --git a/.github/workflows/android.yaml b/.github/workflows/android.yaml index 0af85fda3..5e11c75df 100644 --- a/.github/workflows/android.yaml +++ b/.github/workflows/android.yaml @@ -9,7 +9,7 @@ on: jobs: build: name: NDK-R23 - runs-on: ubuntu-20.04 + runs-on: ubuntu-latest steps: - name: Setup Android NDK R23 diff --git a/.github/workflows/cxx11-ubuntu.yaml b/.github/workflows/cxx11-ubuntu.yaml index 7d30f4a1a..2f362d84f 100644 --- a/.github/workflows/cxx11-ubuntu.yaml +++ b/.github/workflows/cxx11-ubuntu.yaml @@ -9,7 +9,7 @@ on: jobs: build: name: ubuntu - runs-on: ubuntu-20.04 + runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 - name: configure @@ -17,7 +17,7 @@ jobs: mkdir _build && cd _build cmake ../ -DCMAKE_COMPILE_WARNING_AS_ERROR=ON -DENABLE_STDCXX_SYNC=ON -DENABLE_ENCRYPTION=ON -DENABLE_UNITTESTS=ON -DENABLE_BONDING=ON -DENABLE_TESTING=ON -DENABLE_EXAMPLES=ON -DENABLE_CODE_COVERAGE=ON -DCMAKE_EXPORT_COMPILE_COMMANDS=ON - name: build - run: cd _build && build-wrapper-linux-x86-64 --out-dir ${{ env.BUILD_WRAPPER_OUT_DIR }} cmake --build . + run: cd _build && cmake --build . - name: test run: | cd _build && ctest --extra-verbose diff --git a/scripts/googletest-download.cmake b/scripts/googletest-download.cmake index 7469f4f7b..36554f316 100644 --- a/scripts/googletest-download.cmake +++ b/scripts/googletest-download.cmake @@ -11,7 +11,7 @@ ExternalProject_Add( BINARY_DIR "@GOOGLETEST_DOWNLOAD_ROOT@/googletest-build" GIT_REPOSITORY https://github.com/google/googletest.git - GIT_TAG release-1.10.0 + GIT_TAG release-1.12.1 CONFIGURE_COMMAND "" BUILD_COMMAND "" INSTALL_COMMAND "" From 265d0e495f5a44e762b65ceb60b9fa0e7a698b73 Mon Sep 17 00:00:00 2001 From: Sektor van Skijlen Date: Fri, 29 Aug 2025 13:10:58 +0200 Subject: [PATCH 05/19] [core] Fixed invalid cookie contest calculation (#3212) Co-authored-by: Mikolaj Malecki --- srtcore/core.cpp | 41 ++++++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/srtcore/core.cpp b/srtcore/core.cpp index 344f21ac1..050caaa17 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -4107,13 +4107,13 @@ void srt::CUDT::cookieContest() if (m_SrtHsSide != HSD_DRAW) return; - LOGC(cnlog.Debug, - log << CONID() << "cookieContest: agent=" << m_ConnReq.m_iCookie << " peer=" << m_ConnRes.m_iCookie); - // Here m_ConnReq.m_iCookie is a local cookie value sent in connection request to the peer. // m_ConnRes.m_iCookie is a cookie value sent by the peer in its connection request. if (m_ConnReq.m_iCookie == 0 || m_ConnRes.m_iCookie == 0) { + LOGC(cnlog.Debug, log << CONID() << "cookieContest: agent=" << m_ConnReq.m_iCookie << " peer=" << m_ConnRes.m_iCookie + << " - ERROR: zero not allowed!"); + // Note that it's virtually impossible that Agent's cookie is not ready, this // shall be considered IPE. // Not all cookies are ready, don't start the contest. @@ -4134,11 +4134,34 @@ void srt::CUDT::cookieContest() // int32_t iBetterCookie = 2002790373 (7760 27E5); // // Now 64-bit arithmetic is used to calculate the actual result of subtraction. - // The 31-st bit is then checked to check if the resulting is negative in 32-bit aritmetics. - // This way the old contest behavior is preserved, and potential compiler optimisations are avoided. - const int64_t contest = int64_t(m_ConnReq.m_iCookie) - int64_t(m_ConnRes.m_iCookie); - - if ((contest & 0xFFFFFFFF) == 0) + // + // In SRT v1.5.4 there was a version, that checked: + // - if LOWER 32-bits are 0, this is a draw + // - if bit 31 is set (AND with 0x80000000), the result is considered negative. + // This was erroneous because for 1 and 0x80000001 cookie values the + // result was always the same, regardless of the order: + // + // 0x0000000000000001 - 0xFFFFFFFF80000001 = 0x0000000080000000 + // 0xFFFFFFFF80000001 - 0x0000000080000000 = 0xFFFFFFFF80000000 + // + // >> if (contest & 0x80000000) -> results in true in both comparisons. + // + // This version takes the bare result of the 64-bit arithmetics. + const int64_t xreq = int64_t(m_ConnReq.m_iCookie); + const int64_t xres = int64_t(m_ConnRes.m_iCookie); + const int64_t contest = xreq - xres; + + // NOTE: for 1.6.0 use: + // fmtc hex64 = fmtc().uhex().fillzero().width(16); then + // << fmt(xreq, hex64) + LOGC(cnlog.Debug, log << CONID() << "cookieContest: agent=" << m_ConnReq.m_iCookie + << " peer=" << m_ConnRes.m_iCookie + << hex << uppercase << setfill('0') // PERSISTENT flags + << " X64: "<< setw(16) << xreq + << " vs. " << setw(16) << xres + << " DIFF: " << setw(16) << contest); + + if (contest == 0) { // DRAW! The only way to continue would be to force the // cookies to be regenerated and to start over. But it's @@ -4154,7 +4177,7 @@ void srt::CUDT::cookieContest() return; } - if (contest & 0x80000000) + if (contest < 0) { m_SrtHsSide = HSD_RESPONDER; return; From 5138f37d45b5724e379e27760dc7e30bcfa110f1 Mon Sep 17 00:00:00 2001 From: cl-ment Date: Fri, 29 Aug 2025 13:15:35 +0200 Subject: [PATCH 06/19] [core] Cleanup SRT state after a fork() (issue #3177) (#3179) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [core] Cleanup SRT state after a fork() (issue #3177) * Free socket memory without calling the destructor. * Remove srt_cleanupAtFork() from the API. * Make it compile on systems that don't support pthread_atfork() * Remove a typo * Avoid to send shutdown packet when cleaning up after a fork. * Close the dangling UDP sockets, Free memory. * Add TODO for freeing the Send Queue after refacttoring it. * Ensure that CThread is joinable before join() * Try fix iOS-cxxsyncOFF * Replace the mutex pointer by a mutex reference. * Refactor the Multiplexer initialization. * Fix SIGSEGV * Fix Compilation error on a Debug Log * Rollback to cleaner code for the multiplexer initialization. * Add compatibility with C++11 Sync. * Apply code review changes * Replace the resetThread() macro by an inline function. Co-authored-by: Sektor van Skijlen * Reset m_CGStopCond in CUDTUnited * Rework of resetThread() * Ensure Garbage collector is in the right state after fork. * Protect ~CMultiplexer() against NULL pointers. * Protect resetAtFork() and stop() against NULL pointers. * Added fork example to the repository * Remove french comments. --------- Co-authored-by: Clément Gérouville Co-authored-by: Sektor van Skijlen Co-authored-by: Mikolaj Malecki --- CMakeLists.txt | 10 ++- examples/fork-test/Makefile | 9 +++ examples/fork-test/README.md | 7 ++ examples/fork-test/srt_client.c | 55 +++++++++++++++ examples/fork-test/srt_server.c | 118 ++++++++++++++++++++++++++++++++ srtcore/api.cpp | 90 ++++++++++++++++++++++-- srtcore/api.h | 3 + srtcore/core.cpp | 12 ++++ srtcore/core.h | 3 + srtcore/queue.cpp | 74 +++++++++++++++----- srtcore/queue.h | 18 +++++ srtcore/sync.h | 6 +- srtcore/sync_cxx11.cpp | 18 +++++ srtcore/sync_posix.cpp | 39 +++++++---- 14 files changed, 426 insertions(+), 36 deletions(-) create mode 100644 examples/fork-test/Makefile create mode 100644 examples/fork-test/README.md create mode 100644 examples/fork-test/srt_client.c create mode 100644 examples/fork-test/srt_server.c diff --git a/CMakeLists.txt b/CMakeLists.txt index 0acd39b41..2017139ed 100755 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -966,10 +966,16 @@ else () message(STATUS "Pthread library: ${PTHREAD_LIBRARY}") endif() +list(PREPEND CMAKE_REQUIRED_LIBRARIES "${PTHREAD_LIBRARY}") +unset(CMAKE_REQUIRED_QUIET) + +check_symbol_exists(pthread_atfork "pthread.h" HAVE_PTHREAD_ATFORK) +if ("${HAVE_PTHREAD_ATFORK}" STREQUAL "1") + add_definitions(-DHAVE_PTHREAD_ATFORK=1) +endif () + # To avoid the need for other judgments when ENABLE_STDCXX_SYNC is OFF in the future, this is a separate conditional statement. if (NOT ENABLE_STDCXX_SYNC AND ENABLE_MONOTONIC_CLOCK) - list(PREPEND CMAKE_REQUIRED_LIBRARIES "${PTHREAD_LIBRARY}") - unset(CMAKE_REQUIRED_QUIET) check_symbol_exists(pthread_condattr_setclock "pthread.h" HAVE_PTHREAD_CONDATTR_SETCLOCK) message(STATUS "Checking pthread_condattr_setclock: '${HAVE_PTHREAD_CONDATTR_SETCLOCK}'") if ("${HAVE_PTHREAD_CONDATTR_SETCLOCK}" STREQUAL "1") diff --git a/examples/fork-test/Makefile b/examples/fork-test/Makefile new file mode 100644 index 000000000..57e9f3a94 --- /dev/null +++ b/examples/fork-test/Makefile @@ -0,0 +1,9 @@ +TARGETS=srt_server srt_client + +all: $(TARGETS) + +%: %.c + $(CC) $< `pkg-config --cflags --libs srt` -o `basename $< .c` + +clean: + rm -f $(TARGETS) diff --git a/examples/fork-test/README.md b/examples/fork-test/README.md new file mode 100644 index 000000000..59752ad02 --- /dev/null +++ b/examples/fork-test/README.md @@ -0,0 +1,7 @@ +The `srt_server` and `srt_client` apps should be compiled using +external installation of SRT (e.g. in the local directory), each +one as a single program. This is not compiled as a part of SRT. + +If you want to use a local installation, simply set `PKG_CONFIG_PATH` +environment variable to point to the local installation directory with +"lib/pkgconfig" or "lib64/pkgconfig" suffix. diff --git a/examples/fork-test/srt_client.c b/examples/fork-test/srt_client.c new file mode 100644 index 000000000..ab69d5956 --- /dev/null +++ b/examples/fork-test/srt_client.c @@ -0,0 +1,55 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#define SERVER_IP "127.0.0.1" +#define SERVER_PORT 9000 +int main() { + if (srt_startup() != 0) { + fprintf(stderr, "Error initializing SRT.\n"); + return 1; + } + SRTSOCKET client_sock = srt_create_socket(); + if (client_sock == SRT_INVALID_SOCK) { + fprintf(stderr, "Error creating a socket: %s\n", srt_getlasterror_str()); + return 1; + } + struct sockaddr_in sa; + memset(&sa, 0, sizeof sa); + sa.sin_family = AF_INET; + sa.sin_port = htons(SERVER_PORT); + inet_pton(AF_INET, SERVER_IP, &sa.sin_addr); + if (srt_connect(client_sock, (struct sockaddr*)&sa, sizeof(sa)) == SRT_ERROR) { + fprintf(stderr, "Error: srt_connect: %s\n", srt_getlasterror_str()); + return 1; + } + printf("Connected to SRT server %s:%d\n", SERVER_IP, SERVER_PORT); + const char* message = "Hello from SRT client!"; + int bytes = srt_send(client_sock, message, strlen(message)); + if (bytes == SRT_ERROR) { + fprintf(stderr, "Sending error: %s\n", srt_getlasterror_str()); + } else { + printf("Message sent: %s\n", message); + } + + while (1) + { + char buffer[1500]; + int nb = srt_recv(client_sock, buffer, sizeof(buffer)); + if (nb <= 0) + { + printf("Closed from the server !\n"); + srt_close(client_sock); + break; + } + buffer[nb] = 0; + printf("Server has sent: %s\n", buffer); + } + srt_cleanup(); + return 0; +} + diff --git a/examples/fork-test/srt_server.c b/examples/fork-test/srt_server.c new file mode 100644 index 000000000..a680f1966 --- /dev/null +++ b/examples/fork-test/srt_server.c @@ -0,0 +1,118 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#define PORT 9000 + +int run(char *command) { + pid_t pid = fork(); + if (pid < 0) { + perror("fork (intermediate)"); + exit(EXIT_FAILURE); + } + if (pid > 0) { + // Parent process + printf("[GRANDPARENT %d] waiting for grand-child process pid=%d to finish...\n", + getpid(), pid); + waitpid(pid, NULL, 0); // Wait for intermediate child + printf("[GRANDPARENT] returning\n"); + return 0; + } + // Intermediate process + //srt_cleanup(); + if (setsid() < 0) { + perror("setsid"); + exit(EXIT_FAILURE); + } + pid_t grandchild_pid = fork(); + if (grandchild_pid < 0) { + perror("fork (grandchild)"); + exit(EXIT_FAILURE); + } + if (grandchild_pid > 0) { + printf("[PARENT %d] waiting for 10s with child process pid=%d ...\n", + getpid(), grandchild_pid); + // Intermediate process exits immediately + sleep(10); + printf("[PARENT] exitting\n"); + exit(0); + } + // Grandchild process + // Redirect stdin to /dev/null + printf("[CHILD %d] Preparing descriptors...\n", getpid()); + int devnull = open("/dev/null", O_RDONLY); + if (devnull >= 0) { + dup2(devnull, STDIN_FILENO); + close(devnull); + } else { + perror("open /dev/null"); + } + // Redirect stdout to stderr + dup2(STDERR_FILENO, STDOUT_FILENO); + // Execute the command + printf("[CHILD] Executing process '%s'...\n", command); + execl("/bin/sh", "sh", "-c", command, (char *)NULL); + // If execl fails + perror("execl"); + exit(EXIT_FAILURE); +} + +int main() { + if (srt_startup() != 0) { + fprintf(stderr, "Error initializing SRT.\n"); + return 1; + } + + SRTSOCKET serv_sock = srt_create_socket(); + if (serv_sock == SRT_INVALID_SOCK) { + fprintf(stderr, "Error creating SRT socket: %s\n", srt_getlasterror_str()); + return 1; + } + struct sockaddr_in sa; + memset(&sa, 0, sizeof sa); + sa.sin_family = AF_INET; + sa.sin_port = htons(PORT); + sa.sin_addr.s_addr = INADDR_ANY; + if (srt_bind(serv_sock, (struct sockaddr*)&sa, sizeof sa) == SRT_ERROR) { + fprintf(stderr, "Error: srt_bind: %s\n", srt_getlasterror_str()); + return 1; + } + if (srt_listen(serv_sock, 5) == SRT_ERROR) { + fprintf(stderr, "Error: srt_listen: %s\n", srt_getlasterror_str()); + return 1; + } + printf("SRT server is listening on port %d...\n", PORT); + struct sockaddr_in client_addr; + int addr_len = sizeof(client_addr); + SRTSOCKET client_sock = srt_accept(serv_sock, (struct sockaddr*)&client_addr, &addr_len); + if (client_sock == SRT_INVALID_SOCK) { + fprintf(stderr, "Error: srt_accept: %s\n", srt_getlasterror_str()); + return 1; + } + printf("Client connected via SRT !\n"); + char buffer[1500]; + int bytes = srt_recv(client_sock, buffer, sizeof(buffer)); + if (bytes > 0) { + buffer[bytes] = '\0'; + printf("Message received: %s\n", buffer); + const char resp [] = "We read you!"; + srt_send(client_sock, resp, (sizeof resp)-1); + } else { + printf("Error: reading from srt_recv: %s.\n", srt_getlasterror_str()); + } + run("date > /tmp/res"); + printf("Server: sleep(1)\n"); + sleep(1); + // Nettoyage + printf("Server: closing SRT sockets\n"); + srt_close(client_sock); + srt_close(serv_sock); + printf("Server: cleanup\n"); + srt_cleanup(); + printf("Server: exit\n"); + return 0; +} diff --git a/srtcore/api.cpp b/srtcore/api.cpp index da929ef5a..22b093dcc 100644 --- a/srtcore/api.cpp +++ b/srtcore/api.cpp @@ -99,6 +99,12 @@ srt::CUDTSocket::~CUDTSocket() releaseMutex(m_ControlLock); } +void srt::CUDTSocket::resetAtFork() +{ + m_UDT.resetAtFork(); + resetCond(m_AcceptCond); +} + SRT_SOCKSTATUS srt::CUDTSocket::getStatus() { // TTL in CRendezvousQueue::updateConnStatus() will set m_bConnecting to false. @@ -275,6 +281,51 @@ void srt::CUDTUnited::stopGarbageCollector() } } +void srt::CUDTUnited::cleanupAllSockets() +{ + for (sockets_t::iterator i = m_Sockets.begin(); i != m_Sockets.end(); ++i) + { + CUDTSocket* s = i->second; + +#if ENABLE_BONDING + if (s->m_GroupOf) + { + s->removeFromGroup(false); + } +#endif + + // remove from listener's queue + sockets_t::iterator ls = m_Sockets.find(s->m_ListenSocket); + if (ls == m_Sockets.end()) + { + ls = m_ClosedSockets.find(s->m_ListenSocket); + } + if (ls != m_ClosedSockets.end()) + { + ls->second->m_QueuedSockets.erase(s->m_SocketID); + } + s->core().closeAtFork(); + s->resetAtFork(); + delete(s); + } + m_Sockets.clear(); + +#if ENABLE_BONDING + for (groups_t::iterator j = m_Groups.begin(); j != m_Groups.end(); ++j) + { + delete j->second; + } + m_Groups.clear(); +#endif + for (map::iterator i = m_mMultiplexer.begin(); i != m_mMultiplexer.end(); ++i) + { + CMultiplexer &multiplexer = i->second; + multiplexer.resetAtFork(); + } + m_mMultiplexer.clear(); +} + + void srt::CUDTUnited::closeAllSockets() { // remove all sockets and multiplexers @@ -370,6 +421,18 @@ int srt::CUDTUnited::startup() return startGarbageCollector() ? 0 : -1; } +int srt::CUDTUnited::cleanupAtFork() +{ + cleanupAllSockets(); + resetThread(&m_GCThread); + resetCond(m_GCStopCond); + m_GCStopLock.unlock(); + setupCond(m_GCStopCond, "GCStop"); + m_iInstanceCount=0; + m_bGCStatus = false; + return 0; +} + int srt::CUDTUnited::cleanup() { // IMPORTANT!!! @@ -3076,8 +3139,9 @@ bool srt::CUDTUnited::channelSettingsMatch(const CSrtMuxerConfig& cfgMuxer, cons void srt::CUDTUnited::updateMux(CUDTSocket* s, const sockaddr_any& reqaddr, const UDPSOCKET* udpsock /*[[nullable]]*/) { + const int port = reqaddr.hport(); ExclusiveLock cg(m_GlobControlLock); - + // If udpsock is provided, then this socket will be simply // taken for binding as a good deal. It would be nice to make // a sanity check to see if this UDP socket isn't already installed @@ -3087,7 +3151,6 @@ void srt::CUDTUnited::updateMux(CUDTSocket* s, const sockaddr_any& reqaddr, cons { // If not, we need to see if there exist already a multiplexer bound // to the same endpoint. - const int port = reqaddr.hport(); const CSrtConfig& cfgSocket = s->core().m_config; // This loop is going to check the attempted binding of @@ -3305,6 +3368,8 @@ void srt::CUDTUnited::updateMux(CUDTSocket* s, const sockaddr_any& reqaddr, cons } } + + // a new multiplexer is needed CMultiplexer m; configureMuxer((m), s, reqaddr.family()); @@ -3360,7 +3425,7 @@ void srt::CUDTUnited::updateMux(CUDTSocket* s, const sockaddr_any& reqaddr, cons // Rewrite the port here, as it might be only known upon return // from CChannel::open. m.m_iPort = installMuxer((s), m); - m_mMultiplexer[m.m_iID] = m; + swap(m_mMultiplexer[m.m_iID],m); } catch (const CUDTException&) { @@ -3373,7 +3438,7 @@ void srt::CUDTUnited::updateMux(CUDTSocket* s, const sockaddr_any& reqaddr, cons throw CUDTException(MJ_SYSTEMRES, MN_MEMORY, 0); } - HLOGC(smlog.Debug, log << "bind: creating new multiplexer for port " << m.m_iPort); + HLOGC(smlog.Debug, log << "bind: creating new multiplexer for port " << port); } // This function is going to find a multiplexer for the port contained @@ -3492,6 +3557,14 @@ void* srt::CUDTUnited::garbageCollect(void* p) int srt::CUDT::startup() { +#if HAVE_PTHREAD_ATFORK + static bool registered = false; + if (!registered) + { + pthread_atfork(NULL, NULL, (void (*)()) srt::CUDT::cleanupAtFork); + registered = true; + } +#endif return uglobal().startup(); } @@ -3500,6 +3573,15 @@ int srt::CUDT::cleanup() return uglobal().cleanup(); } +int srt::CUDT::cleanupAtFork() +{ + CUDTUnited &context = uglobal(); + context.cleanupAtFork(); + new (&context) CUDTUnited(); + + return context.startup(); +} + SRTSOCKET srt::CUDT::socket() { try diff --git a/srtcore/api.h b/srtcore/api.h index d3f8427d9..42b97ef32 100644 --- a/srtcore/api.h +++ b/srtcore/api.h @@ -120,6 +120,7 @@ class CUDTSocket } ~CUDTSocket(); + void resetAtFork(); void construct(); @@ -263,6 +264,7 @@ class CUDTUnited /// release the UDT library. /// @return 0 if success, otherwise -1 is returned. int cleanup(); + int cleanupAtFork(); /// Create a new UDT socket. /// @param [out] pps Variable (optional) to which the new socket will be written, if succeeded @@ -464,6 +466,7 @@ class CUDTUnited bool acquireSocket(CUDTSocket* s); bool startGarbageCollector(); void stopGarbageCollector(); + void cleanupAllSockets(); void closeAllSockets(); public: diff --git a/srtcore/core.cpp b/srtcore/core.cpp index 050caaa17..0acef17bb 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -6513,6 +6513,12 @@ bool srt::CUDT::closeInternal() ATR_NOEXCEPT return true; } +bool srt::CUDT::closeAtFork() ATR_NOEXCEPT +{ + m_bShutdown = true; + return closeInternal(); +} + int srt::CUDT::receiveBuffer(char *data, int len) { if (!m_CongCtl->checkTransArgs(SrtCongestion::STA_BUFFER, SrtCongestion::STAD_RECV, data, len, SRT_MSGTTL_INF, false)) @@ -7939,6 +7945,12 @@ void srt::CUDT::destroySynch() m_RcvTsbPdCond.notify_all(); releaseCond(m_RcvTsbPdCond); } +void srt::CUDT::resetAtFork() +{ + resetCond(m_SendBlockCond); + resetCond(m_RecvDataCond); + resetCond(m_RcvTsbPdCond); +} void srt::CUDT::releaseSynch() { diff --git a/srtcore/core.h b/srtcore/core.h index 00fd44923..a07d17277 100644 --- a/srtcore/core.h +++ b/srtcore/core.h @@ -197,6 +197,7 @@ class CUDT public: //API static int startup(); static int cleanup(); + static int cleanupAtFork(); static SRTSOCKET socket(); #if ENABLE_BONDING static SRTSOCKET createGroup(SRT_GROUP_TYPE); @@ -604,6 +605,7 @@ class CUDT /// Close the opened UDT entity. bool closeInternal() ATR_NOEXCEPT; + bool closeAtFork() ATR_NOEXCEPT; void updateBrokenConnection(); void completeBrokenConnectionDependencies(int errorcode); @@ -1045,6 +1047,7 @@ class CUDT void initSynch(); void destroySynch(); + void resetAtFork(); void releaseSynch(); private: // Common connection Congestion Control setup diff --git a/srtcore/queue.cpp b/srtcore/queue.cpp index f8b782174..88c0d30f6 100644 --- a/srtcore/queue.cpp +++ b/srtcore/queue.cpp @@ -220,6 +220,11 @@ srt::CSndUList::~CSndUList() delete[] m_pHeap; } +void srt::CSndUList::resetAtFork() +{ + resetCond(m_ListCond); +} + void srt::CSndUList::update(const CUDT* u, EReschedule reschedule, sync::steady_clock::time_point ts) { ScopedLock listguard(m_ListLock); @@ -414,6 +419,17 @@ srt::CSndQueue::CSndQueue() } srt::CSndQueue::~CSndQueue() +{ + delete m_pSndUList; +} + +void srt::CSndQueue::resetAtFork() +{ + resetThread(&m_WorkerThread); + m_pSndUList->resetAtFork(); +} + +void srt::CSndQueue::stop() { m_bClosing = true; @@ -430,8 +446,6 @@ srt::CSndQueue::~CSndQueue() HLOGC(rslog.Debug, log << "SndQueue: EXIT"); m_WorkerThread.join(); } - - delete m_pSndUList; } int srt::CSndQueue::ioctlQuery(int type) const @@ -1168,15 +1182,6 @@ srt::CRcvQueue::CRcvQueue() srt::CRcvQueue::~CRcvQueue() { - m_bClosing = true; - - if (m_WorkerThread.joinable()) - { - HLOGC(rslog.Debug, log << "RcvQueue: EXIT"); - m_WorkerThread.join(); - } - releaseCond(m_BufferCond); - delete m_pUnitQueue; delete m_pRcvUList; delete m_pHash; @@ -1194,6 +1199,24 @@ srt::CRcvQueue::~CRcvQueue() } } +void srt::CRcvQueue::resetAtFork() +{ + resetThread(&m_WorkerThread); +} + +void srt::CRcvQueue::stop() +{ + m_bClosing = true; + + if (m_WorkerThread.joinable()) + { + HLOGC(rslog.Debug, log << "RcvQueue: EXIT"); + m_WorkerThread.join(); + } + releaseCond(m_BufferCond); +} + + #if ENABLE_LOGGING srt::sync::atomic srt::CRcvQueue::m_counter(0); #endif @@ -1819,16 +1842,35 @@ void srt::CRcvQueue::storePktClone(int32_t id, const CPacket& pkt) } } -void srt::CMultiplexer::destroy() +void srt::CMultiplexer::resetAtFork() { - // Reverse order of the assigned. - delete m_pRcvQueue; - delete m_pSndQueue; - delete m_pTimer; + if (m_pRcvQueue != NULL) + m_pRcvQueue->resetAtFork(); + if (m_pSndQueue != NULL) + m_pSndQueue->resetAtFork(); +} +void srt::CMultiplexer::close() +{ if (m_pChannel) { m_pChannel->close(); delete m_pChannel; + m_pChannel = NULL; } } + +void srt::CMultiplexer::stop() +{ + if (m_pRcvQueue != NULL) + m_pRcvQueue->stop(); + if (m_pSndQueue != NULL) + m_pSndQueue->stop(); +} + +void srt::CMultiplexer::destroy() +{ + // Reverse order of the assigned. + stop(); + close(); +} diff --git a/srtcore/queue.h b/srtcore/queue.h index 0d23790e9..cf3109767 100644 --- a/srtcore/queue.h +++ b/srtcore/queue.h @@ -157,6 +157,7 @@ class CSndUList }; static EReschedule rescheduleIf(bool cond) { return cond ? DO_RESCHEDULE : DONT_RESCHEDULE; } + void resetAtFork(); /// Update the timestamp of the UDT instance on the list. /// @param [in] u pointer to the UDT instance @@ -401,6 +402,7 @@ class CSndQueue ~CSndQueue(); public: + void resetAtFork(); // XXX There's currently no way to access the socket ID set for // whatever the queue is currently working for. Required to find // some way to do this, possibly by having a "reverse pointer". @@ -439,6 +441,7 @@ class CSndQueue int sockoptQuery(int level, int type) const; void setClosing() { m_bClosing = true; } + void stop(); private: static void* worker(void* param); @@ -486,6 +489,7 @@ class CRcvQueue ~CRcvQueue(); public: + void resetAtFork(); // XXX There's currently no way to access the socket ID set for // whatever the queue is currently working. Required to find // some way to do this, possibly by having a "reverse pointer". @@ -513,6 +517,7 @@ class CRcvQueue int getIPversion() { return m_iIPversion; } + void stop(); private: static void* worker(void* param); sync::CThread m_WorkerThread; @@ -599,6 +604,19 @@ struct CMultiplexer { } + ~CMultiplexer() + { + if (m_pRcvQueue != NULL) + delete m_pRcvQueue; + if (m_pSndQueue != NULL) + delete m_pSndQueue; + if (m_pTimer != NULL) + delete m_pTimer; + close(); + } + void resetAtFork(); + void close(); + void stop(); void destroy(); }; diff --git a/srtcore/sync.h b/srtcore/sync.h index 100a11eed..72da5869b 100644 --- a/srtcore/sync.h +++ b/srtcore/sync.h @@ -429,11 +429,11 @@ class Condition public: Condition(); ~Condition(); - public: /// These functions do not align with C++11 version. They are here hopefully as a temporal solution /// to avoud issues with static initialization of CV on windows. void init(); + void reset(); void destroy(); public: @@ -486,6 +486,7 @@ class Condition }; inline void setupCond(Condition& cv, const char*) { cv.init(); } +inline void resetCond(Condition& cv) { cv.reset(); } inline void releaseCond(Condition& cv) { cv.destroy(); } /////////////////////////////////////////////////////////////////////////////// @@ -1002,6 +1003,7 @@ class CThread private: pthread_t m_thread; + pid_t m_pid; }; template @@ -1033,6 +1035,8 @@ namespace this_thread #endif +inline void resetThread(CThread* th) { (void)new (th) CThread; } + /// StartThread function should be used to do CThread assignments: /// @code /// CThread a(); diff --git a/srtcore/sync_cxx11.cpp b/srtcore/sync_cxx11.cpp index bdfbf9a4e..c376b0adf 100644 --- a/srtcore/sync_cxx11.cpp +++ b/srtcore/sync_cxx11.cpp @@ -58,6 +58,24 @@ srt::sync::Condition::~Condition() {} void srt::sync::Condition::init() {} +void srt::sync::Condition::reset() +{ + // SRT attempts to safely handle `fork()` in multithreaded environments, + // even though using `fork()` in such contexts is strongly discouraged. + // This is because `fork()` only duplicates the calling thread, leaving + // synchronization primitives (like condition variables) in an + // undefined or inconsistent state in the child process. + // + // To mitigate this, SRT forcefully reinitializes these synchronization + // primitives post-fork. In POSIX, this is done by overwriting the object + // with its default-initialized state. In C++11, we achieve the same effect + // using *placement new* to reconstruct the object in place. This ensures + // the condition variable is returned to a fresh, "neutral" state, + // as if it was just created. + + new (&m_cv) std::condition_variable; +} + void srt::sync::Condition::destroy() {} void srt::sync::Condition::wait(UniqueLock& lock) diff --git a/srtcore/sync_posix.cpp b/srtcore/sync_posix.cpp index 164913097..09def5c33 100644 --- a/srtcore/sync_posix.cpp +++ b/srtcore/sync_posix.cpp @@ -309,6 +309,14 @@ void Condition::init() throw std::runtime_error("pthread_cond_init monotonic failed"); } +void Condition::reset() +{ + + pthread_cond_t temp = PTHREAD_COND_INITIALIZER; + memcpy(&m_cv, (void *) &temp, sizeof(m_cv)); + init(); +} + void Condition::destroy() { pthread_cond_destroy(&m_cv); @@ -370,6 +378,7 @@ void Condition::notify_all() srt::sync::CThread::CThread() { m_thread = pthread_t(); + m_pid = getpid(); } srt::sync::CThread::CThread(void *(*start_routine) (void *), void *arg) @@ -402,9 +411,9 @@ srt::sync::CThread& srt::sync::CThread::operator=(CThread& other) join(); #endif } - // Move thread handler from other m_thread = other.m_thread; + m_pid = other.m_pid; other.m_thread = pthread_t(); return *this; } @@ -419,24 +428,27 @@ void srt::sync::CThread::create_thread(void *(*start_routine) (void *), void *ar bool srt::sync::CThread::joinable() const { - return !pthread_equal(m_thread, pthread_t()); + return m_pid == getpid() && !pthread_equal(m_thread, pthread_t()); } void srt::sync::CThread::join() { - void *retval; - const int ret SRT_ATR_UNUSED = pthread_join(m_thread, &retval); - if (ret != 0) - { - LOGC(inlog.Error, log << "pthread_join failed with " << ret); - } + if (joinable()) + { + void *retval; + const int ret SRT_ATR_UNUSED = pthread_join(m_thread, &retval); + if (ret != 0) + { + LOGC(inlog.Error, log << "pthread_join failed with " << ret << " (" << m_thread << ")"); + } #ifdef HEAVY_LOGGING - else - { - HLOGC(inlog.Debug, log << "pthread_join SUCCEEDED"); - } + else + { + HLOGC(inlog.Debug, log << "pthread_join SUCCEEDED"); + } #endif - // After joining, joinable should be false + // After joining, joinable should be false + } m_thread = pthread_t(); return; } @@ -449,6 +461,7 @@ void srt::sync::CThread::create(void *(*start_routine) (void *), void *arg) LOGC(inlog.Error, log << "pthread_create failed with " << st); throw CThreadException(MJ_SYSTEMRES, MN_THREAD, 0); } + m_pid = getpid(); } From 9b97cfa235c1778262325e84d080aad8ac34de89 Mon Sep 17 00:00:00 2001 From: Sektor van Skijlen Date: Mon, 1 Sep 2025 14:28:01 +0200 Subject: [PATCH 07/19] [core] Fixed reentrancy problem of srt_strerror (#3214) * [core] Fixed reentrancy problem of srt_strerror * [docs] Updated documentation for srt_strerror --------- Co-authored-by: Mikolaj Malecki --- docs/API/API-functions.md | 12 ++++++------ srtcore/srt_c_api.cpp | 14 ++++++++++---- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/docs/API/API-functions.md b/docs/API/API-functions.md index 2fdb8a661..928fd6aaa 100644 --- a/docs/API/API-functions.md +++ b/docs/API/API-functions.md @@ -2841,13 +2841,13 @@ associated with the last error. The system error is: const char* srt_strerror(int code, int errnoval); ``` -Returns a string message that represents a given SRT error code and possibly the -`errno` value, if not 0. +Returns a string message that represents a given SRT error code. -**NOTE:** *This function isn't thread safe. It uses a static variable to hold the -error description. There's no problem with using it in a multithreaded environment, -as long as only one thread in the whole application calls this function at the -moment* +**NOTE:** *The `errnoval` parameter is ignored. This function's old version +was intended to get both the SRT error description and system error description, +but this requires resolution of the reentrancy problem and dynamic strings. +For getting the error description for a system error, you need to use the +`strerror` function or some of its reentrant version.* [:arrow_up:   Back to List of Functions & Structures](#srt-api-functions) diff --git a/srtcore/srt_c_api.cpp b/srtcore/srt_c_api.cpp index 031030cd7..cf82467c2 100644 --- a/srtcore/srt_c_api.cpp +++ b/srtcore/srt_c_api.cpp @@ -26,6 +26,14 @@ written by using namespace std; using namespace srt; +namespace srt +{ +// This is provided in strerror_defs.cpp, which doesn't have +// its header file. +// XXX Consider adding some static function to CUDTException. +const char* strerror_get_message(size_t major, size_t minor); +} + extern "C" { @@ -248,11 +256,9 @@ int srt_getlasterror(int* loc_errno) return CUDT::getlasterror().getErrorCode(); } -const char* srt_strerror(int code, int err) +const char* srt_strerror(int code, int /*err ignored*/) { - static srt::CUDTException e; - e = srt::CUDTException(CodeMajor(code/1000), CodeMinor(code%1000), err); - return(e.getErrorMessage()); + return strerror_get_message(CodeMajor(code/1000), CodeMinor(code%1000)); } From 52ceecdf5190885914f0f94d01be32441ccb1f4c Mon Sep 17 00:00:00 2001 From: Aleksey S P <35438102+SPAleksey@users.noreply.github.com> Date: Mon, 1 Sep 2025 17:14:14 +0400 Subject: [PATCH 08/19] Fix SIGABRT when bonding option is too long (#3210) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix SIGABRT when bonding option is too long * Move the UT to test_bonding.cpp --------- Co-authored-by: Clément Gérouville --- srtcore/socketconfig.cpp | 2 +- test/test_bonding.cpp | 9 +++++++++ test/test_utilities.cpp | 1 + 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/srtcore/socketconfig.cpp b/srtcore/socketconfig.cpp index a0c8a6a47..1b94c9197 100644 --- a/srtcore/socketconfig.cpp +++ b/srtcore/socketconfig.cpp @@ -1092,7 +1092,7 @@ bool SRT_SocketOptionObject::add(SRT_SOCKOPT optname, const void* optval, size_t // needed - and it's better to not risk that alighment rules // will make these calculations result in less space than needed. const size_t headersize = sizeof(SingleOption); - const size_t payload = std::min(sizeof(uint32_t), optlen); + const size_t payload = std::max(sizeof(uint32_t), optlen); unsigned char* mem = new unsigned char[headersize + payload]; SingleOption* option = reinterpret_cast(mem); option->option = optname; diff --git a/test/test_bonding.cpp b/test/test_bonding.cpp index 6307fd21f..982145fae 100644 --- a/test/test_bonding.cpp +++ b/test/test_bonding.cpp @@ -1594,3 +1594,12 @@ TEST(Bonding, BackupPrioritySelection) } +TEST(Bonding, ApiConfig) +{ + using namespace std; + SRT_SOCKOPT_CONFIG config; + + string example = "example_long_excessively"; + + EXPECT_EQ(srt_config_add(&config, SRTO_BINDTODEVICE, (void*)example.data(), example.size()), 0); +} diff --git a/test/test_utilities.cpp b/test/test_utilities.cpp index 28c14044e..e8494e744 100644 --- a/test/test_utilities.cpp +++ b/test/test_utilities.cpp @@ -272,3 +272,4 @@ TEST(ConfigString, Setting) EXPECT_EQ(s.size(), 0U); EXPECT_TRUE(s.empty()); } + From bab403744b4d02005b03dac12a79988bc4590038 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20G=C3=A9rouville?= Date: Wed, 3 Sep 2025 09:43:59 +0200 Subject: [PATCH 09/19] [API] raise libSRT version number to 1.5.5 --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 2017139ed..a7e979513 100755 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -8,7 +8,7 @@ # cmake_minimum_required (VERSION 3.5 FATAL_ERROR) -set (SRT_VERSION 1.5.4) +set (SRT_VERSION 1.5.5) set (CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/scripts") include(CheckSymbolExists) From 5c5f5b5f3ac52c1d453c98bb945b6a39309a32cb Mon Sep 17 00:00:00 2001 From: Sektor van Skijlen Date: Wed, 15 Oct 2025 13:23:41 +0200 Subject: [PATCH 10/19] [MAINT] Add fixes for CI configuration based on latest dev (#3232) * [MAINT] Add fixes for CI configuration based on latest dev * Added fixes from another PR * Added lacking scripts and repos * Fixed error reported in CI. Fixed ABI entry --------- Co-authored-by: Mikolaj Malecki --- .github/workflows/abi.yml | 139 ++++++++++++++++++++++------ .github/workflows/android.yaml | 4 +- .github/workflows/codeql.yml | 4 +- .github/workflows/cxx03-ubuntu.yaml | 29 ++++++ .github/workflows/cxx11-macos.yaml | 16 ++-- .github/workflows/cxx11-ubuntu.yaml | 6 +- .github/workflows/cxx11-win.yaml | 8 +- .github/workflows/iOS.yaml | 10 +- .github/workflows/s390x-focal.yaml | 4 +- .gitmodules | 3 + .travis.yml | 116 +++++++++++++++-------- scripts/abi-check.tcl | 76 +++++++++++++++ scripts/build-windows.ps1 | 24 ++++- scripts/get-build-version.tcl | 47 ++++++++++ scripts/googletest-download.cmake | 4 +- srtcore/utilities.h | 17 ++-- submodules/abi-compliance-checker | 1 + 17 files changed, 402 insertions(+), 106 deletions(-) create mode 100644 .github/workflows/cxx03-ubuntu.yaml create mode 100644 .gitmodules create mode 100644 scripts/abi-check.tcl create mode 100755 scripts/get-build-version.tcl create mode 160000 submodules/abi-compliance-checker diff --git a/.github/workflows/abi.yml b/.github/workflows/abi.yml index 51d22a516..4d4d6012c 100644 --- a/.github/workflows/abi.yml +++ b/.github/workflows/abi.yml @@ -2,60 +2,141 @@ name: ABI checks on: push: - branches: [ master ] + branches: [ "master", "dev" ] pull_request: - branches: [ master ] + branches: [ "master", "dev" ] env: SRT_BASE: v1.5.0 jobs: - build: - name: ABI checks + build_pr: + name: Build current version runs-on: ubuntu-latest + outputs: + base_version: ${{ steps.build.outputs.SRT_BASE }} + tag_version: ${{ steps.build.outputs.SRT_TAG_VERSION }} steps: - uses: actions/checkout@v3 with: - path: pull_request - - name: configure + path: gitview_pr + - id: configure + name: Configure (cmake) run: | - cd pull_request + cd gitview_pr mkdir _build && cd _build cmake -DCMAKE_BUILD_TYPE=Debug ../ - - name: build + - id: build + name: Build and dump run: | sudo apt install -y abi-dumper - cd pull_request/_build && cmake --build ./ + sudo apt install -y tcl + cd gitview_pr/_build && cmake --build ./ make install DESTDIR=./installdir - SRT_TAG_VERSION=$(cat version.h |grep SRT_VERSION_MINOR |head -n1 |awk {'print $3'}) - abi-dumper libsrt.so -o libsrt-pr.dump -public-headers installdir/usr/local/include/srt/ -lver 0 - SRT_BASE="v1.$SRT_TAG_VERSION.0" - echo "SRT_BASE=$SRT_BASE" >> "$GITHUB_ENV" + SRT_TAG_VERSION=v$(../scripts/get-build-version.tcl full) + echo "SRT_TAG_VERSION=$SRT_TAG_VERSION" >> "$GITHUB_OUTPUT" + SRT_TAG=${SRT_TAG_VERSION}dev-$(git rev-parse --short HEAD) + echo "TAGGING PR BUILD: $SRT_TAG" + abi-dumper libsrt.so -o libsrt-pr.dump -public-headers installdir/usr/local/include/srt/ -lver $SRT_TAG + ls -ld libsrt-pr.dump + sha256sum libsrt-pr.dump + SRT_BASE=v$(../scripts/get-build-version.tcl base) + if [[ $SRT_TAG_VERSION == $SRT_BASE ]]; then + echo "NOT CHECKING ABI: base version is being built: $SRT_TAG (not emitting SRT_BASE)" + #echo "SRT_BASE=''" >> "$GITHUB_OUTPUT" + else + echo "WILL CHECK ABI changes $SRT_BASE - $SRT_TAG_VERSION" + echo "SRT_BASE=$SRT_BASE" >> "$GITHUB_OUTPUT" + fi + - id: upload_pr_dump + uses: actions/upload-artifact@v4 + with: + name: abidump_pr + path: gitview_pr/_build/libsrt-pr.dump + + build_base: + name: Build base version + runs-on: ubuntu-latest + needs: build_pr + if: ${{ needs.build_pr.outputs.base_version != '' }} + + env: + SRT_BASE: ${{ needs.build_pr.outputs.base_version }} + SRT_TAG_VERSION: ${{ needs.build_pr.outputs.tag_version }} + steps: - uses: actions/checkout@v3 with: - path: tag + path: gitview_base ref: ${{ env.SRT_BASE }} - - name: configure_tag + - id: configure_tag + name: Configure (cmake) run: | - echo $SRT_TAG_VERSION - cd tag + echo "TAG:$SRT_TAG_VERSION BASE:$SRT_BASE" + + #This is currently a paranoid check - the if should do the job + if [[ -z $SRT_BASE ]]; then + echo "NO BASE DEFINED. NOT BUILDING" + exit 1 + fi + cd gitview_base mkdir _build && cd _build cmake -DCMAKE_BUILD_TYPE=Debug ../ - - name: build_tag + - id: build_tag + name: Build and dump + if: ${{ success() }} run: | - cd tag + sudo apt install -y abi-dumper + sudo apt install -y tcl + cd gitview_base cd _build && cmake --build ./ make install DESTDIR=./installdir - abi-dumper libsrt.so -o libsrt-tag.dump -public-headers installdir/usr/local/include/srt/ -lver 1 + echo "TAGGING BASE BUILD: $SRT_BASE" + abi-dumper libsrt.so -o libsrt-base.dump -public-headers installdir/usr/local/include/srt/ -lver $SRT_BASE + ls -ld libsrt-base.dump + sha256sum libsrt-base.dump + - id: upload_base_dump + uses: actions/upload-artifact@v4 + with: + name: abidump_base + path: gitview_base/_build/libsrt-base.dump + + check_abi: + name: Compare ABI + runs-on: ubuntu-latest + needs: [build_pr, build_base] + env: + SRT_BASE: ${{ needs.build_pr.outputs.base_version }} + + steps: + - name: Download base dump + uses: actions/download-artifact@v4 + with: + name: abidump_base + path: . + - name: Download pr dump + uses: actions/download-artifact@v4 + with: + name: abidump_pr + path: . - name: abi-check run: | - git clone https://github.com/lvc/abi-compliance-checker.git - cd abi-compliance-checker && sudo make install && cd ../ - abi-compliance-checker -l libsrt -old tag/_build/libsrt-tag.dump -new pull_request/_build/libsrt-pr.dump - RES=$? - if (( $RES != 0 )) - then - echo "ABI/API Compatibility check failed with value $?" - exit $RES - fi + git clone https://github.com/lvc/abi-compliance-checker.git + #cd gitview_pr/submodules + #git submodule update --init abi-compliance-checker + cd abi-compliance-checker && sudo make install && cd ../ + #cd ../.. + echo "FILESYSTEM state before running abi-check at $PWD" + ls -l + sha256sum libsrt-base.dump + sha256sum libsrt-pr.dump + abi-compliance-checker -l libsrt -old libsrt-base.dump -new libsrt-pr.dump + RES=$? + if (( $RES != 0 )); then + echo "ABI/API Compatibility check failed with value $?" + exit $RES + fi + - name: Download report + uses: actions/download-artifact@v4 + with: + path: compat_reports diff --git a/.github/workflows/android.yaml b/.github/workflows/android.yaml index 5e11c75df..c94c0bb3c 100644 --- a/.github/workflows/android.yaml +++ b/.github/workflows/android.yaml @@ -2,9 +2,9 @@ name: Android on: push: - branches: [ master ] + branches: [ "master", "dev" ] pull_request: - branches: [ master ] + branches: [ "master", "dev" ] jobs: build: diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 4a337b6ca..f7f417259 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -2,9 +2,9 @@ name: "CodeQL" on: push: - branches: [ "master", "experimental/socket-groups" ] + branches: [ "master", "dev" ] pull_request: - branches: [ "master" ] + branches: [ "master", "dev" ] jobs: analyze: diff --git a/.github/workflows/cxx03-ubuntu.yaml b/.github/workflows/cxx03-ubuntu.yaml new file mode 100644 index 000000000..ce27436aa --- /dev/null +++ b/.github/workflows/cxx03-ubuntu.yaml @@ -0,0 +1,29 @@ +name: C++03 (old compat) + +on: + push: + branches: [ "master", "dev" ] + pull_request: + branches: [ "master", "dev" ] + types: [opened, synchronize, reopened] +jobs: + build: + name: ubuntu + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - name: configure + run: | + mkdir _build && cd _build + cmake ../ -DCMAKE_COMPILE_WARNING_AS_ERROR=ON -DENABLE_STDCXX_SYNC=OFF -DUSE_CXX_STD=c++03 -DENABLE_ENCRYPTION=ON -DENABLE_UNITTESTS=ON -DENABLE_BONDING=ON -DENABLE_TESTING=ON -DENABLE_EXAMPLES=ON -DENABLE_CODE_COVERAGE=ON -DCMAKE_EXPORT_COMPILE_COMMANDS=ON + - name: build + # That below is likely SonarQube remains, which was removed earlier. + #run: cd _build && build-wrapper-linux-x86-64 --out-dir ${{ env.BUILD_WRAPPER_OUT_DIR }} cmake --build . + run: cd _build && cmake --build . + - name: test + run: | + cd _build && ctest --extra-verbose + - name: codecov + run: | + source ./scripts/collect-gcov.sh + bash <(curl -s https://codecov.io/bash) diff --git a/.github/workflows/cxx11-macos.yaml b/.github/workflows/cxx11-macos.yaml index 4732c2abb..b03b4f75f 100644 --- a/.github/workflows/cxx11-macos.yaml +++ b/.github/workflows/cxx11-macos.yaml @@ -1,10 +1,10 @@ -name: cxx11 +name: C++11 on: push: - branches: [ master ] + branches: [ "master", "dev" ] pull_request: - branches: [ master ] + branches: [ "master", "dev" ] jobs: build: @@ -14,13 +14,17 @@ jobs: steps: - name: GoogleTest run: | - curl -o googletest.rb https://raw.githubusercontent.com/Homebrew/homebrew-core/23e7fb4dc0cc73facc3772815741e1deb87d6406/Formula/googletest.rb - brew install -s googletest.rb + brew tap-new Haivision/gt-local + brew extract --version=1.12.1 --force googletest Haivision/gt-local + brew install googletest@1.12.1 + # NOTE: 1.12.1 is the last version that requires C++11; might need update later + # curl -o googletest.rb https://raw.githubusercontent.com/Homebrew/homebrew-core/23e7fb4dc0cc73facc3772815741e1deb87d6406/Formula/googletest.rb + # brew install -s googletest.rb - uses: actions/checkout@v3 - name: configure run: | mkdir _build && cd _build - cmake ../ -DCMAKE_COMPILE_WARNING_AS_ERROR=ON -DENABLE_STDCXX_SYNC=ON -DENABLE_ENCRYPTION=OFF -DENABLE_UNITTESTS=ON -DENABLE_BONDING=ON -DUSE_CXX_STD=14 + cmake ../ -DCMAKE_COMPILE_WARNING_AS_ERROR=ON -DENABLE_STDCXX_SYNC=ON -DENABLE_ENCRYPTION=OFF -DENABLE_UNITTESTS=ON -DENABLE_BONDING=ON -DUSE_CXX_STD=17 - name: build run: cd _build && cmake --build ./ - name: test diff --git a/.github/workflows/cxx11-ubuntu.yaml b/.github/workflows/cxx11-ubuntu.yaml index 2f362d84f..eda7ecca3 100644 --- a/.github/workflows/cxx11-ubuntu.yaml +++ b/.github/workflows/cxx11-ubuntu.yaml @@ -1,10 +1,10 @@ -name: cxx11 +name: C++11 on: push: - branches: [ master ] + branches: [ "master", "dev" ] pull_request: - branches: [ master ] + branches: [ "master", "dev" ] types: [opened, synchronize, reopened] jobs: build: diff --git a/.github/workflows/cxx11-win.yaml b/.github/workflows/cxx11-win.yaml index f1554053d..2947809ab 100644 --- a/.github/workflows/cxx11-win.yaml +++ b/.github/workflows/cxx11-win.yaml @@ -1,10 +1,10 @@ -name: cxx11 +name: C++11 on: push: - branches: [ master ] + branches: [ "master", "dev" ] pull_request: - branches: [ master ] + branches: [ "master", "dev" ] jobs: build: @@ -17,7 +17,7 @@ jobs: - name: configure run: | md _build && cd _build - cmake ../ -DENABLE_STDCXX_SYNC=ON -DENABLE_ENCRYPTION=OFF -DENABLE_UNITTESTS=ON -DENABLE_BONDING=ON -DUSE_CXX_STD=c++11 + cmake ../ -DENABLE_STDCXX_SYNC=ON -DENABLE_ENCRYPTION=OFF -DENABLE_UNITTESTS=ON -DENABLE_BONDING=ON -DENABLE_LOCALIF_WIN32=ON -DUSE_CXX_STD=c++11 - name: build run: cd _build && cmake --build ./ --config Release --verbose - name: test diff --git a/.github/workflows/iOS.yaml b/.github/workflows/iOS.yaml index 0fb11542b..9ae9e9124 100644 --- a/.github/workflows/iOS.yaml +++ b/.github/workflows/iOS.yaml @@ -2,9 +2,9 @@ name: iOS on: push: - branches: [ master ] + branches: [ "master", "dev" ] pull_request: - branches: [ master ] + branches: [ "master", "dev" ] jobs: build: @@ -17,9 +17,13 @@ jobs: steps: - uses: actions/checkout@v3 + - name: Install dependency packages + run: | + brew install make llvm - name: configure run: | mkdir _build && cd _build - cmake ../ -DENABLE_ENCRYPTION=OFF -DENABLE_STDCXX_SYNC=${{matrix.cxxstdsync}} -DENABLE_UNITTESTS=OFF -DENABLE_BONDING=ON --toolchain scripts/iOS.cmake + export PATH="/opt/homebrew/opt/llvm/bin:$PATH" CC=clang CXX=clang++ + cmake .. -DCMAKE_MAKE_PROGRAM=gmake -DENABLE_ENCRYPTION=OFF -DENABLE_STDCXX_SYNC=${{matrix.cxxstdsync}} -DENABLE_MONOTONIC_CLOCK=OFF -DENABLE_UNITTESTS=OFF -DUSE_CXX_STD=c++11 -DENABLE_BONDING=ON --toolchain scripts/iOS.cmake - name: build run: cd _build && cmake --build ./ diff --git a/.github/workflows/s390x-focal.yaml b/.github/workflows/s390x-focal.yaml index f1b6c7508..d91991456 100644 --- a/.github/workflows/s390x-focal.yaml +++ b/.github/workflows/s390x-focal.yaml @@ -2,9 +2,9 @@ name: QEMU to run s390x-focal on: push: - branches: [ master ] + branches: [ "master", "dev" ] pull_request: - branches: [ master ] + branches: [ "master", "dev" ] jobs: Tests: diff --git a/.gitmodules b/.gitmodules new file mode 100644 index 000000000..0c81e9233 --- /dev/null +++ b/.gitmodules @@ -0,0 +1,3 @@ +[submodule "submodules/abi-compliance-checker"] + path = submodules/abi-compliance-checker + url = https://github.com/lvc/abi-compliance-checker.git diff --git a/.travis.yml b/.travis.yml index 0402fb51e..5c54148ea 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,3 +1,10 @@ +# TRAVIS Configuration file +# +# XXX MIND SYNTAX TRICKS: +# - Keep column consistency +# - TAB characters NOT ALLOWED anywhere +# - COLON characters (in freestanding strings) not allowed in the script + language: cpp dist: xenial @@ -16,27 +23,27 @@ addons: packages: - openssl -matrix: +jobs: include: - os: linux env: - - BUILD_TYPE=Debug - - BUILD_OPTS='-DENABLE_BONDING=ON -DCMAKE_CXX_FLAGS="-Werror"' + - BUILD_TYPE=Debug CFG="monotonic openssl werror" + - CMAKE_OPTS='-DENABLE_BONDING=ON -DCMAKE_CXX_FLAGS="-Werror"' - env: - - BUILD_TYPE=Debug - - BUILD_OPTS='-DENABLE_LOGGING=OFF -DUSE_ENCLIB=mbedtls -DENABLE_MONOTONIC_CLOCK=ON -DENABLE_BONDING=ON -DCMAKE_CXX_FLAGS="-Werror"' + - BUILD_TYPE=Debug CFG="nologging mbedtls monotonic werror" + - CMAKE_OPTS='-DENABLE_LOGGING=OFF -DUSE_ENCLIB=mbedtls -DENABLE_MONOTONIC_CLOCK=ON -DENABLE_BONDING=ON -DCMAKE_CXX_FLAGS="-Werror"' - os: linux - env: BUILD_TYPE=Release - - os: osx - osx_image: xcode11.1 - env: - - BUILD_TYPE=Debug - - BUILD_OPTS='-DCMAKE_CXX_FLAGS="-Werror"' - - os: osx - osx_image: xcode11.1 - env: - - BUILD_TYPE=Release - - BUILD_OPTS='-DCMAKE_CXX_FLAGS="-Werror"' + env: BUILD_TYPE=Release CFG=default +# - os: osx +# osx_image: xcode11.1 +# env: +# - BUILD_TYPE=Debug CFG=werror +# - CMAKE_OPTS='-DCMAKE_CXX_FLAGS="-Werror"' +# - os: osx +# osx_image: xcode11.1 +# env: +# - BUILD_TYPE=Release CFG=werror +# - CMAKE_OPTS='-DCMAKE_CXX_FLAGS="-Werror"' - os: linux compiler: x86_64-w64-mingw32-g++ addons: @@ -53,37 +60,70 @@ matrix: - ./Configure --cross-compile-prefix=x86_64-w64-mingw32- mingw64 - make - cd .. - env: BUILD_TYPE=Release + env: BUILD_TYPE=Release CFG=no-UT # Power jobs + # Forcing Focal distro because Xenial + # has somehow outdated CMake - os: linux arch: ppc64le + dist: focal env: - - BUILD_TYPE=Debug + - ARCH=PowerPC BUILD_TYPE=Debug - arch: ppc64le + dist: focal env: - - BUILD_TYPE=Release - - BUILD_OPTS='-DENABLE_MONOTONIC_CLOCK=ON' + - ARCH=PowerPC BUILD_TYPE=Release CFG=monotonic + - CMAKE_OPTS='-DENABLE_MONOTONIC_CLOCK=ON' script: + - CMAKE_VERSION=$(cmake --version | head -1 | awk '{print $3}') + - echo CMAKE version $CMAKE_VERSION + - CMAKE_VERSION_OK=$(echo "puts [package vcompare $CMAKE_VERSION 3.10]" | tclsh); + - if [ "$CMAKE_VERSION_OK" == "-1" ]; then + echo "ERROR - cmake version too old, >=3.10 required"; + exit 1; + fi; + - export REQUIRE_UNITTESTS=1 + - if [ "$TRAVIS_COMPILER" == "x86_64-w64-mingw32-g++" ]; then + CMAKE_OPTS+=" -DCMAKE_C_COMPILER=x86_64-w64-mingw32-gcc"; + CMAKE_OPTS+=" -DCMAKE_CXX_COMPILER=x86_64-w64-mingw32-g++"; + CMAKE_OPTS+=" -DENABLE_STDCXX_SYNC=OFF -DENABLE_LOCALIF_WIN32=OFF -DENABLE_UNITTESTS=OFF -DUSE_OPENSSL_PC=OFF"; + CMAKE_OPTS+=" -DOPENSSL_ROOT_DIR=$PWD/openssl"; + CMAKE_OPTS+=" -DOPENSSL_CRYPTO_LIBRARY=$PWD/openssl/libcrypto-1_1-x64.dll"; + CMAKE_OPTS+=" -DCMAKE_SYSTEM_NAME=Windows"; + REQUIRE_UNITTESTS=0; + fi; + if [ "$TRAVIS_OS_NAME" == "osx" ]; then + export PKG_CONFIG_PATH="$(brew --prefix openssl)/lib/pkgconfig"; + fi; + if (( $REQUIRE_UNITTESTS )); then + CMAKE_OPTS+=" -DENABLE_UNITTESTS=ON"; + fi; - echo COMPILER $TRAVIS_COMPILER - echo SYSTEM $TRAVIS_OS_NAME - echo BUILD_TYPE $BUILD_TYPE - - echo BUILD_OPTS $BUILD_OPTS - - if [ "$TRAVIS_COMPILER" == "x86_64-w64-mingw32-g++" ]; then - export CC="x86_64-w64-mingw32-gcc"; - export CXX="x86_64-w64-mingw32-g++"; - cmake . -DCMAKE_BUILD_TYPE=$BUILD_TYPE $BUILD_OPTS -DENABLE_UNITTESTS="OFF" -DUSE_OPENSSL_PC="OFF" -DOPENSSL_ROOT_DIR="$PWD/openssl" -DCMAKE_SYSTEM_NAME="Windows"; - elif [ "$TRAVIS_OS_NAME" == "linux" ]; then - cmake . -DCMAKE_BUILD_TYPE=$BUILD_TYPE $BUILD_OPTS -DENABLE_UNITTESTS="ON"; - elif [ "$TRAVIS_OS_NAME" == "osx" ]; then - export PKG_CONFIG_PATH=$(brew --prefix openssl)"/lib/pkgconfig"; - cmake . -DCMAKE_BUILD_TYPE=$BUILD_TYPE $BUILD_OPTS -DENABLE_UNITTESTS="ON"; - fi - - make -j$(nproc); - - if [ "$TRAVIS_COMPILER" != "x86_64-w64-mingw32-g++" ]; then + - echo CMAKE_OPTS $CMAKE_OPTS + - export SUCCESS=0 + - cmake . --debug-output -DCMAKE_MESSAGE_LOG_LEVEL=VERBOSE -DCMAKE_BUILD_TYPE=$BUILD_TYPE $CMAKE_OPTS 2>&1 || SUCCESS=$?; + - if (($SUCCESS == 0)); then + echo "Configure OK"; + else + echo "-- OUTPUT --"; + cat CMakeFiles/CMakeOutput.log || echo "NO OUTPUT"; + echo "-- ERRORS --"; + cat CMakeFiles/CMakeError.log || echo "NO LOGS"; + exit 1; + fi; + - make VERBOSE=1 -j$(nproc); + - if (( $REQUIRE_UNITTESTS )); then ulimit -c unlimited; - ./test-srt -disable-ipv6; - SUCCESS=$?; - if [ -f core ]; then gdb -batch ./test-srt -c core -ex bt -ex "info thread" -ex quit; else echo "NO CORE - NO CRY!"; fi; - test $SUCCESS == 0; - fi + if [ ! -f ./test-srt ]; then + echo "ERROR - UT application not found"; + false; + else + ./test-srt -disable-ipv6; + SUCCESS=$?; + if [ -f core ]; then gdb -batch ./test-srt -c core -ex bt -ex "info thread" -ex quit; else echo "NO CORE - NO CRY!"; fi; + test $SUCCESS == 0; + fi; + fi; diff --git a/scripts/abi-check.tcl b/scripts/abi-check.tcl new file mode 100644 index 000000000..7f9c70e79 --- /dev/null +++ b/scripts/abi-check.tcl @@ -0,0 +1,76 @@ +#!/usr/bin/tclsh + +set here [file dirname $argv0] ;# points to [git top]/scripts +set top [file normalize $here/..] + +set abichecker [file normalize [file join $top submodules abi-compliance-checker abi-compliance-checker.pl]] + +if { ![file exists $abichecker] } { + puts stderr "Please update submodules first (compliance checker not found in the current view)" + exit 1 +} + +# Check if abi-dumper is installed + +set abidumper [auto_execok abi-dumper] +if {$abidumper == ""} { + set installer "" + foreach ii {zypper dnf apt} { + if {[auto_execok $ii] != ""} { + set installer $ii + break + } + } + if {$installer != ""} { + puts stderr "ABI dumper not installed. Use such commands to install\n" + puts stderr " $installer install abi-dumper" + } else { + puts stderr "ABI dumper not installed. Find out how to install abi-dumper in your system." + } + exit 1 +} + +# Arguments: +# [directory-with-base] [directory-with-pr] + +# NOTE: ABI dump will be done in every build directory as specified. + +proc generate-abi-dump {directory lver} { + set wd [pwd] + cd $directory + global top + + # You should have libsrt.so in this directory. + # Use [glob] to use exception if no file exists + glob libsrt.so + + exec >@stdout 2>@stderr abi-dumper libsrt.so -o libsrt-abi.dump -public-headers $top/srtcore -lver $lver + cd $wd +} + +set olddir [lindex $argv 0] +set newdir [lindex $argv 1] + +if {![file isdirectory $olddir] || ![file isdirectory $newdir]} { + puts stderr "Wrong arguments. Required build directory" + exit 1 +} + +set wd [pwd] +cd $olddir +set base_ver [exec $top/scripts/get-build-version.tcl] +cd $wd +cd $newdir +set new_ver [exec $top/scripts/get-build-version.tcl] +cd $wd + +generate-abi-dump $olddir $base_ver +generate-abi-dump $newdir $new_ver + +set res [catch {exec >@stdout 2>@stderr $abichecker -l libsrt -old $olddir/libsrt-abi.dump -new $newdir/libsrt-abi.dump} out] + +if {$res} { + puts stderr "ABI compat problems found!!!\nSee the HTML report" +} + + diff --git a/scripts/build-windows.ps1 b/scripts/build-windows.ps1 index c4c7b5740..a65247ccd 100644 --- a/scripts/build-windows.ps1 +++ b/scripts/build-windows.ps1 @@ -49,11 +49,25 @@ if ( $Env:APPVEYOR ) { $CONFIGURATION = $Env:CONFIGURATION - #appveyor has many openssl installations - place the latest one in the default location unless VS2013 - Remove-Item -Path "C:\OpenSSL-Win32" -Recurse -Force -ErrorAction SilentlyContinue | Out-Null - Remove-Item -Path "C:\OpenSSL-Win64" -Recurse -Force -ErrorAction SilentlyContinue | Out-Null - Copy-Item -Path "C:\OpenSSL-v111-Win32" "C:\OpenSSL-Win32" -Recurse | Out-Null - Copy-Item -Path "C:\OpenSSL-v111-Win64" "C:\OpenSSL-Win64" -Recurse | Out-Null + # See https://www.appveyor.com/docs/windows-images-software/#visual-studio-2022 + # According to AppVeyor environment definition: + # + # 2013 - 2017: C:\OpenSSL-Win*: v1.0.2p, C:\OpenSSL-v111-Win*: v1.1.1 (v1.1.1d for 2015+) + # 2019 - 2022: C:\OpenSSL-Win*: v1.1.1w and C:\OpenSSL-v3* with version 3+ + # SO: + # For 2013 - 2017: Delete C:\OpenSSL-Win* and replace it with contents of C:\OpenSSL-v111-Win* + # For 2019 - 2022: Do nothing. + # Note: No guarantee that C:\OpenSSL-Win* directories will always contain version v1.1.1+ in + # any future versions for AppVeyor Windows environment, but then probably the SRT project + # would have to be adjusted to this version anyway. + + if ($VS_VERSION -lt 2019) { + #appveyor has many openssl installations - place the latest one in the default location unless VS2013 + Remove-Item -Path "C:\OpenSSL-Win32" -Recurse -Force -ErrorAction SilentlyContinue | Out-Null + Remove-Item -Path "C:\OpenSSL-Win64" -Recurse -Force -ErrorAction SilentlyContinue | Out-Null + Copy-Item -Path "C:\OpenSSL-v111-Win32" "C:\OpenSSL-Win32" -Recurse | Out-Null + Copy-Item -Path "C:\OpenSSL-v111-Win64" "C:\OpenSSL-Win64" -Recurse | Out-Null + } } # persist VS_VERSION so it can be used in an artifact name later diff --git a/scripts/get-build-version.tcl b/scripts/get-build-version.tcl new file mode 100755 index 000000000..fafda061a --- /dev/null +++ b/scripts/get-build-version.tcl @@ -0,0 +1,47 @@ +#!/usr/bin/tclsh + +if {![file exists version.h]} { + puts stderr "No version.h file found - run this in the build directory!" + exit 1 +} + +set fd [open version.h r] + +set version "" +set line "" +while {[gets $fd line] != -1} { + # Use regexp because this is safer to avoid + # unexpected list interpretation mistakes + if {[regexp {#define SRT_VERSION_STRING} $line]} { + # Once matched this way, we can safely use tcl-list-like access + set version [lindex $line 2 0] + break + } +} +close $fd + +if {$version == ""} { + puts stderr "No version extracted (no SRT_VERSION_STRING found)" + exit 1 +} + +lassign $argv model part + +if {$model == ""} { + set model current +} + +lassign [split $version .] major minor patch + +if {$part == "minor"} { + set prefix "$minor" +} else { + set prefix "$major.$minor" +} + +if {$model == "base"} { + puts "$prefix.0" +} else { + puts "$prefix.$patch" +} + diff --git a/scripts/googletest-download.cmake b/scripts/googletest-download.cmake index 36554f316..f1f6d4902 100644 --- a/scripts/googletest-download.cmake +++ b/scripts/googletest-download.cmake @@ -1,7 +1,7 @@ # code copied from https://crascit.com/2015/07/25/cmake-gtest/ -cmake_minimum_required(VERSION 2.8.12 FATAL_ERROR) +cmake_minimum_required(VERSION 3.10 FATAL_ERROR) -project(googletest-download NONE) +project(GOOGLETEST_DOWNLOAD) include(ExternalProject) diff --git a/srtcore/utilities.h b/srtcore/utilities.h index ca8c365a3..8781b80e7 100644 --- a/srtcore/utilities.h +++ b/srtcore/utilities.h @@ -828,17 +828,14 @@ struct CallbackHolder // Test if the pointer is a pointer to function. Don't let // other type of pointers here. #if HAVE_CXX11 + // NOTE: No poor-man's replacement can be done for C++03 because it's + // not possible to fake calling a function without calling it and no + // other operation can be done without extensive transformations on + // the Signature type, still in C++03 possible only on functions up to + // 2 arguments (callbacks in SRT usually have more). static_assert(std::is_function::value, "CallbackHolder is for functions only!"); -#else - // This is a poor-man's replacement, which should in most compilers - // generate a warning, if `Signature` resolves to a value type. - // This would make an illegal pointer cast from a value to a function type. - // Casting function-to-function, however, should not. Unfortunately - // newer compilers disallow that, too (when a signature differs), but - // then they should better use the C++11 way, much more reliable and safer. - void* (*testfn)(void*) = (void*(*)(void*))f; - (void)(testfn); #endif + opaque = o; fn = f; } @@ -1111,7 +1108,7 @@ struct MapProxy { typename std::map::const_iterator p = find(); if (p == mp.end()) - return ""; + return ValueType(); return p->second; } diff --git a/submodules/abi-compliance-checker b/submodules/abi-compliance-checker new file mode 160000 index 000000000..7c175c45a --- /dev/null +++ b/submodules/abi-compliance-checker @@ -0,0 +1 @@ +Subproject commit 7c175c45a8ba9ac41b8e47d8ebbab557b623b18e From 84676b50be5e14b043b781257ebd9faa14e7a540 Mon Sep 17 00:00:00 2001 From: cl-ment Date: Tue, 21 Oct 2025 16:59:14 +0200 Subject: [PATCH 11/19] [build] Avoid using list(PREPEND). (#3230) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Clément Gérouville --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index a7e979513..6e19caa2c 100755 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -966,7 +966,7 @@ else () message(STATUS "Pthread library: ${PTHREAD_LIBRARY}") endif() -list(PREPEND CMAKE_REQUIRED_LIBRARIES "${PTHREAD_LIBRARY}") +set(CMAKE_REQUIRED_LIBRARIES "${PTHREAD_LIBRARY};${CMAKE_REQUIRED_LIBRARIES}") unset(CMAKE_REQUIRED_QUIET) check_symbol_exists(pthread_atfork "pthread.h" HAVE_PTHREAD_ATFORK) From 547688cce7db6949dd8236507331cbe2a14cd06b Mon Sep 17 00:00:00 2001 From: Sektor van Skijlen Date: Wed, 22 Oct 2025 08:55:08 +0200 Subject: [PATCH 12/19] [core] Restored old cookie contest. Added development support for enforcing cookie values. (#3227) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Initial testing version of the cookie contest fix * Setting cookie contest 1.5.4 as default * [core] Restored old cookie contest. Added ignored wrong peer response on cookie collision * Fixed security report * Removed the (unnecessary now) fix for the future version compatibility * Update test/test_common.cpp - fixed a typo in a comment * Fixed TestFEC tests to be resistant of a listener-readiness bug (fixed in dev) * Try to fix the first issue * Removed remains after the screw-in cookie procedures --------- Co-authored-by: Mikolaj Malecki Co-authored-by: Clément Gérouville --- srtcore/core.cpp | 120 +++++++++++++++++------------------ srtcore/core.h | 16 +++++ test/test_common.cpp | 45 +++++++++++++ test/test_fec_rebuilding.cpp | 65 +++++++++---------- 4 files changed, 150 insertions(+), 96 deletions(-) diff --git a/srtcore/core.cpp b/srtcore/core.cpp index 0acef17bb..dd189f73c 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -4102,88 +4102,69 @@ void srt::CUDT::sendRendezvousRejection(const sockaddr_any& serv_addr, CPacket& m_pSndQueue->sendto(serv_addr, r_rsppkt, m_SourceAddr); } -void srt::CUDT::cookieContest() +enum HandshakeSide srt::CUDT::compareCookies(int32_t req, int32_t res) { - if (m_SrtHsSide != HSD_DRAW) - return; - - // Here m_ConnReq.m_iCookie is a local cookie value sent in connection request to the peer. - // m_ConnRes.m_iCookie is a cookie value sent by the peer in its connection request. - if (m_ConnReq.m_iCookie == 0 || m_ConnRes.m_iCookie == 0) + if (req < res) { - LOGC(cnlog.Debug, log << CONID() << "cookieContest: agent=" << m_ConnReq.m_iCookie << " peer=" << m_ConnRes.m_iCookie - << " - ERROR: zero not allowed!"); - - // Note that it's virtually impossible that Agent's cookie is not ready, this - // shall be considered IPE. - // Not all cookies are ready, don't start the contest. - return; + LOGC(gglog.Note, log << hex << setfill('0') << uppercase + << "REQ: " << setw(8) << req + << " RES: " << setw(8) << res << " - result: RESPONDER"); + return HSD_RESPONDER; } + else if (req > res) + { + LOGC(gglog.Note, log << hex << setfill('0') << uppercase + << "REQ: " << setw(8) << req + << " RES: " << setw(8) << res << " - result: INITIATOR"); + return HSD_INITIATOR; + } + return HSD_DRAW; +} - // INITIATOR/RESPONDER role is resolved by COOKIE CONTEST. - // - // The cookie contest must be repeated every time because it - // may change the state at some point. - // - // In SRT v1.4.3 and prior the below subtraction was performed in 32-bit arithmetic. - // The result of subtraction can overflow 32-bits. - // Example - // m_ConnReq.m_iCookie = -1480577720; - // m_ConnRes.m_iCookie = 811599203; - // int64_t llBetterCookie = -1480577720 - 811599203 = -2292176923 (FFFF FFFF 7760 27E5); - // int32_t iBetterCookie = 2002790373 (7760 27E5); - // - // Now 64-bit arithmetic is used to calculate the actual result of subtraction. - // - // In SRT v1.5.4 there was a version, that checked: - // - if LOWER 32-bits are 0, this is a draw - // - if bit 31 is set (AND with 0x80000000), the result is considered negative. - // This was erroneous because for 1 and 0x80000001 cookie values the - // result was always the same, regardless of the order: - // - // 0x0000000000000001 - 0xFFFFFFFF80000001 = 0x0000000080000000 - // 0xFFFFFFFF80000001 - 0x0000000080000000 = 0xFFFFFFFF80000000 - // - // >> if (contest & 0x80000000) -> results in true in both comparisons. - // - // This version takes the bare result of the 64-bit arithmetics. - const int64_t xreq = int64_t(m_ConnReq.m_iCookie); - const int64_t xres = int64_t(m_ConnRes.m_iCookie); +enum HandshakeSide srt::CUDT::backwardCompatibleCookieContest(int32_t req, int32_t res) +{ + const int64_t xreq = int64_t(req); + const int64_t xres = int64_t(res); const int64_t contest = xreq - xres; - // NOTE: for 1.6.0 use: - // fmtc hex64 = fmtc().uhex().fillzero().width(16); then - // << fmt(xreq, hex64) - LOGC(cnlog.Debug, log << CONID() << "cookieContest: agent=" << m_ConnReq.m_iCookie - << " peer=" << m_ConnRes.m_iCookie + LOGC(cnlog.Debug, log << "cookieContest: agent=" << req + << " peer=" << res << hex << uppercase << setfill('0') // PERSISTENT flags << " X64: "<< setw(16) << xreq << " vs. " << setw(16) << xres << " DIFF: " << setw(16) << contest); - if (contest == 0) + if ((contest & 0xFFFFFFFF) == 0) { - // DRAW! The only way to continue would be to force the - // cookies to be regenerated and to start over. But it's - // not worth a shot - this is an extremely rare case. - // This can simply do reject so that it can be started again. + return HSD_DRAW; + } + if (contest & 0x80000000) + { + const int64_t revert = xres - xreq; + if (revert & 0x80000000 && req > res) + return HSD_INITIATOR; + return HSD_RESPONDER; + } - // Pretend then that the cookie contest wasn't done so that - // it's done again. Cookies are baked every time anew, however - // the successful initial contest remains valid no matter how - // cookies will change. + return HSD_INITIATOR; +} - m_SrtHsSide = HSD_DRAW; +void srt::CUDT::cookieContest() +{ + if (m_SrtHsSide != HSD_DRAW) return; - } - if (contest < 0) + // Here m_ConnReq.m_iCookie is a local cookie value sent in connection request to the peer. + // m_ConnRes.m_iCookie is a cookie value sent by the peer in its connection request. + if (m_ConnReq.m_iCookie == 0 || m_ConnRes.m_iCookie == 0) { - m_SrtHsSide = HSD_RESPONDER; + LOGC(cnlog.Debug, log << CONID() << "cookieContest: agent=" << m_ConnReq.m_iCookie << " peer=" << m_ConnRes.m_iCookie + << " - ERROR: zero not allowed!"); + return; } + m_SrtHsSide = backwardCompatibleCookieContest(m_ConnReq.m_iCookie, m_ConnRes.m_iCookie); - m_SrtHsSide = HSD_INITIATOR; } // This function should complete the data for KMX needed for an out-of-band @@ -12153,3 +12134,18 @@ void srt::CUDT::processKeepalive(const CPacket& ctrlpkt, const time_point& tsArr if (m_config.bDriftTracer) m_pRcvBuffer->addRcvTsbPdDriftSample(ctrlpkt.getMsgTimeStamp(), tsArrival, -1); } + +namespace srt { +HandshakeSide getHandshakeSide(SRTSOCKET u) +{ + return CUDT::handshakeSide(u); +} + +HandshakeSide CUDT::handshakeSide(SRTSOCKET u) +{ + CUDTSocket *s = uglobal().locateSocket(u); + return s ? s->core().handshakeSide() : HSD_DRAW; +} +} + + diff --git a/srtcore/core.h b/srtcore/core.h index a07d17277..6bf75466e 100644 --- a/srtcore/core.h +++ b/srtcore/core.h @@ -294,6 +294,17 @@ class CUDT return m_ConnRes.m_iVersion; } + int32_t handshakeCookie() + { + return m_ConnReq.m_iCookie; + } + + static HandshakeSide handshakeSide(SRTSOCKET u); + HandshakeSide handshakeSide() + { + return m_SrtHsSide; + } + std::string CONID() const { #if ENABLE_LOGGING @@ -1010,6 +1021,8 @@ class CUDT public: static int installAcceptHook(SRTSOCKET lsn, srt_listen_callback_fn* hook, void* opaq); static int installConnectHook(SRTSOCKET lsn, srt_connect_callback_fn* hook, void* opaq); + static enum HandshakeSide compareCookies(int32_t req, int32_t res); + static enum HandshakeSide backwardCompatibleCookieContest(int32_t req, int32_t res); private: void installAcceptHook(srt_listen_callback_fn* hook, void* opaq) { @@ -1248,6 +1261,9 @@ class CUDT void removeEPollID(const int eid); }; +// DEBUG SUPPORT +HandshakeSide getHandshakeSide(SRTSOCKET s); + } // namespace srt #endif diff --git a/test/test_common.cpp b/test/test_common.cpp index 1a8cca061..b4e150f1b 100644 --- a/test/test_common.cpp +++ b/test/test_common.cpp @@ -5,6 +5,7 @@ #include "test_env.h" #include "utilities.h" #include "common.h" +#include "core.h" using namespace srt; @@ -71,3 +72,47 @@ TEST(CIPAddress, IPv4_in_IPv6_pton) test_cipaddress_pton(peer_ip, AF_INET6, ip); } + +void testCookieContest(int32_t agent_cookie, int32_t peer_cookie) +{ + using namespace srt; + using namespace std; + + cout << "TEST: Cookies: agent=" << hex << agent_cookie + << " peer=" << peer_cookie << endl << dec; + HandshakeSide agent_side = CUDT::backwardCompatibleCookieContest(agent_cookie, peer_cookie); + EXPECT_EQ(agent_side, HSD_INITIATOR); + HandshakeSide peer_side = CUDT::backwardCompatibleCookieContest(peer_cookie, agent_cookie); + EXPECT_EQ(peer_side, HSD_RESPONDER); +} + +TEST(Common, CookieContest) +{ + srt::TestInit srtinit; + using namespace std; + + srt_setloglevel(LOG_NOTICE); + + // In this function you should pass cookies always in the order: INITIATOR, RESPONDER. + cout << "TEST 1: two easy comparable values\n"; + testCookieContest(100, 50); + testCookieContest(-1, -1000); + testCookieContest(10055, -10000); + testCookieContest(-1480577720, 811599203); + testCookieContest( -2147483648, 2147483647); + testCookieContest(0x00000001, 0x80000001); + + /* XXX Blocked temporarily because in 1.5.5 they would fail, + and it's decided to not provide the fix, which is too + dangerous. The real fix will be provided in 1.6.0. + + // Values from PR 1517 + cout << "TEST 2: Values from PR 1517\n"; + testCookieContest(811599203, -1480577720); + testCookieContest(2147483647, -2147483648); + + cout << "TEST 3: wrong post-fix\n"; + // NOTE: 0x80000001 is a negative number in hex + testCookieContest(0x00000001, 0x80000001); + */ +} diff --git a/test/test_fec_rebuilding.cpp b/test/test_fec_rebuilding.cpp index 3a7cb0791..b39345c40 100644 --- a/test/test_fec_rebuilding.cpp +++ b/test/test_fec_rebuilding.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include "gtest/gtest.h" #include "test_env.h" @@ -92,6 +93,21 @@ class TestFECRebuilding: public srt::Test } }; +static std::future spawn_connect(SRTSOCKET s, sockaddr_in& sa, int timeout_ms = 1000) +{ + std::cout << "[M] SPAWNING srt_connect()\n"; + return std::async(std::launch::async, [s, &sa, timeout_ms]() + { + // Add a delay for starting connection to give a chance + // for the main thread to establish an EID with the listener + // BEFORE the handshake packet is received, otherwise the + // epoll will miss the signal (a bug fixed in 1.6.0). + std::this_thread::sleep_for(chrono::milliseconds(timeout_ms)); + std::cout << "[T] RUNNING srt_connect()\n"; + return srt_connect(s, (sockaddr*)& sa, sizeof(sa)); + }); +} + namespace srt { class TestMockCUDT { @@ -302,9 +318,7 @@ TEST(TestFEC, Connection) srt_listen(l, 1); - auto connect_res = std::async(std::launch::async, [&s, &sa]() { - return srt_connect(s, (sockaddr*)& sa, sizeof(sa)); - }); + auto connect_res = spawn_connect(s, sa, 1); // Make sure that the async call to srt_connect() is already kicked. std::this_thread::yield(); @@ -312,7 +326,7 @@ TEST(TestFEC, Connection) // Given 2s timeout for accepting as it has occasionally happened with Travis // that 1s might not be enough. SRTSOCKET la[] = { l }; - SRTSOCKET a = srt_accept_bond(la, 1, 2000); + SRTSOCKET a = srt_accept_bond(la, 1, 5000); ASSERT_NE(a, SRT_ERROR); EXPECT_EQ(connect_res.get(), SRT_SUCCESS); @@ -362,15 +376,13 @@ TEST(TestFEC, ConnectionReorder) srt_listen(l, 1); - auto connect_res = std::async(std::launch::async, [&s, &sa]() { - return srt_connect(s, (sockaddr*)& sa, sizeof(sa)); - }); + auto connect_res = spawn_connect(s, sa); // Make sure that the async call to srt_connect() is already kicked. std::this_thread::yield(); SRTSOCKET la[] = { l }; - SRTSOCKET a = srt_accept_bond(la, 1, 2000); + SRTSOCKET a = srt_accept_bond(la, 1, 5000); ASSERT_NE(a, SRT_ERROR); EXPECT_EQ(connect_res.get(), SRT_SUCCESS); @@ -420,15 +432,12 @@ TEST(TestFEC, ConnectionFull1) srt_listen(l, 1); - auto connect_res = std::async(std::launch::async, [&s, &sa]() { - return srt_connect(s, (sockaddr*)& sa, sizeof(sa)); - }); - + auto connect_res = spawn_connect(s, sa); // Make sure that the async call to srt_connect() is already kicked. std::this_thread::yield(); SRTSOCKET la[] = { l }; - SRTSOCKET a = srt_accept_bond(la, 1, 2000); + SRTSOCKET a = srt_accept_bond(la, 1, 5000); ASSERT_NE(a, SRT_ERROR); EXPECT_EQ(connect_res.get(), SRT_SUCCESS); @@ -478,15 +487,13 @@ TEST(TestFEC, ConnectionFull2) srt_listen(l, 1); - auto connect_res = std::async(std::launch::async, [&s, &sa]() { - return srt_connect(s, (sockaddr*)& sa, sizeof(sa)); - }); + auto connect_res = spawn_connect(s, sa); // Make sure that the async call to srt_connect() is already kicked. std::this_thread::yield(); SRTSOCKET la[] = { l }; - SRTSOCKET a = srt_accept_bond(la, 1, 2000); + SRTSOCKET a = srt_accept_bond(la, 1, 5000); ASSERT_NE(a, SRT_ERROR); EXPECT_EQ(connect_res.get(), SRT_SUCCESS); @@ -536,15 +543,13 @@ TEST(TestFEC, ConnectionMess) srt_listen(l, 1); - auto connect_res = std::async(std::launch::async, [&s, &sa]() { - return srt_connect(s, (sockaddr*)& sa, sizeof(sa)); - }); + auto connect_res = spawn_connect(s, sa); // Make sure that the async call to srt_connect() is already kicked. std::this_thread::yield(); SRTSOCKET la[] = { l }; - SRTSOCKET a = srt_accept_bond(la, 1, 2000); + SRTSOCKET a = srt_accept_bond(la, 1, 5000); ASSERT_NE(a, SRT_ERROR) << srt_getlasterror_str(); EXPECT_EQ(connect_res.get(), SRT_SUCCESS); @@ -592,15 +597,13 @@ TEST(TestFEC, ConnectionForced) srt_listen(l, 1); - auto connect_res = std::async(std::launch::async, [&s, &sa]() { - return srt_connect(s, (sockaddr*)& sa, sizeof(sa)); - }); + auto connect_res = spawn_connect(s, sa); // Make sure that the async call to srt_connect() is already kicked. std::this_thread::yield(); SRTSOCKET la[] = { l }; - SRTSOCKET a = srt_accept_bond(la, 1, 2000); + SRTSOCKET a = srt_accept_bond(la, 1, 5000); ASSERT_NE(a, SRT_ERROR); EXPECT_EQ(connect_res.get(), SRT_SUCCESS); @@ -645,9 +648,7 @@ TEST(TestFEC, RejectionConflict) srt_listen(l, 1); - auto connect_res = std::async(std::launch::async, [&s, &sa]() { - return srt_connect(s, (sockaddr*)& sa, sizeof(sa)); - }); + auto connect_res = spawn_connect(s, sa); // Make sure that the async call to srt_connect() is already kicked. std::this_thread::yield(); @@ -689,9 +690,7 @@ TEST(TestFEC, RejectionIncompleteEmpty) srt_listen(l, 1); - auto connect_res = std::async(std::launch::async, [&s, &sa]() { - return srt_connect(s, (sockaddr*)& sa, sizeof(sa)); - }); + auto connect_res = spawn_connect(s, sa); // Make sure that the async call to srt_connect() is already kicked. std::this_thread::yield(); @@ -737,9 +736,7 @@ TEST(TestFEC, RejectionIncomplete) srt_listen(l, 1); - auto connect_res = std::async(std::launch::async, [&s, &sa]() { - return srt_connect(s, (sockaddr*)& sa, sizeof(sa)); - }); + auto connect_res = spawn_connect(s, sa); // Make sure that the async call to srt_connect() is already kicked. std::this_thread::yield(); From a26a7021a501e8c4bc17c75f3a109cca529cf262 Mon Sep 17 00:00:00 2001 From: Sektor van Skijlen Date: Wed, 22 Oct 2025 10:47:34 +0200 Subject: [PATCH 13/19] [core] Reimplemented MAXREXMITBW controller basing on Token Bucket algorithm (#3220) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Added more logs around rexmit bandwidth * [API] raise libSRT version number to 1.5.5 * Maintenance changes * Rewrite the Bandwidth estimation system. * Merged changes from Clement's implementation * Introduce CShaper to control the retransmission BW. * Fixed with average packet size measurement * Added self-adjusting of burst period * Fixed wrong formulas and added some logs * Removed old changes for CSndRateEstimator. Blocked the use of CSndRateEstimator. * Applied most direct review fixes * Refactoring: split packLostData into smaller parts * Some cosmetic fixes * Cosmetics * Removed Tie function that was using non-C++03-compatible assignment * Stretched connection timeout for accept in FEC tests * Trying to track the problem in FEC tests on Mac * Adjusted TestFEC tests to a bug in listener missing signal --------- Co-authored-by: Mikolaj Malecki Co-authored-by: Clément Gérouville --- srtcore/buffer_tools.h | 127 ++++++++ srtcore/common.cpp | 16 +- srtcore/common.h | 1 + srtcore/core.cpp | 576 ++++++++++++++++++++++++++++++----- srtcore/core.h | 156 +++++++++- srtcore/group.cpp | 4 + srtcore/utilities.h | 16 +- test/test_fec_rebuilding.cpp | 4 + 8 files changed, 801 insertions(+), 99 deletions(-) diff --git a/srtcore/buffer_tools.h b/srtcore/buffer_tools.h index 88479827d..1c90a56a1 100644 --- a/srtcore/buffer_tools.h +++ b/srtcore/buffer_tools.h @@ -198,6 +198,133 @@ class CSndRateEstimator int m_iRateBps; //< Rate in Bytes/sec. }; +// Utility class for bandwidth limitation +class CShaper +{ +public: + static double SHAPER_RESOLUTION_US() { return 1000000.; } // micro seconds + static double BURSTPERIOD_DEFAULT() { return 100; } + + typedef sync::steady_clock::time_point time_point; + typedef sync::steady_clock::duration duration; + CShaper (double min_tokens_capacity): + m_BurstPeriod(sync::milliseconds_from(BURSTPERIOD_DEFAULT())), + m_rate_Bps(0), + m_tokens(0), + m_tokensCapacity(0), + m_minTokensCapacity(min_tokens_capacity) + { + + } + + // Token capacity should be settable later + void setMinimumTokenCapacity(double cap) + { + m_minTokensCapacity = cap; + updateLimits(); + } + +private: + duration m_BurstPeriod; + double m_rate_Bps; // current_bitrate in Bytes per second + double m_tokens; // in bytes + double m_tokensCapacity; // in bytes + double m_minTokensCapacity; + time_point m_UpdateTime; + void setTokenCapacity(double tokens) + { + m_tokensCapacity = std::max(0., tokens); + } + + void setTokens(double tokens) + { + // It is allowed to set the negative number of tokens, + // just not less than the negative maximum + m_tokens = Bounds(1 - m_tokensCapacity, tokens, m_tokensCapacity); + } + + static double periodToTokens(double rate_Bps, duration period) + { + double seconds = double(sync::count_microseconds(period)) / SHAPER_RESOLUTION_US(); + return rate_Bps * seconds; + } + + static duration tokensToPeriod(double rate_Bps, double tokens) + { + double seconds = tokens / rate_Bps; + return sync::microseconds_from(seconds * SHAPER_RESOLUTION_US()); + } + + void updateLimits() + { + double token_cap = periodToTokens(m_rate_Bps, m_BurstPeriod); + if (token_cap < m_minTokensCapacity) + { + // We have a too small value; recalculate the burst period to reach the minimum. + duration minperiod = tokensToPeriod(m_rate_Bps, m_minTokensCapacity); + token_cap = m_minTokensCapacity; + m_BurstPeriod = minperiod; + } + setTokenCapacity(token_cap); + } + +public: + void setBitrate(double bw_Bps) + { + // NOTE: comparison on double to check the PREVIOUSLY + // SET value (m_rate_Bps), not recalculated value. + if (bw_Bps != m_rate_Bps) + { + m_rate_Bps = bw_Bps; + updateLimits(); + } + } + + void setMinimumBurstPeriod(duration hp) + { + if (m_BurstPeriod < hp) + { + setBurstPeriod(hp); + } + } + + void setBurstPeriod(duration bp) + { + if (bp != m_BurstPeriod) + { + m_BurstPeriod = bp; + updateLimits(); + } + } + + // Note that tick() must be always called after setBitrate. + void tick(const time_point& now) + { + duration delta = now - m_UpdateTime; + m_UpdateTime = now; + + double update_tokens = periodToTokens(m_rate_Bps, delta); + setTokens(update_tokens + m_tokens); + } + + bool enoughTokens(double len) const { return len <= m_tokens; } + void consumeTokens(double len) { setTokens(m_tokens - len); } + + + // For debug purposes + int ntokens() const { return m_tokens; } + duration burstPeriod() const { return m_BurstPeriod; } + int maxTokens() const { return m_tokensCapacity; } + // TOKENS = BITRATE * BURST_PERIOD / (SHAPER_UNIT * SHAPER_RESOLUTION_US) + // TOKENS * SHAPER_UNIT * SHAPER_RESOLUTION_US = BITRATE * BURST_PERIOD + // BITRATE = (TOKENS * SHAPER_UNIT * SHAPER_RESOLUTION_US) / (BURST_PERIOD) + double tokenRate_Bps(double tokens) const + { + return (tokens * SHAPER_RESOLUTION_US()) / (sync::count_microseconds(m_BurstPeriod)); + } + double availRate_Bps() const { return tokenRate_Bps(m_tokens); } + double usedRate_Bps() const { return tokenRate_Bps(m_tokensCapacity - m_tokens); } +}; } // namespace srt #endif diff --git a/srtcore/common.cpp b/srtcore/common.cpp index 2e94668ae..9d0b8e338 100644 --- a/srtcore/common.cpp +++ b/srtcore/common.cpp @@ -464,12 +464,12 @@ bool SrtParseConfig(const string& s, SrtConfig& w_config) return true; } -std::string FormatLossArray(const std::vector< std::pair >& lra) +string FormatLossArray(const vector< pair >& lra) { - std::ostringstream os; + ostringstream os; os << "[ "; - for (std::vector< std::pair >::const_iterator i = lra.begin(); i != lra.end(); ++i) + for (vector< pair >::const_iterator i = lra.begin(); i != lra.end(); ++i) { int len = CSeqNo::seqoff(i->first, i->second); os << "%" << i->first; @@ -482,6 +482,16 @@ std::string FormatLossArray(const std::vector< std::pair >& lr return os.str(); } +string FormatValue(int value, int factor, const char* unit) +{ + ostringstream out; + double showval = value; + showval /= factor; + out << std::fixed << showval << unit; + return out.str(); +} + + ostream& PrintEpollEvent(ostream& os, int events, int et_events) { static pair const namemap [] = { diff --git a/srtcore/common.h b/srtcore/common.h index ff84c3faf..15fa39e37 100644 --- a/srtcore/common.h +++ b/srtcore/common.h @@ -1437,6 +1437,7 @@ inline bool checkMappedIPv4(const sockaddr_in6& sa) std::string FormatLossArray(const std::vector< std::pair >& lra); std::ostream& PrintEpollEvent(std::ostream& os, int events, int et_events = 0); +std::string FormatValue(int value, int factor, const char* unit); } // namespace srt diff --git a/srtcore/core.cpp b/srtcore/core.cpp index dd189f73c..9e8d6b11e 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -259,6 +259,134 @@ CUDTUnited& srt::CUDT::uglobal() #endif +#ifdef ENABLE_RATE_MEASUREMENT +void srt::RateMeasurement::pickup(const clock_time& time) +{ + // Check if the time has passed the period. + clock_interval diff; + if (!passedInterval(time, (diff))) + { + HLOGC(bslog.Debug, log << "RateMeasurement: too early, " << FormatDuration(diff) + << " with begin TS=" << FormatTime(m_beginTime) << " now=" << FormatTime(time)); + // Still collect data for the current period. + return; + } + + // Doesn't matter what `diff` really is. Enough that we have + // this time aligned with 10ms resolution calls 10 times. + + Slice slice; + { + // Lock for the moment of taking the data + sync::ScopedLock lk (m_lock); + + slice.packets = m_nInstaPackets; + slice.bytes = m_nInstaBytes; + slice.begin_time = m_beginTime; + + m_nInstaPackets = 0; + m_nInstaBytes = 0; + m_beginTime = time; + } + + HLOGC(bslog.Debug, log << "RateMeasurement: passed " << FormatDuration(diff) + << " extracted packets=" << slice.packets << " bytes=" << slice.bytes + << " now beginTS=" << FormatTime(m_beginTime)); + + // Ok, now stickpile the slice. + // If the container is empty, do not stockpile empty slices. + if (m_slices.empty() && slice.bytes == 0) + { + HLOGC(bslog.Debug, log << "... NOT COLLECING packet with " << slice.bytes << " bytes to " << m_slices.size() << " total"); + // Empty-to-empty: DO NOT stockpile. + // NOTE: the container is never cleared when it has + // any older data, so this situation is only possible: + // - in the beginning + // - in the long run if it happened that 10 slices in + // a row were empty. + // So in this case we simply wait for having the first + // packets with something notified. We also know that + // the rate was initially 0 and it remains so now. + return; + } + + // We either have something and have pushed an empty slice + // or we have pushed filled slice onto the empty container. + // Still, we need at least 5 samples to start measuring. + m_slices.push_back(slice); + + // NOTE: this is also a situation that might have happened + // in the beginning, or after a long period without sending. + // If that period happens, the sending rate is also cleared, + // in any case we need to wait for the minimum samples. + if (m_slices.size() < MIN_SLICES) + { + HLOGC(bslog.Debug, log << "... NOT MEASURING: still " << m_slices.size() << " < 5 slices"); + return; + } + + // Keep track of only 10 last slices + if (m_slices.size() > MAX_SLICES) + { + m_slices.pop_front(); + + // After a slice was removed, there's already a chance + // that all other slices are also 0. Check the minimum + // number of slices + 1 and drop all that are empty + size_t end_range = 0; // default: nothing to delete + for (size_t i = 0; i < m_slices.size(); ++i) + { + if (i > MIN_SLICES + 1) + break; + + if (m_slices[i].bytes > 0) + break; + + // This can be skipped and check the next one. + end_range = i; + } + + // PARANOID + if (end_range >= m_slices.size()) + end_range = m_slices.size() - 1; // at least 1 element must stay + + m_slices.erase(m_slices.begin(), m_slices.begin() + end_range); + + m_beginTime = m_slices[0].begin_time; + HLOGC(bslog.Debug, log << "... Dropping 1 oldest + " << end_range << " empty slices, TS=" << FormatTime(m_beginTime)); + } + + // Check also if the begin time is older than 2* period; if so, + // reset it to that value + if (m_slices.size() > 1) // with 1 always m_beginTime = m_slices[0].begin_time + { + clock_time earliest = m_slices[1].begin_time - milliseconds_from(SLICE_INTERVAL_MS*2); + if (m_beginTime < earliest) + { + HLOGC(bslog.Debug, log << "... BEGIN TIME too old TS=" << FormatTime(m_beginTime) + << " - fixed to " << FormatTime(earliest)); + m_beginTime = earliest; + m_slices[0].begin_time = earliest; + } + } + + // Ok, so we have enough samples to record the measurement. + // (so we have at least one) + Summary sum (m_slices[0].begin_time); + + for (size_t i = 0; i < m_slices.size(); ++i) + { + sum.consume(m_slices[i]); + } + + m_currentPktRate = sum.ratePackets(time); + m_currentByteRate = sum.rateBytes(time, m_SysHeaderSize + CPacket::HDR_SIZE); + + HLOGC(bslog.Debug, log << "... CALCULATED from " << m_slices.size() << " slices: total TD=" << FormatDuration(time - m_slices[0].begin_time) + << " packets=" << sum.packets << " bytes=" << sum.bytes << " pkt-rate=" << m_currentPktRate << " byte-rate=" << m_currentByteRate); +} +#endif + void srt::CUDT::construct() { m_pSndBuffer = NULL; @@ -303,6 +431,12 @@ void srt::CUDT::construct() m_bPeerTLPktDrop = false; m_bBufferWasFull = false; + // Will be updated on first send +#ifdef ENABLE_MAXREXMITBW + m_zSndAveragePacketSize = 0; + m_zSndMaxPacketSize = 0; +#endif + // Initilize mutex and condition variables. initSynch(); @@ -313,7 +447,8 @@ void srt::CUDT::construct() srt::CUDT::CUDT(CUDTSocket* parent) : m_parent(parent) #ifdef ENABLE_MAXREXMITBW - , m_SndRexmitRate(sync::steady_clock::now()) + // , m_SndRexmitRate(sync::steady_clock::now()) + , m_SndRexmitShaper(CSrtConfig::DEF_MSS) #endif , m_iISN(-1) , m_iPeerISN(-1) @@ -342,7 +477,8 @@ srt::CUDT::CUDT(CUDTSocket* parent) srt::CUDT::CUDT(CUDTSocket* parent, const CUDT& ancestor) : m_parent(parent) #ifdef ENABLE_MAXREXMITBW - , m_SndRexmitRate(sync::steady_clock::now()) + // , m_SndRexmitRate(sync::steady_clock::now()) + , m_SndRexmitShaper(CSrtConfig::DEF_MSS) #endif , m_iISN(-1) , m_iPeerISN(-1) @@ -6202,6 +6338,13 @@ SRT_REJECT_REASON srt::CUDT::setupCC() m_tsLastRspAckTime = currtime; m_tsLastSndTime.store(currtime); +#ifdef ENABLE_RATE_MEASUREMENT + // XXX NOTE: use IPv4 or IPv6 as applicable! + HLOGC(bslog.Debug, log << CONID() << "RATE-MEASUREMENT: initializing time TS=" << FormatTime(currtime)); + m_SndRegularMeasurement.init(currtime, CPacket::UDP_HDR_SIZE); + m_SndRexmitMeasurement.init(currtime, CPacket::UDP_HDR_SIZE); +#endif + HLOGC(rslog.Debug, log << CONID() << "setupCC: setting parameters: mss=" << m_config.iMSS << " maxCWNDSize/FlowWindowSize=" << m_iFlowWindowSize << " rcvrate=" << m_iDeliveryRate << "p/s (" << m_iByteDeliveryRate << "B/S)" @@ -9461,80 +9604,157 @@ void srt::CUDT::updateAfterSrtHandshake(int hsv) } } -int srt::CUDT::packLostData(CPacket& w_packet) +// XXX During sender buffer refax turn this into either returning +// a sequence number or move it to the sender buffer facility. +// [[using locked (m_RecvAckLock)]] +std::pair srt::CUDT::getCleanRexmitOffset() { - // protect m_iSndLastDataAck from updating by ACK processing - UniqueLock ackguard(m_RecvAckLock); - const steady_clock::time_point time_now = steady_clock::now(); - const steady_clock::time_point time_nak = time_now - microseconds_from(m_iSRTT - 4 * m_iRTTVar); + // This function is required to look into the loss list and + // drop all sequences that are alrady revoked from the sender + // buffer, send the drop request if needed, and return the sender + // buffer offset for the next packet to retransmit, or -1 if + // there is no retransmission candidate at the moment. for (;;) { - w_packet.set_seqno(m_pSndLossList->popLostSeq()); - if (w_packet.seqno() == SRT_SEQNO_NONE) - break; + // REPEATABLE because we might need to drop this scheduled seq. - // XXX See the note above the m_iSndLastDataAck declaration in core.h - // This is the place where the important sequence numbers for - // sender buffer are actually managed by this field here. - const int offset = CSeqNo::seqoff(m_iSndLastDataAck, w_packet.seqno()); + // Pick up the first sequence. + int32_t seq = m_pSndLossList->popLostSeq(); + if (seq == SRT_SEQNO_NONE) + { + // No loss reports, we are clear here. + return std::make_pair(SRT_SEQNO_NONE, -1); + } + + int offset = CSeqNo::seqoff(m_iSndLastDataAck, seq); if (offset < 0) { // XXX Likely that this will never be executed because if the upper // sequence is not in the sender buffer, then most likely the loss // was completely ignored. - LOGC(qrlog.Error, - log << CONID() << "IPE/EPE: packLostData: LOST packet negative offset: seqoff(seqno() " - << w_packet.seqno() << ", m_iSndLastDataAck " << m_iSndLastDataAck << ")=" << offset - << ". Continue, request DROP"); + LOGC(qslog.Error, log << CONID() + << "IPE/EPE: packLostData: LOST packet negative offset: seqoff(seqno() " + << seq << ", m_iSndLastDataAck " << m_iSndLastDataAck << ")=" << offset + << ". Continue, request DROP"); + + int32_t oldest_outdated_seq = seq; + // We have received the first outdated sequence. + // Loop on to find out more. + // Interrupt on the first non-outdated sequence + // or when the loss list gets empty. + + for (;;) + { + seq = m_pSndLossList->popLostSeq(); + if (seq == SRT_SEQNO_NONE) + { + offset = -1; // set it because this will be returned. + break; + } + + offset = CSeqNo::seqoff(m_iSndLastDataAck, seq); + if (offset >= 0) + break; + } + + // Now we have 'seq' that is either non-sequence + // or a valid sequence. We can safely exit with this + // value from this branch. We just need to manage + // the drop report. // No matter whether this is right or not (maybe the attack case should be // considered, and some LOSSREPORT flood prevention), send the drop request // to the peer. int32_t seqpair[2] = { - w_packet.seqno(), + oldest_outdated_seq, CSeqNo::decseq(m_iSndLastDataAck) }; - HLOGC(qrlog.Debug, - log << CONID() << "PEER reported LOSS not from the sending buffer - requesting DROP: #" - << MSGNO_SEQ::unwrap(w_packet.msgflags()) << " SEQ:" << seqpair[0] << " - " << seqpair[1] << "(" - << (-offset) << " packets)"); + HLOGC(qslog.Debug, + log << CONID() << "PEER reported LOSS not from the sending buffer - requesting DROP: %(" + << seqpair[0] << " - " << seqpair[1] << ") (" << (-offset) << " packets)"); // See interpretation in processCtrlDropReq(). We don't know the message number, // so we request that the drop be exclusively sequence number based. int32_t msgno = SRT_MSGNO_CONTROL; - sendCtrl(UMSG_DROPREQ, &msgno, seqpair, sizeof(seqpair)); + } + + // We have exit anyway with the right sequence or no sequence. + if (seq == SRT_SEQNO_NONE) + return std::make_pair(SRT_SEQNO_NONE, -1); + + // For the right sequence though check also the rigt time. + const steady_clock::time_point time_now = steady_clock::now(); + if (!checkRexmitRightTime(offset, time_now)) + { + // Ignore this, even though valid, sequence, and pick up + // the next from the list. continue; } - if (m_bPeerNakReport && m_config.iRetransmitAlgo != 0) + // Ok, we have what we needed one way or another. + return std::make_pair(seq, offset); + } +} + +// [[using locked (m_RecvAckLock)]] +bool srt::CUDT::checkRexmitRightTime(int offset, const srt::sync::steady_clock::time_point& current_time) +{ + // If not configured, the time is always right + if (!m_bPeerNakReport || m_config.iRetransmitAlgo == 0) + return true; + + const steady_clock::time_point time_nak = current_time - microseconds_from(m_iSRTT - 4 * m_iRTTVar); + const steady_clock::time_point tsLastRexmit = m_pSndBuffer->getPacketRexmitTime(offset); + + if (tsLastRexmit >= time_nak) + { +#if ENABLE_HEAVY_LOGGING + ostringstream last_rextime; + if (is_zero(tsLastRexmit)) { - const steady_clock::time_point tsLastRexmit = m_pSndBuffer->getPacketRexmitTime(offset); - if (tsLastRexmit >= time_nak) - { - HLOGC(qrlog.Debug, log << CONID() << "REXMIT: ignoring seqno " - << w_packet.seqno() << ", last rexmit " << (is_zero(tsLastRexmit) ? "never" : FormatTime(tsLastRexmit)) - << " RTT=" << m_iSRTT << " RTTVar=" << m_iRTTVar - << " now=" << FormatTime(time_now)); - continue; - } + last_rextime << "REXMIT FIRST TIME"; } + else + { + last_rextime << "REXMITTED " << FormatDuration(current_time - tsLastRexmit) + << " ago at " << FormatTime(tsLastRexmit); + } + HLOGC(qslog.Debug, log << CONID() << "REXMIT: ignoring %" << CSeqNo::incseq(m_iSndLastDataAck, offset) + << " " << last_rextime.str() + << ", RTT: ~" << FormatValue(m_iSRTT, 1000, "ms") + << " <>" << FormatValue(m_iRTTVar, 1000, "") + << " now=" << FormatTime(current_time)); +#endif + return false; + } + + return true; +} - typedef CSndBuffer::DropRange DropRange; +// [[using locked (m_RecvAckLock)]] +int srt::CUDT::extractCleanRexmitPacket(int32_t seqno, int offset, CPacket& w_packet, + srt::sync::steady_clock::time_point& w_tsOrigin) +{ + // REPEATABLE BLOCK (not a real loop) + for (;;) + { + typedef CSndBuffer::DropRange DropRange; DropRange buffer_drop; - steady_clock::time_point tsOrigin; - const int payload = m_pSndBuffer->readData(offset, (w_packet), (tsOrigin), (buffer_drop)); + w_packet.set_seqno(seqno); + + const int payload = m_pSndBuffer->readData(offset, (w_packet), (w_tsOrigin), (buffer_drop)); if (payload == CSndBuffer::READ_DROP) { SRT_ASSERT(CSeqNo::seqoff(buffer_drop.seqno[DropRange::BEGIN], buffer_drop.seqno[DropRange::END]) >= 0); - HLOGC(qrlog.Debug, - log << CONID() << "loss-reported packets expired in SndBuf - requesting DROP: #" - << buffer_drop.msgno << " %(" << buffer_drop.seqno[DropRange::BEGIN] << " - " - << buffer_drop.seqno[DropRange::END] << ")"); + HLOGC(qslog.Debug, + log << CONID() << "loss-reported packets expired in SndBuf - requesting DROP: #" + << buffer_drop.msgno << " %(" << buffer_drop.seqno[DropRange::BEGIN] << " - " + << buffer_drop.seqno[DropRange::END] << ")"); sendCtrl(UMSG_DROPREQ, &buffer_drop.msgno, buffer_drop.seqno, sizeof(buffer_drop.seqno)); // skip all dropped packets @@ -9542,43 +9762,133 @@ int srt::CUDT::packLostData(CPacket& w_packet) m_iSndCurrSeqNo = CSeqNo::maxseq(m_iSndCurrSeqNo, buffer_drop.seqno[DropRange::END]); continue; } - else if (payload == CSndBuffer::READ_NONE) - continue; - // The packet has been ecrypted, thus the authentication tag is expected to be stored - // in the SND buffer as well right after the payload. - if (m_pCryptoControl && m_pCryptoControl->getCryptoMode() == CSrtConfig::CIPHER_MODE_AES_GCM) + if (payload == CSndBuffer::READ_NONE) { - w_packet.setLength(w_packet.getLength() + HAICRYPT_AUTHTAG_MAX); + LOGC(qslog.Error, log << CONID() << "loss-reported packet %" << w_packet.seqno() << " NOT FOUND in the sender buffer"); + return 0; } - // At this point we no longer need the ACK lock, - // because we are going to return from the function. - // Therefore unlocking in order not to block other threads. - ackguard.unlock(); + return payload; + } + +} - enterCS(m_StatsLock); - m_stats.sndr.sentRetrans.count(payload); - leaveCS(m_StatsLock); +int srt::CUDT::packLostData(CPacket& w_packet) +{ +#if ENABLE_MAXREXMITBW + +#ifdef ENABLE_RATE_MEASUREMENT +#define IF_RATE_MEA(expr) expr +#else +#define IF_RATE_MEA(expr) +#endif - // Despite the contextual interpretation of packet.m_iMsgNo around - // CSndBuffer::readData version 2 (version 1 doesn't return -1), in this particular - // case we can be sure that this is exactly the value of PH_MSGNO as a bitset. - // So, set here the rexmit flag if the peer understands it. - if (m_bPeerRexmitFlag) + // XXX NOTE: If you refactor the sender buffer so that the random access + // is possible, you might be able to extract the exact packet size to + // check for enough tokens and consume them in one step. + + bool maxrexmitbw_enabled = m_config.llMaxRexmitBW >= 0; + if (maxrexmitbw_enabled) + { + IF_RATE_MEA( IF_HEAVY_LOGGING(const int64_t iRexmitRateMeasured = m_SndRexmitMeasurement.rateBytes()); ) + size_t granted_size = m_zSndAveragePacketSize; + size_t len = granted_size + CPacket::HDR_SIZE + CPacket::UDP_HDR_SIZE; + + if (!m_SndRexmitShaper.enoughTokens(len)) { - w_packet.set_msgflags(w_packet.msgflags() | PACKET_SND_REXMIT); + HLOGC(qslog.Debug, log << "REXMIT-SH: BLOCKED pkt est/len=" << len << " exceeds " << m_SndRexmitShaper.ntokens() + << " tokens, rate/Bps:used=" << m_SndRexmitShaper.usedRate_Bps() << ",avail=" << m_SndRexmitShaper.availRate_Bps() + IF_RATE_MEA( << " (measured: " << FormatValue(iRexmitRateMeasured, 1024, "kBps") << ")" ) + ); + return 0; } - setDataPacketTS(w_packet, tsOrigin); -#ifdef ENABLE_MAXREXMITBW - m_SndRexmitRate.addSample(time_now, 1, w_packet.getLength()); + HLOGC(qslog.Debug, log << "REXMIT-SH: ALLOWED pkt est/len=" << len << " allowed, budget " << m_SndRexmitShaper.ntokens() + << " tokens, rate/Bps:used=" << m_SndRexmitShaper.usedRate_Bps() << ",avail=" << m_SndRexmitShaper.availRate_Bps() + IF_RATE_MEA( << " (measured: " << FormatValue(iRexmitRateMeasured, 1024, "kBps") << ")" ) + ); + } + +#undef IF_RATE_MEA #endif - return payload; + // Original sending time of the packet, to be stamped after filling. + steady_clock::time_point tsOrigin; + + { + // protect m_iSndLastDataAck from updating by ACK processing + ScopedLock ackguard(m_RecvAckLock); + int32_t seqno; + int offset; + + // Get the first sequence for retransmission, bypassing and taking care of + // those that are in the forgotten region, as well as required to be rejected. + Tie2(seqno, offset) = getCleanRexmitOffset(); + + if (seqno == SRT_SEQNO_NONE) + return 0; + + // Extract the packet from the sender buffer that is mapped to the expected sequence + // number, bypassing and taking care of those that are decided to be dropped. + const int payload = extractCleanRexmitPacket(seqno, offset, (w_packet), (tsOrigin)); + if (payload <= 0) + return 0; } - return 0; + HLOGC(qslog.Debug, log << CONID() << "packed REXMIT packet %" << w_packet.seqno() << " size=" << w_packet.getLength() + << " - still " << m_pSndLossList->getLossLength() << " LOSS ENTRIES left"); + + // POST-EXTRACTION length fixes. + + // The packet has been ecrypted, thus the authentication tag is expected to be stored + // in the SND buffer as well right after the payload. + if (m_pCryptoControl && m_pCryptoControl->getCryptoMode() == CSrtConfig::CIPHER_MODE_AES_GCM) + { + w_packet.setLength(w_packet.getLength() + HAICRYPT_AUTHTAG_MAX); + } + +#if ENABLE_MAXREXMITBW + if (maxrexmitbw_enabled) + { + // Token consumption will only happen when the retransmission + // effectively happens. + // XXX NOTE: In version 1.6.0 use the IP-version dependent value for UDP_HDR_SIZE + size_t network_size = w_packet.getLength() + CPacket::HDR_SIZE + CPacket::UDP_HDR_SIZE; + m_SndRexmitShaper.consumeTokens(network_size); + HLOGC(qslog.Debug, log << "REXMIT-SH: consumed " << network_size << " tokens, remain " << m_SndRexmitShaper.ntokens()); + } +#endif + + enterCS(m_StatsLock); + m_stats.sndr.sentRetrans.count(w_packet.getLength()); + leaveCS(m_StatsLock); + + // Despite the contextual interpretation of packet.m_iMsgNo around + // CSndBuffer::readData version 2 (version 1 doesn't return -1), in this particular + // case we can be sure that this is exactly the value of PH_MSGNO as a bitset. + // So, set here the rexmit flag if the peer understands it. + if (m_bPeerRexmitFlag) + { + w_packet.set_msgflags(w_packet.msgflags() | PACKET_SND_REXMIT); + } + setDataPacketTS(w_packet, tsOrigin); + +#ifdef ENABLE_MAXREXMITBW + // XXX OLD rexmit measurement + // m_SndRexmitRate.addSample(time_now, 1, w_packet.getLength()); +#endif + + // XXX Consider calculating the total packet length once you have a possibility + // to get the in-connection used IP version. +#ifdef ENABLE_RATE_MEASUREMENT + m_SndRexmitMeasurement.dataUpdate(1, w_packet.getLength()); + HLOGC(bslog.Debug, log << "RateMeasurement: REXMIT, pkt-size=" << w_packet.getLength() + << " - COLLECTED pkts=" << m_SndRexmitMeasurement.m_nInstaPackets + << " bytes=" << m_SndRexmitMeasurement.m_nInstaBytes); +#endif + + return w_packet.getLength(); } #if SRT_DEBUG_TRACE_SND @@ -9703,15 +10013,69 @@ void srt::CUDT::setDataPacketTS(CPacket& p, const time_point& ts) p.set_timestamp(makeTS(ts, tsStart)); } -bool srt::CUDT::isRetransmissionAllowed(const time_point& tnow SRT_ATR_UNUSED) +bool srt::CUDT::isRegularSendingPriority() { - // Prioritization of original packets only applies to Live CC. + // In order to have regular packets take precedense over retransmitted: + // - SRTO_TLPKTDROP = true + // - SRTO_MESSAGEAPI = true + // NOTE: + // - tlpktdrop is ignored in stream mode + // - messageapi without tlpktdrop is possible in non-live message mode + // - Live mode without tlpktdrop is possible and in this mode also the + // retransmitted packets should have priority. if (!m_bPeerTLPktDrop || !m_config.bMessageAPI) - return true; + return false; - // TODO: lock sender buffer? + // XXX NOTE: the current solution is simple - the regular packet takes precedence + // over a retransmitted packet, if there is at least one such packet already + // scheduled. + // + // This probably isn't the most wanted solution, some more elaborate condition + // might be better in some situations. For example, it should be acceptable + // that a packet that has very little time to be recovered is sent before a + // regular packet that has still STT + Latency time to deliver. The regular + // packets should still be favorized, but not necessarily at the expence of + // dismissing a recovery chance that wouldn't endanger the delivery of a regular + // packet. Criteria might be various, for example, the number of scheduled + // packets and their late delivery time might be taken into account. const time_point tsNextPacket = m_pSndBuffer->peekNextOriginal(); + if (tsNextPacket != time_point()) + { + // Have regular packet and we decided they have a priority. + HLOGC(qslog.Debug, log << "REXMIT-SH: BLOCKED because regular packets have priority"); + return true; + } + return false; +} + +namespace srt { +#ifdef ENABLE_MAXREXMITBW +static sync::steady_clock::duration optimisticSingleTripDuration(int rttval, int rttvar) +{ + int lowrtt = rttval - rttvar; + if (lowrtt < 0) // bullshit? + lowrtt = rttval; + + int stt = lowrtt/2; + + // Make sure that burst period has at least this value + sync::steady_clock::duration stt_td = sync::microseconds_from(stt); + + // Still, prevent from setting enormous STT values + if (stt_td < sync::seconds_from(4)) + { + return stt_td; + } + return sync::steady_clock::duration(); +} +#endif +} + +void srt::CUDT::updateSenderMeasurements(bool can_rexmit SRT_ATR_UNUSED) +{ + const time_point tnow SRT_ATR_UNUSED = steady_clock::now(); + #if SRT_DEBUG_TRACE_SND const int buffdelay_ms = count_milliseconds(m_pSndBuffer->getBufferingDelay(tnow)); // If there is a small loss, still better to retransmit. If timespan is already big, @@ -9727,31 +10091,37 @@ bool srt::CUDT::isRetransmissionAllowed(const time_point& tnow SRT_ATR_UNUSED) g_snd_logger.state.msTimespanTh = threshold_ms_min; g_snd_logger.state.msNextUniqueToSend = msNextUniqueToSend; g_snd_logger.state.usElapsedLastDrop = count_microseconds(tnow - m_tsLastTLDrop); - g_snd_logger.state.canRexmit = false; + g_snd_logger.state.canRexmit = can_rexmit; #endif - if (tsNextPacket != time_point()) - { - // Can send original packet, so just send it - return false; - } - #ifdef ENABLE_MAXREXMITBW + /* OLD rate estimator; CODE for packLostData()! m_SndRexmitRate.addSample(tnow, 0, 0); // Update the estimation. const int64_t iRexmitRateBps = m_SndRexmitRate.getRate(); - const int64_t iRexmitRateLimitBps = m_config.llMaxRexmitBW; if (iRexmitRateLimitBps >= 0 && iRexmitRateBps > iRexmitRateLimitBps) { // Too many retransmissions, so don't send anything. // TODO: When to wake up next time? return false; } -#endif + */ + const int64_t iRexmitRateLimitBps = m_config.llMaxRexmitBW; + if (iRexmitRateLimitBps >= 0) + { + int b4_tokens SRT_ATR_UNUSED = m_SndRexmitShaper.ntokens(); + m_SndRexmitShaper.setBitrate(iRexmitRateLimitBps); + duration stt_td = optimisticSingleTripDuration(m_iSRTT, m_iRTTVar); + if (stt_td != duration()) + m_SndRexmitShaper.setMinimumBurstPeriod(stt_td); + m_SndRexmitShaper.tick(tnow); + int a4_tokens SRT_ATR_UNUSED = m_SndRexmitShaper.ntokens(); -#if SRT_DEBUG_TRACE_SND - g_snd_logger.state.canRexmit = true; + HLOGC(qslog.Debug, log << "REXMIT-SW: tick tokens updated: " << b4_tokens << " -> " << a4_tokens + << "/" << m_SndRexmitShaper.maxTokens() << " period=" + << FormatDuration(m_SndRexmitShaper.burstPeriod())); + } #endif - return true; + } bool srt::CUDT::packData(CPacket& w_packet, steady_clock::time_point& w_nexttime, sockaddr_any& w_src_addr) @@ -9780,9 +10150,25 @@ bool srt::CUDT::packData(CPacket& w_packet, steady_clock::time_point& w_nexttime if (!m_bOpened) return false; - payload = isRetransmissionAllowed(enter_time) - ? packLostData((w_packet)) - : 0; + // This function returns true if there is a regular packet candidate waiting + // for being sent and sending this packet has a prioriry over retransmission candidate. + if (!isRegularSendingPriority()) + { + payload = packLostData((w_packet)); +#if ENABLE_MAXREXMITBW + if (payload == 0) + { + HLOGC(qslog.Debug, log << "REXMIT-SH: no rexmit required, remain " << m_SndRexmitShaper.ntokens() << " tokens"); + } +#endif + } + else + { + HLOGC(qslog.Debug, log << "REXMIT: retransmission SUPERSEDED, proceed with regular candidate"); + } + + // Updates the data that will be next used in packLostData() in next calls. + updateSenderMeasurements(payload != 0); IF_HEAVY_LOGGING(const char* reason); // The source of the data packet (normal/rexmit/filter) if (payload > 0) @@ -9808,6 +10194,18 @@ bool srt::CUDT::packData(CPacket& w_packet, steady_clock::time_point& w_nexttime m_tdSendTimeDiff = steady_clock::duration(); return false; } + +#if ENABLE_MAXREXMITBW + if (m_zSndAveragePacketSize > 0) + { + m_zSndAveragePacketSize = avg_iir<16>(m_zSndAveragePacketSize, w_packet.getLength()); + } + else + { + m_zSndAveragePacketSize = w_packet.getLength(); + } + m_zSndMaxPacketSize = std::max(m_zSndMaxPacketSize, w_packet.getLength()); +#endif new_packet_packed = true; // every 16 (0xF) packets, a packet pair is sent @@ -10044,6 +10442,11 @@ bool srt::CUDT::packUniqueData(CPacket& w_packet) g_snd_logger.trace(); #endif +#ifdef ENABLE_RATE_MEASUREMENT + HLOGC(bslog.Debug, log << "RATE-MEASUREMENT: REGULAR, pkt-size=" << w_packet.getLength()); + m_SndRegularMeasurement.dataUpdate(1, w_packet.getLength()); +#endif + return true; } @@ -11773,6 +12176,11 @@ void srt::CUDT::checkTimers() << " pkt-count=" << m_iPktCount << " liteack-count=" << m_iLightACKCount); #endif +#ifdef ENABLE_RATE_MEASUREMENT + m_SndRegularMeasurement.pickup(currtime); + m_SndRexmitMeasurement.pickup(currtime); +#endif + // Check if it is time to send ACK int debug_decision = checkACKTimer(currtime); diff --git a/srtcore/core.h b/srtcore/core.h index 6bf75466e..0e4b4b5d1 100644 --- a/srtcore/core.h +++ b/srtcore/core.h @@ -159,6 +159,133 @@ class CUDTSocket; class CUDTGroup; #endif +#ifdef ENABLE_RATE_MEASUREMENT +struct RateMeasurement +{ + typedef sync::steady_clock clock_type; + typedef clock_type::time_point clock_time; + typedef clock_type::duration clock_interval; + + static const int SLICE_INTERVAL_MS = 20; + static const size_t MIN_SLICES = 5; // min + static const size_t MAX_SLICES = 10; + + sync::Mutex m_lock; + + size_t m_SysHeaderSize; + + // Instantaneous data updated by sending packets + // AFFINITY: Snd:worker thread + // MODE: write + sync::atomic m_nInstaPackets; + sync::atomic m_nInstaBytes; + + void dataUpdate(int packets, int bytes) + { + sync::ScopedLock lk (m_lock); + m_nInstaPackets = m_nInstaPackets + packets; + m_nInstaBytes = m_nInstaBytes + bytes; + } + + // Cached last measurement + // AFFINITY: Snd:worker thread MODE: read + // AFFINITY: Rcv:worker thread MODE: write + // (not locking because one data is updated at a time). + sync::atomic m_currentPktRate; + sync::atomic m_currentByteRate; + int64_t rateBytes() const + { + return m_currentByteRate; + } + int64_t ratePackets() const + { + return m_currentPktRate; + } + + // Time of the last checkpoint or initialization + clock_time m_beginTime; + + struct Slice + { + int packets; + int bytes; + clock_time begin_time; + + Slice(): packets(0), bytes(0) {} // clock_time is non-POD + Slice(const clock_time& t): packets(0), bytes(0), begin_time(t) {} + + }; + + struct Summary: Slice + { + Summary(const clock_time& earliest_time): Slice(earliest_time), nsamples(0) {} + + unsigned char nsamples; + + void consume(const Slice& another) + { + if (is_zero(begin_time)) + begin_time = another.begin_time; + packets += another.packets; + bytes += another.bytes; + nsamples++; + } + + int64_t ratePackets(const clock_time& end_time) const + { + double packets_per_micro = double(packets) * 1000 * 1000; + double rate = packets_per_micro / count_microseconds(end_time - begin_time); + return rate; + } + + int64_t rateBytes(const clock_time& end_time, size_t hdr_size) const + { + double data_bytes = bytes + (packets * hdr_size); + double bytes_per_micro = data_bytes * 1000 * 1000; + double rate = bytes_per_micro / count_microseconds(end_time - begin_time); + return rate; + } + }; + + std::deque m_slices; + + RateMeasurement(): + m_SysHeaderSize(CPacket::UDP_HDR_SIZE), // XXX NOTE: IPv4 ! + m_nInstaPackets(0), + m_nInstaBytes(0), + m_currentPktRate(0), + m_currentByteRate(0) + { + } + + bool passedInterval(const clock_time& this_time, clock_interval& w_interval) + { + if (is_zero(m_beginTime)) + { + sync::ScopedLock lk (m_lock); + m_beginTime = this_time; + w_interval = clock_interval(0); + return false; + } + w_interval = this_time - m_beginTime; + return (sync::count_milliseconds(w_interval) > SLICE_INTERVAL_MS); + } + + // This is to be called in constructor, only once. + void init(const clock_time& time, size_t sys_hdr_size) + { + // Just formally. + sync::ScopedLock lk (m_lock); + m_beginTime = time; + m_SysHeaderSize = sys_hdr_size; + } + + // AFFINITY: Rcv:worker thread (update thread) + // This function should be called in regular time periods. + void pickup(const clock_time& time); +}; +#endif + // XXX REFACTOR: The 'CUDT' class is to be merged with 'CUDTSocket'. // There's no reason for separating them, there's no case of having them // anyhow managed separately. After this is done, with a small help with @@ -593,11 +720,14 @@ class CUDT SRT_ATTR_REQUIRES2(m_RecvAckLock, m_StatsLock) int sndDropTooLate(); - /// @bried Allow packet retransmission. - /// Depending on the configuration mode (live / file), retransmission - /// can be blocked if e.g. there are original packets pending to be sent. - /// @return true if retransmission is allowed; false otherwise. - bool isRetransmissionAllowed(const time_point& tnow); + // Returns true if there is a regular packet waiting for sending + // and sending regular packets has priority over retransmitted ones. + bool isRegularSendingPriority(); + + // Performs updates in the measurement variables after possible + // extraction of a retransmission packet. Some are for debug purposes, + // others for retransmission rate measurement. + void updateSenderMeasurements(bool can_rexmit); /// Connect to a UDT entity as per hs request. This will update /// required data in the entity, then update them also in the hs structure, @@ -873,7 +1003,16 @@ class CUDT CSndLossList* m_pSndLossList; // Sender loss list CPktTimeWindow<16, 16> m_SndTimeWindow; // Packet sending time window #ifdef ENABLE_MAXREXMITBW - CSndRateEstimator m_SndRexmitRate; // Retransmission rate estimation. + size_t m_zSndAveragePacketSize; + size_t m_zSndMaxPacketSize; + // XXX Old rate estimator for rexmit + // CSndRateEstimator m_SndRexmitRate; // Retransmission rate estimation. + CShaper m_SndRexmitShaper; + +#ifdef ENABLE_RATE_MEASUREMENT + RateMeasurement m_SndRegularMeasurement; // Regular rate measurement + RateMeasurement m_SndRexmitMeasurement; // Retransmission rate measurement +#endif #endif atomic_duration m_tdSendInterval; // Inter-packet time, in CPU clock cycles @@ -1131,6 +1270,11 @@ class CUDT /// @return payload size on success, <=0 on failure int packLostData(CPacket &packet); + std::pair getCleanRexmitOffset(); + bool checkRexmitRightTime(int offset, const srt::sync::steady_clock::time_point& current_time); + int extractCleanRexmitPacket(int32_t seqno, int offset, CPacket& w_packet, + srt::sync::steady_clock::time_point& w_tsOrigin); + /// Pack a unique data packet (never sent so far) in CPacket for sending. /// @param packet [in, out] a CPacket structure to fill. /// diff --git a/srtcore/group.cpp b/srtcore/group.cpp index 339512573..ed18a72d6 100644 --- a/srtcore/group.cpp +++ b/srtcore/group.cpp @@ -730,6 +730,10 @@ static bool getOptDefault(SRT_SOCKOPT optname, void* pw_optval, int& w_optlen) RD(true); case SRTO_MAXBW: RD(int64_t(-1)); +#ifdef ENABLE_MAXREXMITBW + case SRTO_MAXREXMITBW: + RD(int64_t(-1)); +#endif case SRTO_INPUTBW: RD(int64_t(-1)); case SRTO_MININPUTBW: diff --git a/srtcore/utilities.h b/srtcore/utilities.h index 8781b80e7..cf3add810 100644 --- a/srtcore/utilities.h +++ b/srtcore/utilities.h @@ -789,12 +789,6 @@ inline void insert_uniq(std::vector& v, const ArgValue& val) v.push_back(val); } -template -inline std::pair Tie(Type1& var1, Type2& var2) -{ - return std::pair(var1, var2); -} - // This can be used in conjunction with Tie to simplify the code // in loops around a whole container: // list::const_iterator it, end; @@ -972,6 +966,16 @@ inline void AccumulatePassFilterParallel(const int* p, size_t size, PassFilter +inline Type Bounds(Type lower, Type value, Type upper) +{ + if (value < lower) + return lower; + if (value > upper) + return upper; + return value; +} + inline std::string FormatBinaryString(const uint8_t* bytes, size_t size) { diff --git a/test/test_fec_rebuilding.cpp b/test/test_fec_rebuilding.cpp index b39345c40..3910c959b 100644 --- a/test/test_fec_rebuilding.cpp +++ b/test/test_fec_rebuilding.cpp @@ -91,6 +91,7 @@ class TestFECRebuilding: public srt::Test { delete fec; } + }; static std::future spawn_connect(SRTSOCKET s, sockaddr_in& sa, int timeout_ms = 1000) @@ -374,6 +375,9 @@ TEST(TestFEC, ConnectionReorder) ASSERT_NE(srt_setsockflag(s, SRTO_PACKETFILTER, fec_config1, (sizeof fec_config1)-1), -1); ASSERT_NE(srt_setsockflag(l, SRTO_PACKETFILTER, fec_config2, (sizeof fec_config2)-1), -1); + int conntimeo = 10000; + ASSERT_NE(srt_setsockflag(s, SRTO_CONNTIMEO, &conntimeo, sizeof (conntimeo)), SRT_ERROR); + srt_listen(l, 1); auto connect_res = spawn_connect(s, sa); From c09532f8edf28de91854f975bb16e643e8085ed9 Mon Sep 17 00:00:00 2001 From: Maria Sharabayko Date: Mon, 27 Oct 2025 11:06:48 +0100 Subject: [PATCH 14/19] [docs] Added link to the blogpost with - Live Streaming using SRT with QUIC Datagrams - research --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 0e18ff4a5..e6e7f7905 100644 --- a/README.md +++ b/README.md @@ -159,6 +159,7 @@ In live streaming configurations, the SRT protocol maintains a constant end-to-e - [RTMP vs. SRT: Comparing Latency and Maximum Bandwidth](https://www.haivision.com/resources/white-paper/srt-versus-rtmp/) White Paper. - [Documentation on GitHub](./docs#documentation-overview) with SRT API documents, features decsriptions, etc. - The SRT Protocol Internet Draft: [Datatracker](https://datatracker.ietf.org/doc/draft-sharabayko-srt/) | [Latest Version](https://datatracker.ietf.org/doc/html/draft-sharabayko-srt-01) | [Latest Working Copy](https://haivision.github.io/srt-rfc/draft-sharabayko-srt.html) | [GitHub Repo](https://github.com/Haivision/srt-rfc) +- If you are curious about live streaming using SRT with QUIC datagrams as an alternative to UDP transport, take a look at the following [blog post](https://medium.com/innovation-labs-blog/live-streaming-using-srt-with-quic-datagrams-7896f7ce7bf3?source=friends_link&sk=d0a00e79861d89673e27a04260f279b5) on Medium. ## Build Instructions From f0368c6998fbf6f789f8e484bdc6ea6f4a48a750 Mon Sep 17 00:00:00 2001 From: Sektor van Skijlen Date: Tue, 25 Nov 2025 08:50:17 +0100 Subject: [PATCH 15/19] [build] Added handling of LIBSRT_ prefix when setting cmake options for SRT build (#3219) * [BUILD] Added handling of LIBSRT_ prefix when setting cmake options for SRT build * Added documentation * Apply suggestions from code review Co-authored-by: stevomatthews * Fixed text alignment after doc review --------- Co-authored-by: Mikolaj Malecki Co-authored-by: stevomatthews --- CMakeLists.txt | 10 ++++++++++ docs/build/build-options.md | 33 +++++++++++++++++++++++++++++++++ scripts/haiUtil.cmake | 13 +++++++++++++ 3 files changed, 56 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 6e19caa2c..cf9b360ed 100755 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -122,6 +122,16 @@ endforeach() # SRT_MAVG_SAMPLING_RATE 40 /* Max sampling rate */ # SRT_ENABLE_FREQUENT_LOG_TRACE 0 : set to 1 to enable printing reason for suppressed freq logs +# CMake offers sometimes OLD behavior, where names that are defined below with +# option() function, will be deleted if they are not in cache or do not have set +# type - so effectively options provided through LIBSRT_* prefix would be rejected. +# We do want these variables, obviously. +cmake_policy(SET CMP0077 NEW) + +# Import all options preceded with LIBSRT_ likely from a parent scope. +# Names are not being checked, at worst it will set an unused variable. +srt_import_parent_options() + # option defaults set(ENABLE_HEAVY_LOGGING_DEFAULT OFF) diff --git a/docs/build/build-options.md b/docs/build/build-options.md index e09ea41bb..05fcf8be4 100644 --- a/docs/build/build-options.md +++ b/docs/build/build-options.md @@ -17,6 +17,39 @@ Additional information on building for Windows is available in the document and in the [SRT CookBook](https://srtlab.github.io/srt-cookbook/getting-started/build-on-windows/). +## Building as a subproject + +The CMake tool offers the ability to add a complete project as a subdirectory. +Variables used by the SRT project in this case remain in their own scope, but +all variables from the parent scope are reflected. In order to prevent name +clashes for option-designating variables, SRT provides a namespace-like +prefixing for the optional variables it uses. If you want to configure optional +variables from the level of `CMakeLists.txt` of the parent project, use the +`LIBSRT_` prefix for the option names. + +This will not prevent the variables from being seen as derived in SRT project +scope, but if you explicitly set a variable this way, it will be set to the +desired value inside the SRT project. It will not set the same variable in the +parent project, and it will also override (locally in SRT project only) any +value of a variable with the same name in the parent project. + +For example, if you want to set `ENABLE_SHARED=OFF` in the parent project, +simply do: + +``` +set (LIBSRT_ENABLE_SHARED OFF) +``` + +If you already have a variable named `ENABLE_SHARED` in your project (existing +before the call to `add_subdirectory` with SRT), its value will be derived in +the SRT project, unless you override it by setting `LIBSRT_ENABLE_SHARED` to a +different value. + +Note that the trick works simply by getting the actual variable name through +cutting off the `LIBSRT_` prefix; the check whether this variable is of any use +will be done after that. + + ## List of Build Options The following table lists available build options in alphabetical order. diff --git a/scripts/haiUtil.cmake b/scripts/haiUtil.cmake index 222661989..312480e1e 100644 --- a/scripts/haiUtil.cmake +++ b/scripts/haiUtil.cmake @@ -288,3 +288,16 @@ function (parse_compiler_type wct _type _suffix) endif() endif() endfunction() + +macro (srt_import_parent_options) + getVarsWith(LIBSRT_ extoptions) + foreach(ef ${extoptions}) + set (val ${${ef}}) + string(LENGTH LIBSRT_ pflen) + string(LENGTH ${ef} eflen) + math(EXPR alen ${eflen}-${pflen}) + string(SUBSTRING ${ef} ${pflen} ${alen} ef) + message(STATUS "IMPORTED OPTION: ${ef}='${val}'") + set (${ef} "${val}") + endforeach() +endmacro() From f8a53dadfb4d023405c6b49699f2f8bdf0b7fdd6 Mon Sep 17 00:00:00 2001 From: Tomoya Fujita Date: Tue, 16 Dec 2025 17:37:02 +0900 Subject: [PATCH 16/19] [build] Ubuntu 23.04 reached end-of-life in January 2024. (#3261) Signed-off-by: Tomoya.Fujita --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index e6e7f7905..4255c0663 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ [![Build Status Linux and macOS][travis-badge]][travis] [![Build Status Windows][appveyor-badge]][appveyor] -[![Ubuntu 23.04][Ubuntu-badge]][Ubuntu-package] +[![Ubuntu 24.04][Ubuntu-badge]][Ubuntu-package] [![Fedora 37][fedora-badge]][fedora-package] [![Debian][debian-badge]][debian-package] [![Homebrew][Homebrew-badge]][Homebrew-package] @@ -242,4 +242,4 @@ By contributing code to the SRT project, you agree to license your contribution [homebrew-badge]: https://repology.org/badge/version-for-repo/homebrew/srt.svg [Ubuntu-package]: https://repology.org/project/srt/versions -[Ubuntu-badge]: https://repology.org/badge/version-for-repo/ubuntu_23_04/srt.svg +[Ubuntu-badge]: https://repology.org/badge/version-for-repo/ubuntu_24_04/srt.svg From 72f0c6e0e9a186adec428a145f4e085412d96796 Mon Sep 17 00:00:00 2001 From: Sektor van Skijlen Date: Tue, 16 Dec 2025 09:37:59 +0100 Subject: [PATCH 17/19] [BUG] [core] Prevent blocking srt_close call from interrupting connection (#3259) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [BUG] Prevent blocking srt_close call from interrupting connection * Test: added check that srt_close call is not blocked. Ensured that waiting is interrupt ed on close. Ensured that the socket is closed right after wait quit * Fixed incorrect C++17 literals * Increased wait time tolerance for srt_close (failing on some slower CI machines) --------- Co-authored-by: Mikołaj Małecki --- srtcore/api.cpp | 5 +- srtcore/api.h | 5 ++ srtcore/core.cpp | 3 +- srtcore/core.h | 1 + test/test_connection_timeout.cpp | 122 ++++++++++++++++++++++++++++++- test/test_file_transmission.cpp | 16 +++- 6 files changed, 146 insertions(+), 6 deletions(-) diff --git a/srtcore/api.cpp b/srtcore/api.cpp index 22b093dcc..0b96a33bc 100644 --- a/srtcore/api.cpp +++ b/srtcore/api.cpp @@ -2144,6 +2144,9 @@ void srt::CUDTUnited::deleteGroup_LOCKED(CUDTGroup* g) int srt::CUDTUnited::close(CUDTSocket* s) { + // Set the closing flag BEFORE you attempt to acquire + s->setBreaking(); + HLOGC(smlog.Debug, log << s->core().CONID() << "CLOSE. Acquiring control lock"); ScopedLock socket_cg(s->m_ControlLock); HLOGC(smlog.Debug, log << s->core().CONID() << "CLOSING (removing from listening, closing CUDT)"); @@ -2973,7 +2976,7 @@ void srt::CUDTUnited::removeSocket(const SRTSOCKET u) return; } - LOGC(smlog.Note, log << "@" << s->m_SocketID << " busy=" << s->isStillBusy()); + HLOGC(smlog.Note, log << "@" << s->m_SocketID << " busy=" << s->isStillBusy()); #if ENABLE_BONDING if (s->m_GroupOf) diff --git a/srtcore/api.h b/srtcore/api.h index 42b97ef32..789d68d0c 100644 --- a/srtcore/api.h +++ b/srtcore/api.h @@ -216,6 +216,11 @@ class CUDTSocket core().m_bClosing = true; } + void setBreaking() + { + core().m_bBreaking = true; + } + /// This does the same as setClosed, plus sets the m_bBroken to true. /// Such a socket can still be read from so that remaining data from /// the receiver buffer can be read, but no longer sends anything. diff --git a/srtcore/core.cpp b/srtcore/core.cpp index 9e8d6b11e..d13221f1c 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -414,6 +414,7 @@ void srt::CUDT::construct() m_bConnected = false; m_bClosing = false; m_bShutdown = false; + m_bBreaking = false; m_bBroken = false; m_bBreakAsUnstable = false; // TODO: m_iBrokenCounter should be still set to some default. @@ -3868,7 +3869,7 @@ void srt::CUDT::startConnect(const sockaddr_any& serv_addr, int32_t forced_isn) // We can't record this address yet until the cookie-confirmation is done, for safety reasons. sockaddr_any use_source_adr(serv_addr.family()); - while (!m_bClosing) + while (!m_bClosing && !m_bBreaking) { const steady_clock::time_point local_tnow = steady_clock::now(); const steady_clock::duration tdiff = local_tnow - m_tsLastReqTime.load(); diff --git a/srtcore/core.h b/srtcore/core.h index 0e4b4b5d1..81a5142a5 100644 --- a/srtcore/core.h +++ b/srtcore/core.h @@ -972,6 +972,7 @@ class CUDT sync::atomic m_bConnected; // Whether the connection is on or off sync::atomic m_bClosing; // If the UDT entity is closing sync::atomic m_bShutdown; // If the peer side has shutdown the connection + sync::atomic m_bBreaking; // The flag that declares interrupt of the connecting process sync::atomic m_bBroken; // If the connection has been broken sync::atomic m_bBreakAsUnstable; // A flag indicating that the socket should become broken because it has been unstable for too long. sync::atomic m_bPeerHealth; // If the peer status is normal diff --git a/test/test_connection_timeout.cpp b/test/test_connection_timeout.cpp index c8cbc8722..281376ce1 100644 --- a/test/test_connection_timeout.cpp +++ b/test/test_connection_timeout.cpp @@ -14,9 +14,10 @@ typedef int SOCKET; #include"platform_sys.h" #include "srt.h" #include "netinet_any.h" +#include "common.h" using namespace std; - +using namespace srt_logging; class TestConnectionTimeout : public ::srt::Test @@ -172,12 +173,19 @@ TEST_F(TestConnectionTimeout, BlockingLoop) { const sockaddr* psa = reinterpret_cast(&m_sa); const int connection_timeout_ms = 999; + cout << "[ ]\r[" << flush; + for (int i = 0; i < 10; ++i) { const SRTSOCKET client_sock = srt_create_socket(); EXPECT_GT(client_sock, 0); // socket_id should be > 0 if (client_sock <= 0) + { + cout << "!\n"; break; + } + + cout << "." << flush; // Set connection timeout to 999 ms to reduce the test execution time. // Also need to hit a time point between two threads: @@ -198,15 +206,125 @@ TEST_F(TestConnectionTimeout, BlockingLoop) EXPECT_EQ(error_code, SRT_ENOSERVER); if (error_code != SRT_ENOSERVER) { - cerr << "Connection attempt no. " << i << " resulted with: " + cout << "!\nConnection attempt no. " << i << " resulted with: " << error_code << " " << srt_getlasterror_str() << "\n"; break; } EXPECT_EQ(srt_close(client_sock), SRT_SUCCESS); } + cout << endl; +} + +TEST_F(TestConnectionTimeout, BlockingInterrupted) +{ + const SRTSOCKET client_sock = srt_create_socket(); + EXPECT_GT(client_sock, 0); // socket_id should be > 0 + + const int connection_timeout_ms = 10000; + EXPECT_EQ(srt_setsockopt(client_sock, 0, SRTO_CONNTIMEO, &connection_timeout_ms, sizeof connection_timeout_ms), SRT_SUCCESS); + + using namespace std::chrono; + + steady_clock::time_point begin = steady_clock::now(); + + std::thread interrupter ( [client_sock] () { + cout << "[T] START: Waiting 1s\n"; + std::this_thread::sleep_for(seconds(1)); + steady_clock::time_point b = steady_clock::now(); + + cout << "[T] CLOSING @" << client_sock << "\n"; + srt_close(client_sock); + steady_clock::time_point e = steady_clock::now(); + auto passed = duration_cast(e - b); + int close_time_passed_ms = passed.count(); + EXPECT_LT(close_time_passed_ms, 2000); + + cout << "[T] Thread exit\n"; + }); + + const sockaddr* psa = reinterpret_cast(&m_sa); + cout << "START: Connect @" << client_sock << " blind\n"; + EXPECT_EQ(srt_connect(client_sock, psa, sizeof m_sa), SRT_ERROR); + cout << "STOP: Connect\n"; + + steady_clock::time_point end = steady_clock::now(); + auto passed = duration_cast(end - begin); + int time_passed_ms = passed.count(); + cout << "Interrupted after " << time_passed_ms << "ms\n"; + + EXPECT_LT(time_passed_ms, 8000); + + interrupter.join(); } +TEST_F(TestConnectionTimeout, NonblockingInterrupted) +{ + const SRTSOCKET client_sock = srt_create_socket(); + // const SRTSOCKET extra_sock = srt_create_socket(); + EXPECT_GT(client_sock, 0); // socket_id should be > 0 + + const int connection_timeout_ms = 10000; + EXPECT_EQ(srt_setsockopt(client_sock, 0, SRTO_CONNTIMEO, &connection_timeout_ms, sizeof connection_timeout_ms), SRT_SUCCESS); + + const bool non_blocking = false; + EXPECT_EQ(srt_setsockflag(client_sock, SRTO_RCVSYN, &non_blocking, sizeof non_blocking), SRT_SUCCESS); + EXPECT_EQ(srt_setsockflag(client_sock, SRTO_SNDSYN, &non_blocking, sizeof non_blocking), SRT_SUCCESS); + + using namespace std::chrono; + + int eid = srt_epoll_create(); + + int conn_err = SRT_EPOLL_OUT | SRT_EPOLL_ERR; + + srt_epoll_add_usock(eid, client_sock, &conn_err); + // srt_epoll_add_usock(eid, extra_sock, &conn_err); + + steady_clock::time_point begin = steady_clock::now(); + + std::thread interrupter ( [client_sock] () { + cout << "[T] START: Waiting 1s\n"; + std::this_thread::sleep_for(seconds(1)); + steady_clock::time_point b = steady_clock::now(); + + cout << "[T] CLOSING @" << client_sock << "\n"; + srt_close(client_sock); + steady_clock::time_point e = steady_clock::now(); + auto passed = duration_cast(e - b); + int close_time_passed_ms = passed.count(); + EXPECT_LT(close_time_passed_ms, 2000); + + cout << "[T] Thread exit\n"; + }); + + const sockaddr* psa = reinterpret_cast(&m_sa); + cout << "START: Connect @" << client_sock << " blind\n"; + + // Result should be CONNREQ because this is non-blocking connect + EXPECT_EQ(srt_connect(client_sock, psa, sizeof m_sa), 0); + + cout << "EPOLL - wait for connect\n"; + + // Expect uwait to return -1 because all sockets have been + // removed from EID, so the call would block forever. + SRT_EPOLL_EVENT fdset[2]; + EXPECT_EQ(srt_epoll_uwait(eid, fdset, 2, 3000), -1); + SRT_SOCKSTATUS socket_status = srt_getsockstate(client_sock); + + steady_clock::time_point end = steady_clock::now(); + auto passed = duration_cast(end - begin); + int time_passed_ms = passed.count(); + cout << "Interrupted after " << time_passed_ms << "ms\n"; + + EXPECT_LT(time_passed_ms, 8000); + + interrupter.join(); + + cout << "SOCKET STATUS: " << SockStatusStr(socket_status) << endl; + EXPECT_GE(socket_status, SRTS_CLOSED); + + // srt_close(extra_sock); +} TEST(TestConnectionAPI, Accept) { diff --git a/test/test_file_transmission.cpp b/test/test_file_transmission.cpp index bfd668ac7..e7140bf69 100644 --- a/test/test_file_transmission.cpp +++ b/test/test_file_transmission.cpp @@ -50,9 +50,13 @@ TEST(Transmission, FileUpload) sa_lsn.sin_addr.s_addr = INADDR_ANY; sa_lsn.sin_port = htons(5555); + MAKE_UNIQUE_SOCK(sock_lsn_u, "listener", sock_lsn); + MAKE_UNIQUE_SOCK(sock_clr_u, "listener", sock_clr); + // Find unused a port not used by any other service. // Otherwise srt_connect may actually connect. int bind_res = -1; + std::cout << "Looking for a free port... " << std::flush; for (int port = 5000; port <= 5555; ++port) { sa_lsn.sin_port = htons(port); @@ -97,10 +101,13 @@ TEST(Transmission, FileUpload) std::atomic thread_exit { false }; + std::cout << "Running accept [A] thread\n"; + auto client = std::thread([&] { sockaddr_in remote; int len = sizeof remote; + std::cout << "[A] waiting for connection\n"; const SRTSOCKET accepted_sock = srt_accept(sock_lsn, (sockaddr*)&remote, &len); ASSERT_GT(accepted_sock, 0); @@ -115,27 +122,30 @@ TEST(Transmission, FileUpload) std::vector buf(1456); + std::cout << "[A] Connected, reading data...\n"; for (;;) { int n = srt_recv(accepted_sock, buf.data(), 1456); EXPECT_NE(n, SRT_ERROR) << srt_getlasterror_str(); if (n == 0) { - std::cerr << "Received 0 bytes, breaking.\n"; + std::cout << "Received 0 bytes, breaking.\n"; break; } else if (n == -1) { - std::cerr << "READ FAILED, breaking anyway\n"; + std::cout << "READ FAILED, breaking anyway\n"; break; } // Write to file any amount of data received copyfile.write(buf.data(), n); } + std::cout << "[A] Closing socket\n"; EXPECT_NE(srt_close(accepted_sock), SRT_ERROR); + std::cout << "[A] Exit\n"; thread_exit = true; }); @@ -144,6 +154,7 @@ TEST(Transmission, FileUpload) sa.sin_port = sa_lsn.sin_port; ASSERT_EQ(inet_pton(AF_INET, "127.0.0.1", &sa.sin_addr), 1); + std::cout << "Connecting...\n"; srt_connect(sock_clr, (sockaddr*)&sa, sizeof(sa)); std::cout << "Connection initialized" << std::endl; @@ -151,6 +162,7 @@ TEST(Transmission, FileUpload) std::ifstream ifile("file.source", std::ios::in | std::ios::binary); std::vector buf(1456); + std::cout << "Reading file and sending...\n"; for (;;) { size_t n = ifile.read(buf.data(), 1456).gcount(); From f5e6bb98d5ff1532c270e8314ec20a2de13df620 Mon Sep 17 00:00:00 2001 From: Tomoya Fujita Date: Mon, 12 Jan 2026 19:07:32 +0900 Subject: [PATCH 18/19] Old URL (404 - no longer exists) (#3271) Signed-off-by: Tomoya.Fujita --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 4255c0663..16a1d8e08 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@

- SRT + SRT

From 80c871e1fe6604ef0d95556b9e7ba03497daa606 Mon Sep 17 00:00:00 2001 From: Tomoya Fujita Date: Mon, 12 Jan 2026 22:41:37 +0900 Subject: [PATCH 19/19] Detec IPv6 with colon to harden the check if IPv6 is used in the test. (#3264) * Detec IPv6 with colon to harden the check if IPv6 is used in the test. Signed-off-by: Tomoya.Fujita * just relies on srt::CreateAddr to resolve IP family. Signed-off-by: Tomoya Fujita --------- Signed-off-by: Tomoya.Fujita Signed-off-by: Tomoya Fujita --- test/test_reuseaddr.cpp | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/test/test_reuseaddr.cpp b/test/test_reuseaddr.cpp index 14789f897..929f6b3db 100644 --- a/test/test_reuseaddr.cpp +++ b/test/test_reuseaddr.cpp @@ -148,15 +148,6 @@ class ReuseAddr : public srt::Test int yes = 1; int no = 0; - int family = AF_INET; - string famname = "IPv4"; - if (ip.substr(0, 2) == "6.") - { - family = AF_INET6; - ip = ip.substr(2); - famname = "IPv6"; - } - cout << "[T/C] Setting up client socket\n"; ASSERT_NE(client_sock, SRT_INVALID_SOCK); ASSERT_EQ(srt_getsockstate(client_sock), SRTS_INIT); @@ -171,7 +162,8 @@ class ReuseAddr : public srt::Test int epoll_out = SRT_EPOLL_OUT; srt_epoll_add_usock(client_pollid, client_sock, &epoll_out); - sockaddr_any sa = srt::CreateAddr(ip, port, family); + sockaddr_any sa = srt::CreateAddr(ip, port, AF_UNSPEC); + string famname = (sa.family() == AF_INET) ? "IPv4" : "IPv6"; cout << "[T/C] Connecting to: " << sa.str() << " (" << famname << ")" << endl;