From 888fe9a4a2028c31617dfb39f805e41a8733a4e4 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Wed, 19 Aug 2026 22:50:08 +0200 Subject: [PATCH 1/2] fix(peer): join internal threads on shutdown instead of abandoning them (#7) RakPeer teardown raced with its own internal threads because neither of them could be waited on: - Both the update/network thread and each socket's recv polling thread were created detached (PTHREAD_CREATE_DETACHED; the Win32 handle was closed at creation), so Shutdown() could only spin on plain 'volatile bool' flags, which establish no happens-before edge with the threads' writes. - Worse, RNS2_Berkley::BlockOnStopRecvPollingThread() gave up after 1 second if the blocking recvfrom never woke (its only wake-up was a best-effort datagram sent to self). Shutdown then released the socket and DestroyInstance freed the RakPeer while the leaked thread kept dereferencing both (binding.eventHandler->AllocRNS2RecvStruct(), RecvFromBlocking(this)) -- a use-after-free that corrupted the heap and crashed at varying points later in teardown. The destroy/recreate churn in ManyClientsOneServerDeallocateBlocking made hitting that 1s abandonment likely under CI load. The fix makes teardown deterministic: - RakThread gains CreateJoinable()/Join(); the update thread and the recv polling thread are now joinable and joined before any state they use is torn down. The join also provides the memory-visibility edge the flag spin never did. - SO_RCVTIMEO (500ms) bounds the recv thread's blocking recvfrom/recvmmsg so it re-checks endThreads even if the wake-up datagram is lost; the 1-second abandonment deadline is gone. - The teardown handshake flags (RakPeer::endThreads, isMainLoopThreadActive, RNS2_Berkley::endThreads) are std::atomic instead of volatile bool. Adjacent data races surfaced by TSan on the same churn path, fixed while here: GetTimeUS_Linux's lazy initialTime init (magic static now), ThreadsafeAllocatingQueue::PopInaccurate's unlocked emptiness probe, RunUpdateCycle's unlocked requestedConnectionQueue.IsEmpty() check, and LocklessUint32_t (now std::atomic; GetValue() was an unsynchronized read and the __sync_fetch_and_add branch returned the pre-change value, unlike every other platform). ManyClientsOneServerDeallocateBlocking is un-quarantined: it runs under CI again. New coverage: RakThreadTests (unit) for the joinable API, and PeerTeardownTests (integration) churning Startup/Connect/Shutdown/ DestroyInstance with live connections, including DestroyInstance without a prior Shutdown. Verified on Linux (Docker, ubuntu:24.04) in Debug and Release, full ctest suite, plus repeated runs of the teardown tests in Release; on macOS with ASan+UBSan (full integration suite, and the new churn test x10) and TSan (teardown-lifecycle races gone; remaining reports are the engine's long-standing by-design unsynchronized remoteSystemList reads). Fixes #7 --- .../mafianet/DS_ThreadsafeAllocatingQueue.h | 7 +- Source/include/mafianet/LocklessTypes.h | 19 +-- Source/include/mafianet/peer.h | 17 ++- Source/include/mafianet/socket2.h | 10 +- Source/include/mafianet/thread.h | 28 ++++ Source/src/GetTime.cpp | 22 ++- Source/src/LocklessTypes.cpp | 35 ++--- Source/src/RakNetSocket2.cpp | 38 ++++- Source/src/RakNetSocket2_Berkley.cpp | 3 +- Source/src/RakPeer.cpp | 24 ++- Source/src/RakThread.cpp | 47 ++++++ ...lientsOneServerDeallocateBlockingTests.cpp | 18 +-- Tests/Integration/PeerTeardownTests.cpp | 139 ++++++++++++++++++ Tests/Unit/RakThreadTests.cpp | 66 +++++++++ 14 files changed, 397 insertions(+), 76 deletions(-) create mode 100644 Tests/Integration/PeerTeardownTests.cpp create mode 100644 Tests/Unit/RakThreadTests.cpp diff --git a/Source/include/mafianet/DS_ThreadsafeAllocatingQueue.h b/Source/include/mafianet/DS_ThreadsafeAllocatingQueue.h index 7247f43bf..fc92c4a30 100644 --- a/Source/include/mafianet/DS_ThreadsafeAllocatingQueue.h +++ b/Source/include/mafianet/DS_ThreadsafeAllocatingQueue.h @@ -70,15 +70,16 @@ void ThreadsafeAllocatingQueue::Push(structureType *s) template structureType *ThreadsafeAllocatingQueue::PopInaccurate(void) { + // Historically this probed queue.IsEmpty() without the mutex as a fast path + // ("inaccurate"); that unlocked read raced with Push from other threads. An + // uncontended lock is cheap enough that the fast path isn't worth the UB. structureType *s; - if (queue.IsEmpty()) - return 0; queueMutex.Lock(); if (queue.IsEmpty()==false) s=queue.Pop(); else s=0; - queueMutex.Unlock(); + queueMutex.Unlock(); return s; } diff --git a/Source/include/mafianet/LocklessTypes.h b/Source/include/mafianet/LocklessTypes.h index 89249ef73..9e1a4ce3a 100644 --- a/Source/include/mafianet/LocklessTypes.h +++ b/Source/include/mafianet/LocklessTypes.h @@ -18,11 +18,10 @@ #include "Export.h" #include "NativeTypes.h" +// Kept for transitive users even though the atomics below no longer need it. #include "WindowsIncludes.h" -#if defined(ANDROID) || defined(__S3E__) || defined(__APPLE__) -// __sync_fetch_and_add not supported apparently -#include "SimpleMutex.h" -#endif + +#include namespace MafiaNet { @@ -36,18 +35,10 @@ class RAK_DLL_EXPORT LocklessUint32_t uint32_t Increment(void); // Returns variable value after changing it uint32_t Decrement(void); - uint32_t GetValue(void) const {return value;} + uint32_t GetValue(void) const {return value.load();} protected: -#ifdef _WIN32 - volatile LONG value; -#elif defined(ANDROID) || defined(__S3E__) || defined(__APPLE__) - // __sync_fetch_and_add not supported apparently - SimpleMutex mutex; - uint32_t value; -#else - volatile uint32_t value; -#endif + std::atomic value; }; } diff --git a/Source/include/mafianet/peer.h b/Source/include/mafianet/peer.h index fbc41666f..9ac4e0f10 100644 --- a/Source/include/mafianet/peer.h +++ b/Source/include/mafianet/peer.h @@ -42,6 +42,8 @@ #include "LocklessTypes.h" #include "DS_Queue.h" +#include + namespace MafiaNet { /// Forward declarations class HuffmanEncodingTree; @@ -761,11 +763,16 @@ class RAK_DLL_EXPORT RakPeer : public RakPeerInterface, public RNS2EventHandler bool IsLoopbackAddress(const AddressOrGUID &systemIdentifier, bool matchPort) const; SystemAddress GetLoopbackAddress(void) const; - ///Set this to true to terminate the Peer thread execution - volatile bool endThreads; - ///true if the peer thread is active. - volatile bool isMainLoopThreadActive; - + ///Set this to true to terminate the Peer thread execution + std::atomic endThreads; + ///true if the peer thread is active. + std::atomic isMainLoopThreadActive; + /// Joinable handle for the update/network thread. Shutdown() joins it so the + /// thread has fully exited -- with all its writes visible -- before any + /// connection state is torn down (issue #7). + RakThread::ThreadHandle updateThread; + bool updateThreadJoinable; + // MafiaNet::LocklessUint32_t isRecvFromLoopThreadActive; diff --git a/Source/include/mafianet/socket2.h b/Source/include/mafianet/socket2.h index 9e3054b4c..83b77008a 100644 --- a/Source/include/mafianet/socket2.h +++ b/Source/include/mafianet/socket2.h @@ -23,6 +23,8 @@ #include "DS_ThreadsafeAllocatingQueue.h" #include "Export.h" +#include + // Batched datagram I/O (recvmmsg / sendmmsg) is guarded by a plain // `#if defined(__linux__)` wherever it appears. There is no macro and no build // option for it: the syscalls exist on Linux and nowhere else MafiaNet targets, @@ -240,7 +242,13 @@ class RNS2_Berkley : public IRNS2_Berkley // same guard, and only defined on that platform. void RecvFromBatchedLoop(void); MafiaNet::LocklessUint32_t isRecvFromLoopThreadActive; - volatile bool endThreads; + std::atomic endThreads; + // The recv polling thread is joinable so teardown can wait for it to fully + // exit before the socket (and the RakPeer it calls back into) are freed. + // A detached thread with a bounded wait allowed a leaked thread to keep + // dereferencing both after Shutdown (issue #7). + RakThread::ThreadHandle recvThread; + bool recvThreadJoinable; // Constructor not called! #if defined(__APPLE__) diff --git a/Source/include/mafianet/thread.h b/Source/include/mafianet/thread.h index af0a3423f..5ccd5e818 100644 --- a/Source/include/mafianet/thread.h +++ b/Source/include/mafianet/thread.h @@ -18,6 +18,10 @@ #include "Export.h" +#if !defined(_WIN32) +#include +#endif + namespace MafiaNet { /// To define a thread, use RAK_THREAD_DECLARATION(functionName); @@ -50,6 +54,30 @@ class RAK_DLL_EXPORT RakThread #else static int Create( void* start_address( void* ), void *arglist, int priority=0); #endif + + /// Handle to a joinable thread created with CreateJoinable(). Must be + /// reaped with Join() exactly once, or the thread's resources leak. +#if defined(_WIN32) + typedef void *ThreadHandle; // HANDLE +#else + typedef pthread_t ThreadHandle; +#endif + + /// Like Create(), but the thread is joinable: the caller receives a handle + /// and MUST call Join() on it. Join() blocks until the thread function has + /// returned and establishes a happens-before edge with all of the thread's + /// writes -- use this for threads whose owner frees state the thread uses. + /// \param[out] handle Receives the thread handle on success; unchanged on failure. + /// \return 0=success. >0 = error code +#if defined(_WIN32) + static int CreateJoinable( unsigned __stdcall start_address( void* ), void *arglist, ThreadHandle *handle, int priority=0); +#else + static int CreateJoinable( void* start_address( void* ), void *arglist, ThreadHandle *handle, int priority=0); +#endif + + /// Block until the thread behind \a handle has fully exited, then release + /// the handle. Call exactly once per CreateJoinable(). + static void Join( ThreadHandle handle ); }; } diff --git a/Source/src/GetTime.cpp b/Source/src/GetTime.cpp index 772a733c6..349533f7e 100644 --- a/Source/src/GetTime.cpp +++ b/Source/src/GetTime.cpp @@ -46,10 +46,11 @@ #else #include #include -MafiaNet::TimeUS initialTime; #endif +#if defined(_WIN32) static bool initialized=false; +#endif #if defined(GET_TIME_SPIKE_LIMIT) && GET_TIME_SPIKE_LIMIT>0 #include "mafianet/SimpleMutex.h" @@ -177,16 +178,21 @@ MafiaNet::TimeUS GetTimeUS_Windows( void ) #endif // #if defined(GET_TIME_SPIKE_LIMIT) && GET_TIME_SPIKE_LIMIT>0 } #elif defined(__GNUC__) || defined(__GCCXML__) || defined(__S3E__) +static MafiaNet::TimeUS GetInitialTime_Linux( void ) +{ + timeval tp; + gettimeofday( &tp, 0 ); + return ( tp.tv_sec ) * (MafiaNet::TimeUS) 1000000 + ( tp.tv_usec ); +} + MafiaNet::TimeUS GetTimeUS_Linux( void ) { timeval tp; - if ( initialized == false) - { - gettimeofday( &tp, 0 ); - initialized=true; - // I do this because otherwise MafiaNet::Time in milliseconds won't work as it will underflow when dividing by 1000 to do the conversion - initialTime = ( tp.tv_sec ) * (MafiaNet::TimeUS) 1000000 + ( tp.tv_usec ); - } + // Thread-safe first-use initialization (C++11 magic static). Every RakPeer + // startup spawns threads that call this concurrently; a plain lazy-init + // bool/global raced, letting a thread read a torn or stale base time. + // I subtract an initial time because otherwise MafiaNet::Time in milliseconds won't work as it will underflow when dividing by 1000 to do the conversion + static const MafiaNet::TimeUS initialTime = GetInitialTime_Linux(); // GCC MafiaNet::TimeUS curTime; diff --git a/Source/src/LocklessTypes.cpp b/Source/src/LocklessTypes.cpp index 90db56dc2..999151075 100644 --- a/Source/src/LocklessTypes.cpp +++ b/Source/src/LocklessTypes.cpp @@ -3,7 +3,7 @@ * All rights reserved. * * This source code is licensed under the BSD-style license found in the - * RakNet License.txt file in the licenses directory of this source tree. An additional grant + * RakNet License.txt file in the licenses directory of this source tree. An additional grant * of patent rights can be found in the RakNet Patents.txt file in the same directory. * * @@ -17,6 +17,13 @@ using namespace MafiaNet; +// std::atomic replaces the previous per-platform mix of InterlockedIncrement, +// mutex-guarded arithmetic, and __sync_fetch_and_add. Besides being standard, +// this fixes two defects of the old code: GetValue() read the counter without +// any synchronization (a data race with concurrent modifications), and the +// __sync_fetch_and_add branch returned the value from *before* the change +// while every other platform returned the value after it. + LocklessUint32_t::LocklessUint32_t() { value=0; @@ -27,31 +34,9 @@ LocklessUint32_t::LocklessUint32_t(uint32_t initial) } uint32_t LocklessUint32_t::Increment(void) { -#ifdef _WIN32 - return (uint32_t) InterlockedIncrement(&value); -#elif defined(ANDROID) || defined(__S3E__) || defined(__APPLE__) - uint32_t v; - mutex.Lock(); - ++value; - v=value; - mutex.Unlock(); - return v; -#else - return __sync_fetch_and_add (&value, (uint32_t) 1); -#endif + return value.fetch_add(1)+1; } uint32_t LocklessUint32_t::Decrement(void) { -#ifdef _WIN32 - return (uint32_t) InterlockedDecrement(&value); -#elif defined(ANDROID) || defined(__S3E__) || defined(__APPLE__) - uint32_t v; - mutex.Lock(); - --value; - v=value; - mutex.Unlock(); - return v; -#else - return __sync_fetch_and_add (&value, (uint32_t) -1); -#endif + return value.fetch_sub(1)-1; } diff --git a/Source/src/RakNetSocket2.cpp b/Source/src/RakNetSocket2.cpp index 748cde5cb..ebcec576c 100644 --- a/Source/src/RakNetSocket2.cpp +++ b/Source/src/RakNetSocket2.cpp @@ -212,6 +212,8 @@ unsigned RNS2_Berkley::RecvFromLoopInt(void) RNS2_Berkley::RNS2_Berkley() { rns2Socket=(RNS2Socket)INVALID_SOCKET; + endThreads=false; + recvThreadJoinable=false; } RNS2_Berkley::~RNS2_Berkley() { @@ -231,7 +233,25 @@ int RNS2_Berkley::CreateRecvPollingThread(int threadPriority) { endThreads=false; - int errorCode = MafiaNet::RakThread::Create(RecvFromLoop, this, threadPriority); + // Bound the time a blocking recvfrom/recvmmsg can sit in the kernel so the + // polling thread re-checks endThreads even if the wake-up datagram sent by + // BlockOnStopRecvPollingThread is lost. Both recv loops already treat a + // zero/negative return (EAGAIN/WSAETIMEDOUT) as "no data" and loop. +#if defined(_WIN32) + DWORD recvTimeout = 500; // milliseconds + setsockopt__(rns2Socket, SOL_SOCKET, SO_RCVTIMEO, (char *) &recvTimeout, sizeof(recvTimeout)); +#else + timeval recvTimeout; + recvTimeout.tv_sec = 0; + recvTimeout.tv_usec = 500000; + setsockopt__(rns2Socket, SOL_SOCKET, SO_RCVTIMEO, (char *) &recvTimeout, sizeof(recvTimeout)); +#endif + + // Joinable, not detached: BlockOnStopRecvPollingThread must be able to wait + // for the thread to fully exit before the socket and its event handler are + // freed (issue #7). + int errorCode = MafiaNet::RakThread::CreateJoinable(RecvFromLoop, this, &recvThread, threadPriority); + recvThreadJoinable = (errorCode==0); return errorCode; } void RNS2_Berkley::SignalStopRecvPollingThread(void) @@ -242,7 +262,11 @@ void RNS2_Berkley::BlockOnStopRecvPollingThread(void) { endThreads=true; - // Get recvfrom to unblock + if (recvThreadJoinable==false) + return; + + // Get recvfrom to unblock promptly (SO_RCVTIMEO bounds the wait even if + // this datagram is lost) RNS2_SendParameters bsp; unsigned long zero=0; bsp.data=(char*) &zero; @@ -251,13 +275,19 @@ void RNS2_Berkley::BlockOnStopRecvPollingThread(void) bsp.ttl=0; Send(&bsp, _FILE_AND_LINE_); - MafiaNet::TimeMS timeout = MafiaNet::GetTimeMS()+1000; - while ( isRecvFromLoopThreadActive.GetValue()>0 && MafiaNet::GetTimeMS()0 ) { // Get recvfrom to unblock Send(&bsp, _FILE_AND_LINE_); RakSleep(30); } + + // Never abandon the thread on a deadline: the caller frees this socket and + // the RakPeer the thread calls back into right after we return, so leaving + // the thread running was a use-after-free (issue #7). Join also gives the + // happens-before edge that makes the thread's writes visible. + MafiaNet::RakThread::Join(recvThread); + recvThreadJoinable=false; } const RNS2_BerkleyBindParameters *RNS2_Berkley::GetBindings(void) const {return &binding;} RNS2Socket RNS2_Berkley::GetSocket(void) const {return rns2Socket;} diff --git a/Source/src/RakNetSocket2_Berkley.cpp b/Source/src/RakNetSocket2_Berkley.cpp index ff3217ebc..3bbeedd66 100644 --- a/Source/src/RakNetSocket2_Berkley.cpp +++ b/Source/src/RakNetSocket2_Berkley.cpp @@ -452,7 +452,8 @@ void RNS2_Berkley::RecvFromBlockingIPV4And6(RNS2RecvStruct *recvFromStruct) DWORD dwIOError = GetLastError(); // 10035 = WSAEWOULDBLOCK (expected for non-blocking sockets) // 10054 = WSAECONNRESET (expected for UDP - prior sendto received ICMP port unreachable) - if (dwIOError != 10035 && dwIOError != 10054) + // 10060 = WSAETIMEDOUT (expected: SO_RCVTIMEO is set so the recv polling thread can re-check shutdown flags) + if (dwIOError != 10035 && dwIOError != 10054 && dwIOError != 10060) { LPVOID messageBuffer; FormatMessage( FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, diff --git a/Source/src/RakPeer.cpp b/Source/src/RakPeer.cpp index a2e5f6302..cd26d46e1 100644 --- a/Source/src/RakPeer.cpp +++ b/Source/src/RakPeer.cpp @@ -215,6 +215,7 @@ RakPeer::RakPeer() bytesSentPerSecond = bytesReceivedPerSecond = 0; endThreads = true; isMainLoopThreadActive = false; + updateThreadJoinable = false; incomingDatagramEventHandler=0; @@ -663,8 +664,10 @@ StartupResult RakPeer::Startup( unsigned int maxConnections, SocketDescriptor *s - errorCode = MafiaNet::RakThread::Create(UpdateNetworkLoop, this, threadPriority); - + // Joinable so Shutdown() can wait for the thread to fully exit + // before tearing down the connection state it uses (issue #7). + errorCode = MafiaNet::RakThread::CreateJoinable(UpdateNetworkLoop, this, &updateThread, threadPriority); + updateThreadJoinable = (errorCode==0); if ( errorCode != 0 ) { @@ -1111,6 +1114,16 @@ void RakPeer::Shutdown( unsigned int blockDuration, unsigned char orderingChanne } */ + // Join the update/network thread rather than spin-waiting on a flag: the + // join both guarantees the thread has fully exited and establishes the + // happens-before edge that makes its writes visible before the connection + // state below is torn down (issue #7). + if ( updateThreadJoinable ) + { + MafiaNet::RakThread::Join(updateThread); + updateThreadJoinable = false; + } + // Fallback for configurations where no joinable thread was created. while ( isMainLoopThreadActive ) { RakSleep(15); @@ -5722,7 +5735,12 @@ bool RakPeer::RunUpdateCycle(BitStream &updateBitStream ) bufferedCommands.Deallocate(bcs, _FILE_AND_LINE_); } - if (requestedConnectionQueue.IsEmpty()==false) + // The queue is filled from the user thread (Connect/SendConnectionRequest), + // so even the emptiness probe must hold the mutex. + requestedConnectionQueueMutex.Lock(); + const bool requestedConnectionQueueHasEntries = requestedConnectionQueue.IsEmpty()==false; + requestedConnectionQueueMutex.Unlock(); + if (requestedConnectionQueueHasEntries) { if (timeNS==0) { diff --git a/Source/src/RakThread.cpp b/Source/src/RakThread.cpp index 77157e680..58c3614ec 100644 --- a/Source/src/RakThread.cpp +++ b/Source/src/RakThread.cpp @@ -116,6 +116,53 @@ int RakThread::Create( void* start_address( void* ), void *arglist, int priority #endif } +#if defined(_WIN32) +int RakThread::CreateJoinable( unsigned __stdcall start_address( void* ), void *arglist, ThreadHandle *handle, int priority) +#else +int RakThread::CreateJoinable( void* start_address( void* ), void *arglist, ThreadHandle *handle, int priority) +#endif +{ +#ifdef _WIN32 + HANDLE threadHandle; + unsigned threadID = 0; + + threadHandle = (HANDLE) _beginthreadex(nullptr, MAX_ALLOCA_STACK_ALLOCATION*2, start_address, arglist, 0, &threadID ); + + if (threadHandle==0) + { + return 1; + } + + SetThreadPriority(threadHandle, priority); + *handle = threadHandle; + return 0; +#else + pthread_t threadHandle; + pthread_attr_t attr; + sched_param param; + param.sched_priority=priority; + pthread_attr_init( &attr ); + pthread_attr_setschedparam(&attr, ¶m); + pthread_attr_setstacksize(&attr, MAX_ALLOCA_STACK_ALLOCATION*2); + pthread_attr_setdetachstate( &attr, PTHREAD_CREATE_JOINABLE ); + int res = pthread_create( &threadHandle, &attr, start_address, arglist ); + RakAssert(res==0 && "pthread_create in RakThread.cpp failed.") + if (res==0) + *handle = threadHandle; + return res; +#endif +} + +void RakThread::Join( ThreadHandle handle ) +{ +#ifdef _WIN32 + WaitForSingleObject( (HANDLE) handle, INFINITE ); + CloseHandle( (HANDLE) handle ); +#else + pthread_join( handle, nullptr ); +#endif +} + diff --git a/Tests/Integration/ManyClientsOneServerDeallocateBlockingTests.cpp b/Tests/Integration/ManyClientsOneServerDeallocateBlockingTests.cpp index ccd12d15a..8096750c0 100644 --- a/Tests/Integration/ManyClientsOneServerDeallocateBlockingTests.cpp +++ b/Tests/Integration/ManyClientsOneServerDeallocateBlockingTests.cpp @@ -161,18 +161,12 @@ IsConnected */ TEST_F(ManyClientsOneServerDeallocateBlocking, ClientsReconnectAfterDeallocateAndTimeout) { - // QUARANTINED under CI: this stress test destroys and recreates client peers - // mid-flight (see DestroyInstance/GetInstance below) while their connections and - // network threads are still live, exposing a pre-existing multithreaded teardown - // race in RakPeer. It crashes intermittently in the full suite (SIGSEGV in - // release -> exit 139; RakAssert/SIGBUS under ASan), but passes in isolation and - // under a debugger (timing masks the race). Skipped in CI so unrelated PRs aren't - // blocked; still runs locally for debugging. Tracked in - // https://github.com/MafiaHub/MafiaNet/issues/7 - if (getenv("CI") != nullptr) - { - GTEST_SKIP() << "QUARANTINED in CI: skipping flaky teardown race (see issue #7)"; - } + // This stress test destroys and recreates client peers mid-flight (see + // DestroyInstance/GetInstance below) while their connections and network + // threads are still live. It used to be quarantined under CI because RakPeer + // teardown could abandon its recv polling thread and free state under it + // (https://github.com/MafiaHub/MafiaNet/issues/7); teardown now joins both + // internal threads, so the test runs everywhere again. const int testDurationMs = 30000; const int sleepTimeMs = 2000; diff --git a/Tests/Integration/PeerTeardownTests.cpp b/Tests/Integration/PeerTeardownTests.cpp new file mode 100644 index 000000000..249575cf2 --- /dev/null +++ b/Tests/Integration/PeerTeardownTests.cpp @@ -0,0 +1,139 @@ +/* + * Copyright (c) 2026, MafiaHub + * + * This source code is licensed under the MIT-style license found in the + * license.txt file in the root directory of this source tree. + */ + +// Regression coverage for https://github.com/MafiaHub/MafiaNet/issues/7: +// destroying a RakPeer while its connections and internal threads are live +// must not leave a leaked network/recv thread touching freed memory. Each +// scenario below churns the Startup/Connect/Shutdown/DestroyInstance cycle +// that exposed the teardown race; correctness is asserted deterministically, +// and the memory-safety aspect is what ASan/TSan CI runs of this suite verify. + +#include + +#include "mafianet/peerinterface.h" +#include "mafianet/peer.h" +#include "mafianet/sleep.h" +#include "mafianet/GetTime.h" + +using namespace MafiaNet; + +class PeerTeardown : public ::testing::Test +{ +protected: + void SetUp() override + { + // Destroyed clients disappear silently (no disconnect notification), so + // their server-side slots linger as zombies until the server's timeout + // reaps them. Give the server enough headroom that every churn round can + // connect fresh clients while earlier rounds' zombies are still pending. + server = RakPeerInterface::GetInstance(); + SocketDescriptor sd(0, "127.0.0.1"); + ASSERT_EQ(server->Startup(kServerCapacity, &sd, 1), RAKNET_STARTED); + server->SetMaximumIncomingConnections(kServerCapacity); + serverPort = server->GetInternalID().GetPort(); + } + + void TearDown() override + { + if (server) + { + server->Shutdown(100); + RakPeerInterface::DestroyInstance(server); + } + } + + // Pump a peer's receive queue so its user-thread bookkeeping advances. + static void Pump(RakPeerInterface *peer) + { + Packet *packet; + while ((packet = peer->Receive()) != nullptr) + peer->DeallocatePacket(packet); + } + + bool WaitForConnection(RakPeerInterface *client, TimeMS timeoutMs) + { + SystemAddress serverAddress("127.0.0.1", serverPort); + TimeMS start = GetTimeMS(); + while (GetTimeMS() - start < timeoutMs) + { + Pump(client); + Pump(server); + if (client->GetConnectionState(serverAddress) == IS_CONNECTED) + return true; + RakSleep(10); + } + return false; + } + + static const int kMaxClients = 8; + static const int kServerCapacity = 64; + RakPeerInterface *server = nullptr; + unsigned short serverPort = 0; +}; + +// A single client repeatedly connects and is torn down mid-connection. Every +// cycle must start up successfully (a leaked recv thread from the previous +// cycle would hold the port / corrupt the allocator) and reconnect. +TEST_F(PeerTeardown, DestroyWithLiveConnectionThenRecreateRepeatedly) +{ + const int kCycles = 12; + for (int cycle = 0; cycle < kCycles; cycle++) + { + RakPeerInterface *client = RakPeerInterface::GetInstance(); + SocketDescriptor clientSd(0, "127.0.0.1"); + ASSERT_EQ(client->Startup(1, &clientSd, 1), RAKNET_STARTED) + << "cycle " << cycle << ": client failed to start"; + + ASSERT_EQ(client->Connect("127.0.0.1", serverPort, nullptr, 0), CONNECTION_ATTEMPT_STARTED) + << "cycle " << cycle; + ASSERT_TRUE(WaitForConnection(client, 5000)) + << "cycle " << cycle << ": client never connected"; + + // Tear the peer down while the connection is fully live. Alternate + // between a graceful window and an immediate teardown so both paths + // (flush-and-close and drop-everything) run under sanitizers. + client->Shutdown(cycle % 2 == 0 ? 100 : 0); + EXPECT_FALSE(client->IsActive()) << "cycle " << cycle; + RakPeerInterface::DestroyInstance(client); + } + + // The server must survive all of that with its own threads intact: it can + // still accept a fresh connection afterwards. + RakPeerInterface *client = RakPeerInterface::GetInstance(); + SocketDescriptor clientSd(0, "127.0.0.1"); + ASSERT_EQ(client->Startup(1, &clientSd, 1), RAKNET_STARTED); + ASSERT_EQ(client->Connect("127.0.0.1", serverPort, nullptr, 0), CONNECTION_ATTEMPT_STARTED); + EXPECT_TRUE(WaitForConnection(client, 5000)) << "server no longer accepts connections after churn"; + client->Shutdown(100); + RakPeerInterface::DestroyInstance(client); +} + +// Several clients are destroyed at once while all their connections are live, +// then immediately recreated -- the pattern from +// ManyClientsOneServerDeallocateBlockingTests that exposed the race. +TEST_F(PeerTeardown, DestroyManyClientsSimultaneouslyWhileConnected) +{ + RakPeerInterface *clients[kMaxClients]; + + for (int round = 0; round < 3; round++) + { + for (int i = 0; i < kMaxClients; i++) + { + clients[i] = RakPeerInterface::GetInstance(); + SocketDescriptor sd(0, "127.0.0.1"); + ASSERT_EQ(clients[i]->Startup(1, &sd, 1), RAKNET_STARTED) << "round " << round << " client " << i; + ASSERT_EQ(clients[i]->Connect("127.0.0.1", serverPort, nullptr, 0), CONNECTION_ATTEMPT_STARTED); + } + for (int i = 0; i < kMaxClients; i++) + ASSERT_TRUE(WaitForConnection(clients[i], 5000)) << "round " << round << " client " << i; + + // No Shutdown() call first: DestroyInstance itself must cope with live + // connections and running threads. + for (int i = 0; i < kMaxClients; i++) + RakPeerInterface::DestroyInstance(clients[i]); + } +} diff --git a/Tests/Unit/RakThreadTests.cpp b/Tests/Unit/RakThreadTests.cpp new file mode 100644 index 000000000..469388028 --- /dev/null +++ b/Tests/Unit/RakThreadTests.cpp @@ -0,0 +1,66 @@ +/* + * Copyright (c) 2026, MafiaHub + * + * This source code is licensed under the MIT-style license found in the + * license.txt file in the root directory of this source tree. + */ + +#include + +#include "mafianet/thread.h" + +#include + +using namespace MafiaNet; + +namespace +{ + +RAK_THREAD_DECLARATION(IncrementCounterThread) +{ + std::atomic *counter = (std::atomic *) arguments; + counter->fetch_add(1); + return 0; +} + +RAK_THREAD_DECLARATION(IncrementCounterManyTimesThread) +{ + std::atomic *counter = (std::atomic *) arguments; + for (int i = 0; i < 1000; i++) + counter->fetch_add(1); + return 0; +} + +} // namespace + +TEST(RakThread, JoinWaitsForThreadCompletion) +{ + std::atomic counter(0); + + RakThread::ThreadHandle handle; + int errorCode = RakThread::CreateJoinable(IncrementCounterManyTimesThread, &counter, &handle); + ASSERT_EQ(errorCode, 0) << "CreateJoinable failed"; + + RakThread::Join(handle); + + // Join must not return before the thread function has fully completed, and + // it must establish a happens-before edge making the thread's writes visible. + EXPECT_EQ(counter.load(), 1000); +} + +TEST(RakThread, JoinReapsMultipleThreadsIndependently) +{ + std::atomic counterA(0); + std::atomic counterB(0); + + RakThread::ThreadHandle handleA; + RakThread::ThreadHandle handleB; + ASSERT_EQ(RakThread::CreateJoinable(IncrementCounterThread, &counterA, &handleA), 0); + ASSERT_EQ(RakThread::CreateJoinable(IncrementCounterThread, &counterB, &handleB), 0); + + RakThread::Join(handleB); + EXPECT_EQ(counterB.load(), 1); + + RakThread::Join(handleA); + EXPECT_EQ(counterA.load(), 1); +} From 705fbff9afcc9edda575034fd3afec313d395502 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Sat, 22 Aug 2026 20:21:40 +0200 Subject: [PATCH 2/2] fix(peer): address review findings on teardown PR - RunUpdateCycle now holds requestedConnectionQueueMutex for the whole connection-request pass. It used to unlock after fetching the entry and keep dereferencing it while CancelConnectionAttempt (user thread) could delete it under that same mutex -- a use-after-free; the delete branch also freed the entry before unlinking it, leaving a dangling pointer visible to other threads. Entries are now unlinked and deleted under the held lock. The only OnDirectSocketSend implementers (PacketLogger, StatisticsHistory) don't re-enter connection APIs, so holding the lock across the send cannot deadlock. - GetTimeUS_Windows: removed the racy first-call `initialized` guard; its body was entirely commented out, so it only wrote a non-atomic flag from every calling thread. - RakThreadTests: join thread A before the fatal assertion if creating thread B fails, so a failed create can't leave a thread referencing the dead stack frame. - PeerTeardownTests: clients are fixture-tracked and destroyed in TearDown() so cleanup survives a failed ASSERT_; WaitForConnection now requires BOTH peers to report IS_CONNECTED before returning. - New Tests/Unit/ConcurrencyPrimitivesTests.cpp: LocklessUint32_t return-after-change semantics and lost-update check under concurrent increments/decrements, ThreadsafeAllocatingQueue concurrent push/PopInaccurate with no lost or duplicated entries, and concurrent GetTimeUS calls sharing one time base. All join-based, no wall-clock deadlines; TSan-clean. --- Source/src/GetTime.cpp | 21 +-- Source/src/RakPeer.cpp | 22 +-- Tests/Integration/PeerTeardownTests.cpp | 68 ++++++-- Tests/Unit/ConcurrencyPrimitivesTests.cpp | 182 ++++++++++++++++++++++ Tests/Unit/RakThreadTests.cpp | 9 +- 5 files changed, 255 insertions(+), 47 deletions(-) create mode 100644 Tests/Unit/ConcurrencyPrimitivesTests.cpp diff --git a/Source/src/GetTime.cpp b/Source/src/GetTime.cpp index 349533f7e..48f38039d 100644 --- a/Source/src/GetTime.cpp +++ b/Source/src/GetTime.cpp @@ -48,9 +48,6 @@ #include #endif -#if defined(_WIN32) -static bool initialized=false; -#endif #if defined(GET_TIME_SPIKE_LIMIT) && GET_TIME_SPIKE_LIMIT>0 #include "mafianet/SimpleMutex.h" @@ -142,21 +139,9 @@ MafiaNet::TimeMS MafiaNet::GetTimeMS( void ) #if defined(_WIN32) MafiaNet::TimeUS GetTimeUS_Windows( void ) { - if ( initialized == false) - { - initialized = true; - - // Save the current process -// HANDLE mProc = GetCurrentProcess(); - - // Get the current Affinity -#if defined (_M_X64) -// GetProcessAffinityMask(mProc, (PDWORD_PTR)&mProcMask, (PDWORD_PTR)&mSysMask); -#else -// GetProcessAffinityMask(mProc, &mProcMask, &mSysMask); -#endif -// mThread = GetCurrentThread(); - } + // A first-call `initialized` guard used to live here; its body was entirely + // commented out, so all it did was write a non-atomic flag from every + // calling thread (a data race). Removed. // 9/26/2010 In China running LuDaShi, QueryPerformanceFrequency has to be called every time because CPU clock speeds can be different MafiaNet::TimeUS curTime; diff --git a/Source/src/RakPeer.cpp b/Source/src/RakPeer.cpp index cd26d46e1..2e956aeae 100644 --- a/Source/src/RakPeer.cpp +++ b/Source/src/RakPeer.cpp @@ -5750,12 +5750,16 @@ bool RakPeer::RunUpdateCycle(BitStream &updateBitStream ) bool condition1, condition2; unsigned requestedConnectionQueueIndex=0; + // Hold the mutex for the whole pass: CancelConnectionAttempt (user + // thread) deletes entries under this mutex, so dropping it while still + // dereferencing rcs was a use-after-free. The only OnDirectSocketSend + // implementers (PacketLogger, StatisticsHistory) don't call back into + // connection APIs, so the callbacks below cannot re-enter this lock. requestedConnectionQueueMutex.Lock(); while (requestedConnectionQueueIndex < requestedConnectionQueue.Size()) { RequestedConnectionStruct *rcs; rcs = requestedConnectionQueue[requestedConnectionQueueIndex]; - requestedConnectionQueueMutex.Unlock(); if (rcs->nextRequestTime < timeMS) { condition1=rcs->requestsMade==rcs->sendConnectionAttemptCount+1; @@ -5783,18 +5787,10 @@ bool RakPeer::RunUpdateCycle(BitStream &updateBitStream ) CAT_AUDIT_PRINTF("AUDIT: Connection attempt FAILED so deleting rcs->client_handshake object %x\n", rcs->client_handshake); MafiaNet::OP_DELETE(rcs->client_handshake,_FILE_AND_LINE_); #endif + // Unlink before deleting (both under the held mutex) so the + // queue never holds a dangling pointer. + requestedConnectionQueue.RemoveAtIndex(requestedConnectionQueueIndex); MafiaNet::OP_DELETE(rcs,_FILE_AND_LINE_); - - requestedConnectionQueueMutex.Lock(); - for (unsigned int k=0; k < requestedConnectionQueue.Size(); k++) - { - if (requestedConnectionQueue[k]==rcs) - { - requestedConnectionQueue.RemoveAtIndex(k); - break; - } - } - requestedConnectionQueueMutex.Unlock(); } else { @@ -5874,8 +5870,6 @@ bool RakPeer::RunUpdateCycle(BitStream &updateBitStream ) } else requestedConnectionQueueIndex++; - - requestedConnectionQueueMutex.Lock(); } requestedConnectionQueueMutex.Unlock(); } diff --git a/Tests/Integration/PeerTeardownTests.cpp b/Tests/Integration/PeerTeardownTests.cpp index 249575cf2..9feee2f7b 100644 --- a/Tests/Integration/PeerTeardownTests.cpp +++ b/Tests/Integration/PeerTeardownTests.cpp @@ -19,6 +19,8 @@ #include "mafianet/sleep.h" #include "mafianet/GetTime.h" +#include + using namespace MafiaNet; class PeerTeardown : public ::testing::Test @@ -30,6 +32,10 @@ class PeerTeardown : public ::testing::Test // their server-side slots linger as zombies until the server's timeout // reaps them. Give the server enough headroom that every churn round can // connect fresh clients while earlier rounds' zombies are still pending. + // TrackNewClient() hands out references into this vector; reserve enough + // that no test can trigger a reallocation and invalidate them. + clients.reserve(kServerCapacity); + server = RakPeerInterface::GetInstance(); SocketDescriptor sd(0, "127.0.0.1"); ASSERT_EQ(server->Startup(kServerCapacity, &sd, 1), RAKNET_STARTED); @@ -37,8 +43,20 @@ class PeerTeardown : public ::testing::Test serverPort = server->GetInternalID().GetPort(); } + // Every client is registered in `clients`, so cleanup survives a failed + // fatal assertion mid-test: any peer not already destroyed by the test body + // is shut down here. void TearDown() override { + for (RakPeerInterface *&client : clients) + { + if (client) + { + client->Shutdown(100); + RakPeerInterface::DestroyInstance(client); + client = nullptr; + } + } if (server) { server->Shutdown(100); @@ -46,6 +64,21 @@ class PeerTeardown : public ::testing::Test } } + // Create a client peer that TearDown() will clean up if the test body + // doesn't destroy it first. Returns a reference to the tracked slot so the + // test can mark it destroyed (slot = nullptr) after DestroyInstance. + RakPeerInterface *&TrackNewClient() + { + clients.push_back(RakPeerInterface::GetInstance()); + return clients.back(); + } + + static void DestroyTrackedClient(RakPeerInterface *&slot) + { + RakPeerInterface::DestroyInstance(slot); + slot = nullptr; + } + // Pump a peer's receive queue so its user-thread bookkeeping advances. static void Pump(RakPeerInterface *peer) { @@ -54,15 +87,20 @@ class PeerTeardown : public ::testing::Test peer->DeallocatePacket(packet); } + // Wait until BOTH sides have observed the connection: the client reports + // IS_CONNECTED to the server address, and the server reports IS_CONNECTED + // for the client's bound address. bool WaitForConnection(RakPeerInterface *client, TimeMS timeoutMs) { SystemAddress serverAddress("127.0.0.1", serverPort); + SystemAddress clientAddress("127.0.0.1", client->GetInternalID().GetPort()); TimeMS start = GetTimeMS(); while (GetTimeMS() - start < timeoutMs) { Pump(client); Pump(server); - if (client->GetConnectionState(serverAddress) == IS_CONNECTED) + if (client->GetConnectionState(serverAddress) == IS_CONNECTED && + server->GetConnectionState(clientAddress) == IS_CONNECTED) return true; RakSleep(10); } @@ -73,6 +111,7 @@ class PeerTeardown : public ::testing::Test static const int kServerCapacity = 64; RakPeerInterface *server = nullptr; unsigned short serverPort = 0; + std::vector clients; }; // A single client repeatedly connects and is torn down mid-connection. Every @@ -83,7 +122,7 @@ TEST_F(PeerTeardown, DestroyWithLiveConnectionThenRecreateRepeatedly) const int kCycles = 12; for (int cycle = 0; cycle < kCycles; cycle++) { - RakPeerInterface *client = RakPeerInterface::GetInstance(); + RakPeerInterface *&client = TrackNewClient(); SocketDescriptor clientSd(0, "127.0.0.1"); ASSERT_EQ(client->Startup(1, &clientSd, 1), RAKNET_STARTED) << "cycle " << cycle << ": client failed to start"; @@ -91,25 +130,25 @@ TEST_F(PeerTeardown, DestroyWithLiveConnectionThenRecreateRepeatedly) ASSERT_EQ(client->Connect("127.0.0.1", serverPort, nullptr, 0), CONNECTION_ATTEMPT_STARTED) << "cycle " << cycle; ASSERT_TRUE(WaitForConnection(client, 5000)) - << "cycle " << cycle << ": client never connected"; + << "cycle " << cycle << ": connection not observed on both sides"; // Tear the peer down while the connection is fully live. Alternate // between a graceful window and an immediate teardown so both paths // (flush-and-close and drop-everything) run under sanitizers. client->Shutdown(cycle % 2 == 0 ? 100 : 0); EXPECT_FALSE(client->IsActive()) << "cycle " << cycle; - RakPeerInterface::DestroyInstance(client); + DestroyTrackedClient(client); } // The server must survive all of that with its own threads intact: it can // still accept a fresh connection afterwards. - RakPeerInterface *client = RakPeerInterface::GetInstance(); + RakPeerInterface *&client = TrackNewClient(); SocketDescriptor clientSd(0, "127.0.0.1"); ASSERT_EQ(client->Startup(1, &clientSd, 1), RAKNET_STARTED); ASSERT_EQ(client->Connect("127.0.0.1", serverPort, nullptr, 0), CONNECTION_ATTEMPT_STARTED); EXPECT_TRUE(WaitForConnection(client, 5000)) << "server no longer accepts connections after churn"; client->Shutdown(100); - RakPeerInterface::DestroyInstance(client); + DestroyTrackedClient(client); } // Several clients are destroyed at once while all their connections are live, @@ -117,23 +156,24 @@ TEST_F(PeerTeardown, DestroyWithLiveConnectionThenRecreateRepeatedly) // ManyClientsOneServerDeallocateBlockingTests that exposed the race. TEST_F(PeerTeardown, DestroyManyClientsSimultaneouslyWhileConnected) { - RakPeerInterface *clients[kMaxClients]; - for (int round = 0; round < 3; round++) { + // Indices into the fixture-tracked list for this round's clients. + std::vector roundClients; for (int i = 0; i < kMaxClients; i++) { - clients[i] = RakPeerInterface::GetInstance(); + RakPeerInterface *&client = TrackNewClient(); + roundClients.push_back(clients.size() - 1); SocketDescriptor sd(0, "127.0.0.1"); - ASSERT_EQ(clients[i]->Startup(1, &sd, 1), RAKNET_STARTED) << "round " << round << " client " << i; - ASSERT_EQ(clients[i]->Connect("127.0.0.1", serverPort, nullptr, 0), CONNECTION_ATTEMPT_STARTED); + ASSERT_EQ(client->Startup(1, &sd, 1), RAKNET_STARTED) << "round " << round << " client " << i; + ASSERT_EQ(client->Connect("127.0.0.1", serverPort, nullptr, 0), CONNECTION_ATTEMPT_STARTED); } for (int i = 0; i < kMaxClients; i++) - ASSERT_TRUE(WaitForConnection(clients[i], 5000)) << "round " << round << " client " << i; + ASSERT_TRUE(WaitForConnection(clients[roundClients[i]], 5000)) << "round " << round << " client " << i; // No Shutdown() call first: DestroyInstance itself must cope with live // connections and running threads. - for (int i = 0; i < kMaxClients; i++) - RakPeerInterface::DestroyInstance(clients[i]); + for (size_t index : roundClients) + DestroyTrackedClient(clients[index]); } } diff --git a/Tests/Unit/ConcurrencyPrimitivesTests.cpp b/Tests/Unit/ConcurrencyPrimitivesTests.cpp new file mode 100644 index 000000000..2f95dc09d --- /dev/null +++ b/Tests/Unit/ConcurrencyPrimitivesTests.cpp @@ -0,0 +1,182 @@ +/* + * Copyright (c) 2026, MafiaHub + * + * This source code is licensed under the MIT-style license found in the + * license.txt file in the root directory of this source tree. + */ + +// Regression coverage for the synchronization primitives hardened alongside +// https://github.com/MafiaHub/MafiaNet/issues/7: LocklessUint32_t (now +// std::atomic-backed), ThreadsafeAllocatingQueue's PopInaccurate (no longer +// probes the queue outside its mutex), and GetTimeUS' first-use +// initialization (now a thread-safe magic static). All tests are join-based: +// no wall-clock deadlines, deterministic assertions. Their data-race aspect +// is what TSan runs of this suite verify. + +#include + +#include "mafianet/LocklessTypes.h" +#include "mafianet/DS_ThreadsafeAllocatingQueue.h" +#include "mafianet/GetTime.h" +#include "mafianet/thread.h" +#include "mafianet/defines.h" + +#include +#include + +using namespace MafiaNet; + +namespace +{ + +const int kThreads = 8; +const int kOpsPerThread = 10000; + +struct CounterJob +{ + LocklessUint32_t *counter; + bool increment; +}; + +RAK_THREAD_DECLARATION(CounterThread) +{ + CounterJob *job = (CounterJob *) arguments; + for (int i = 0; i < kOpsPerThread; i++) + { + if (job->increment) + job->counter->Increment(); + else + job->counter->Decrement(); + } + return 0; +} + +struct QueueItem +{ + unsigned value; +}; + +typedef DataStructures::ThreadsafeAllocatingQueue ItemQueue; + +struct ProducerJob +{ + ItemQueue *queue; + unsigned firstValue; // pushes firstValue .. firstValue+kOpsPerThread-1 +}; + +RAK_THREAD_DECLARATION(ProducerThread) +{ + ProducerJob *job = (ProducerJob *) arguments; + for (int i = 0; i < kOpsPerThread; i++) + { + QueueItem *item = job->queue->Allocate(_FILE_AND_LINE_); + item->value = job->firstValue + (unsigned) i; + job->queue->Push(item); + } + return 0; +} + +RAK_THREAD_DECLARATION(GetTimeThread) +{ + MafiaNet::TimeUS *out = (MafiaNet::TimeUS *) arguments; + *out = MafiaNet::GetTimeUS(); + return 0; +} + +} // namespace + +TEST(LocklessUint32, ReturnsValueAfterChange) +{ + LocklessUint32_t counter; + EXPECT_EQ(counter.GetValue(), 0u); + // Both mutators are documented to return the value AFTER the change; the + // old __sync_fetch_and_add implementation returned the value before it. + EXPECT_EQ(counter.Increment(), 1u); + EXPECT_EQ(counter.Increment(), 2u); + EXPECT_EQ(counter.Decrement(), 1u); + EXPECT_EQ(counter.Decrement(), 0u); +} + +TEST(LocklessUint32, ConcurrentIncrementsAndDecrementsBalanceExactly) +{ + // Seed high enough that concurrent decrements can never underflow. + LocklessUint32_t counter(kThreads * kOpsPerThread); + + CounterJob jobs[kThreads]; + RakThread::ThreadHandle handles[kThreads]; + for (int i = 0; i < kThreads; i++) + { + jobs[i].counter = &counter; + jobs[i].increment = (i % 2 == 0); // half increment, half decrement + ASSERT_EQ(RakThread::CreateJoinable(CounterThread, &jobs[i], &handles[i]), 0); + } + for (int i = 0; i < kThreads; i++) + RakThread::Join(handles[i]); + + // Equal numbers of increments and decrements: no update may be lost. + EXPECT_EQ(counter.GetValue(), (uint32_t)(kThreads * kOpsPerThread)); +} + +TEST(ThreadsafeAllocatingQueue, ConcurrentPushersLoseNothing) +{ + ItemQueue queue; + + ProducerJob jobs[kThreads]; + RakThread::ThreadHandle handles[kThreads]; + for (int i = 0; i < kThreads; i++) + { + jobs[i].queue = &queue; + jobs[i].firstValue = (unsigned) (i * kOpsPerThread); + ASSERT_EQ(RakThread::CreateJoinable(ProducerThread, &jobs[i], &handles[i]), 0); + } + + // Pop concurrently with the producers through the code path the network + // thread uses (PopInaccurate), then drain the remainder after joining. + std::set seen; + const size_t expected = (size_t) kThreads * kOpsPerThread; + while (seen.size() < expected / 2) + { + QueueItem *item = queue.PopInaccurate(); + if (item == nullptr) + continue; + EXPECT_TRUE(seen.insert(item->value).second) << "duplicate value " << item->value; + queue.Deallocate(item, _FILE_AND_LINE_); + } + + for (int i = 0; i < kThreads; i++) + RakThread::Join(handles[i]); + + // All producers done: everything not yet seen must still be in the queue. + QueueItem *item; + while ((item = queue.PopInaccurate()) != nullptr) + { + EXPECT_TRUE(seen.insert(item->value).second) << "duplicate value " << item->value; + queue.Deallocate(item, _FILE_AND_LINE_); + } + EXPECT_EQ(seen.size(), expected); +} + +TEST(GetTime, ConcurrentCallsShareOneTimeBase) +{ + // Exercises concurrent (potentially first) calls to GetTimeUS. Before the + // magic-static fix a racing thread could observe a torn or stale base time + // and return a wildly wrong timestamp. + MafiaNet::TimeUS results[kThreads]; + RakThread::ThreadHandle handles[kThreads]; + for (int i = 0; i < kThreads; i++) + ASSERT_EQ(RakThread::CreateJoinable(GetTimeThread, &results[i], &handles[i]), 0); + for (int i = 0; i < kThreads; i++) + RakThread::Join(handles[i]); + + const MafiaNet::TimeUS after = MafiaNet::GetTimeUS(); + for (int i = 0; i < kThreads; i++) + { + // 0 is legitimate for a caller racing the very first initialization + // (the base time is the first call), so no lower bound beyond the type. + // Every concurrent reading must lie in the past relative to a call made + // after all of them completed, and within the same time base (a torn + // base would put it minutes-to-years off). + EXPECT_LE(results[i], after) << "thread " << i; + EXPECT_LT(after - results[i], (MafiaNet::TimeUS) 60 * 1000000) << "thread " << i; + } +} diff --git a/Tests/Unit/RakThreadTests.cpp b/Tests/Unit/RakThreadTests.cpp index 469388028..0867bee29 100644 --- a/Tests/Unit/RakThreadTests.cpp +++ b/Tests/Unit/RakThreadTests.cpp @@ -56,7 +56,14 @@ TEST(RakThread, JoinReapsMultipleThreadsIndependently) RakThread::ThreadHandle handleA; RakThread::ThreadHandle handleB; ASSERT_EQ(RakThread::CreateJoinable(IncrementCounterThread, &counterA, &handleA), 0); - ASSERT_EQ(RakThread::CreateJoinable(IncrementCounterThread, &counterB, &handleB), 0); + const int createB = RakThread::CreateJoinable(IncrementCounterThread, &counterB, &handleB); + if (createB != 0) + { + // Reap thread A before the fatal exit: it references this frame's + // counterA, which dies when the assertion returns. + RakThread::Join(handleA); + FAIL() << "second CreateJoinable failed: " << createB; + } RakThread::Join(handleB); EXPECT_EQ(counterB.load(), 1);