Repository navigation
Conversation
…y migrate StatisticsHistory keeps two container-backed stores -- `objects`, a DataStructures::OrderedList keyed by objectId, and per-object `dataQueues`, a DataStructures::Hash of key -> sample queue. Stage 2 of #60 moves both to the standard library, and the only existing suite covers the plugin's sampling clock, touching neither. The property that matters is order. Three public methods expose the index of `objects` directly -- GetObjectIndex, GetObjectAtIndex, RemoveObjectAtIndex -- so "index" means position in ascending objectId order, not insertion order. That rules out std::map for the replacement (no O(1) indexed access) and makes an unsorted std::vector a silent renumbering of every index. Mutation-checked: appending instead of inserting at the sorted position, which is the mistake a careless OrderedList -> std::vector migration makes, fails 5 of these 16 and none of the 4 pre-existing sampling tests.
…containers Part of #60, stage 2. Both stores are protected members, so no public declaration changes here. - `objects`: DataStructures::OrderedList<uint64_t, TrackedObject*, Comp> -> std::vector<TrackedObject*> kept sorted by objectId, searched with std::lower_bound. Deliberately NOT std::map: GetObjectIndex, GetObjectAtIndex and RemoveObjectAtIndex all publish this index, so being both sorted and O(1) indexable is a public contract. - `dataQueues`: DataStructures::Hash<RakString, ..., 32, RakString::ToInteger> -> std::unordered_map with a hasher reusing that same ToInteger. - GetHistorySorted's local OrderedList -> std::vector + std::sort with the unchanged comparators. Equivalent rather than approximately so: the comparators tie-break on the key name after sortValue and keys are unique within an object, so they never return 0 for two distinct queues -- which matters because the OrderedList::Insert they replace refused comparator-equal entries outright and would have dropped such a queue. - GetUniqueKeyList now iterates the map instead of materialising two parallel lists via GetAsList. Its output order was previously hash-bucket order and is now unordered_map order; both are unspecified and the only contract is that it is a set union, which the tests assert as a set. Fixes a leak found while reading this code: TrackedObject::~TrackedObject declared two empty local lists and iterated the empty one, never calling GetAsList, so the OP_DELETE never ran and every TimeAndValueQueue the object owned leaked -- one per distinct key passed to AddValueByObjectID, and nothing else in the file freed them. LeakSanitizer on Linux reported 12 leaked queues plus their sample arrays for 4 objects x 3 keys before, and 0 records after. Note the leak fix itself is not guarded by the test suite: it is only observable under a leak checker, and CI has no sanitizer job. The 16 tests added in dd40528 cover the destructor path functionally but would still pass if the fix were reverted. Left as stage 3 work, bound to public signatures: GetHistorySorted and GetUniqueKeyList out-parameters, ResizeSampleSet's histogram argument and the local it is passed, and three locals that are arguments to RakPeerInterface::GetStatisticsList.
…red id Found by writing the tests that have to exist before RPC4's registration tables move to std containers (#60, stage 2). UnregisterLocalCallback put its `return true` inside the branch that fires only when the removed name was the LAST one for that message id. With two or more names registered against one id, removing one succeeded but the call returned false, contradicting the documented contract: "True if the combination of uniqueID and messageId was in use, and hence removed". Adds 14 cases covering the three registration tables: register/unregister round-trips for non-blocking, blocking and local-callback entries, refusal of a duplicate name, independence of the blocking and non-blocking tables, 200 names at once so a migration that collapses colliding keys shows up here, and loopback dispatch reaching the registered function. RPC4Registry.OneMessageIdCarriesSeveralFunctionNames fails before this fix and passes after. Not flaky: --gtest_repeat=10 clean. The fixture starts the peer on an ephemeral 127.0.0.1 port, because CallLoopback pushes a packet that only surfaces through Receive() on a started peer; there is no connection and no traffic, so the suite stays hermetic.
Part of #60, stage 2. All three are protected members, so no public declaration changes. - registeredNonblockingFunctions, registeredBlockingFunctions: DataStructures::Hash<RakString, ..., 64, RakString::ToInteger> -> std::unordered_map with a hasher reusing that same ToInteger. Hash::Pop became find + erase, which is also what gives UnregisterFunction and UnregisterBlockingFunction their bool. - localCallbacks: DataStructures::OrderedList<MessageID, LocalCallback*, Comp> -> std::vector<LocalCallback*> kept sorted by messageId and searched with std::lower_bound through the new IndexForMessageId helper. The dispatch path looks up by message id for every received packet, so the ordering is kept rather than traded for an unsorted vector. - CallBlocking's local DataStructures::Queue<Packet*> -> std::deque. It was used as a stack, not a queue: PushAtHead(p,0) pushed to the front and Pop() took from the front, so held packets are handed back in reverse arrival order. push_front/pop_front preserves that; a vector with push_back/pop_back would have reversed the order they are given back to the peer. Left as stage 3 work: localSlots, LocalCallback::functions and LocalSlot::slotObjects are tagged \internal but declared in RPC4's public section, and GetLocalSlotIndex/InvokeSignal take DataStructures::HashIndex because of it. Those four cannot move while the declarations are public. Unit suite green, RPC4Context integration test green, RPC4 suites clean under --gtest_repeat=5.
…ntainers Part of #60, stage 2. Everything changed here is protected; no public declaration moves. - worldsList: DataStructures::List<RM3World*> -> std::vector. RemoveWorld's RemoveAtIndexFast is reproduced explicitly as a swap-remove, not an erase: GetWorldIdAtIndex documents that "worlds will not necessarily be in the order added with AddWorld()", so the swap is a publicly observable contract and an erase would renumber every later world. - constructedReplicaList: OrderedList<Replica3*, LastSerializationResult*, Replica3LSRComp> -> std::vector kept sorted by Replica3::referenceIndex, with IndexForReplica (lower_bound) and InsertConstructedReplica replacing GetIndexFromKey and Insert. InsertConstructedReplica keeps the old contract exactly: refuse a duplicate, assert, return (unsigned)-1. - queryToConstruct/Serialize/DestructReplicaList: List -> std::vector. - downloadGroup: DataStructures::Queue<Packet*> -> std::deque. Adds 6 tests. Two of them exist because the migration was not covered at all: - RemoveWorldSwapsTheLastWorldIntoTheHole pins the documented swap-remove; turning it into an erase fails it. - EveryConstructedReplicaIsFoundWhateverTheConstructionOrder pins that constructedReplicaList stays sorted. This was a real hole: appending instead of inserting in place passed all 304 unit tests, because every existing test constructs at most one replica and one element is trivially sorted. The test constructs 16 in an interleaved order and looks each one up through the public HasReplicaConstructed; it fails against the appending version. Left as stage 3 work: constructedReplicasCulled and destroyedReplicasCulled stay DataStructures::List because they are handed straight to the public virtuals QueryReplicaList and SendConstruction, and converting at that boundary would copy both lists every tick on the serialize path. RM3World::connectionList and userReplicaList are public members of a public struct.
…stries TeamManager had no tests. It holds the most container references left in Source/ after RPC4 and ReplicaManager3 (39), and two of its orderings are published and differ from each other, which is what makes a mechanical swap risky in #60 stage 2: - TM_World::GetTeamByIndex / GetTeamIndex and the team-member equivalents publish the index of order-PRESERVING lists; DereferenceTeam and DereferenceTeamMember use RemoveAtIndex, so the remainder shifts down. - TeamManager::GetWorldAtIndex publishes the index of an order-NOT-preserving list; RemoveWorld uses RemoveAtIndexFast, swapping the last world into the hole. A migration that applied one idiom to both would renumber half of these while the NetworkID lookups kept answering correctly, so nothing else would notice. 12 cases, all passing against the current implementation: index order for teams and members, shift-down on removal, swap-into-hole on world removal, NetworkID lookup resolution and absence through teamsHash/teamMembersHash, 120 teams at once to spread hash buckets, and participant round-trip.
Part of #60, stage 2. Everything changed is protected; the public getters keep their DataStructures::List out-parameters. - TM_World::teams, teamMembers, participants; TM_TeamMember::teams, teamsRequested, lastTeams; TM_Team::teamMembers: List -> std::vector, with RemoveAtIndex becoming erase so the published indices keep shifting down. - TM_World::teamsHash, teamMembersHash: DataStructures::Hash<NetworkID, ..., 256, ToUint32> -> std::unordered_map. NetworkID is an integer type, so the default std::hash applies and the ToUint32 projection is no longer needed. - TeamManager::worldsList: List -> std::vector, with RemoveAtIndexFast reproduced as an explicit swap-remove. GetWorldAtIndex exposes that index, and unlike the team lists it has never preserved order. - GetSortedJoinRequests: OrderedList<JoinRequestHelper, ...> -> std::vector + std::sort with the unchanged comparator. requestIndex breaks ties and is a monotonically increasing per-request counter, so no two requests compare equal and the sort matches the ordered inserts exactly. - A local DataStructures::List working copy in GetAvailableTeamIndexWithFewestMembers keeps its swap-remove, which is correct there because the loop re-tests the same index. Adds a file-local IndexOf to replace List::GetIndexOf including its (unsigned)-1 result, and CopyToList to fill the four still-public DataStructures::List out-parameters from the internal vectors. Stage 3 removes those conversions with the signatures. Two corrections to the tests added in 9a91bea, both found by mutation: - RemoveWorldSwapsTheLastWorldIntoTheHole removed the second-to-last world, which is degenerate: swap-into-hole and shift-down agree there, and turning the swap into an erase passed. It now removes an index with two worlds after it and fails against the erase. - Turning the team list's erase into a swap-remove fails DereferencingATeamShiftsTheRestDown, so the opposite direction is pinned too.
Part of #60, stage 2. TeamBalancer is now free of DataStructures:: entirely -- all four members and both helper signatures were protected, and the locals are file-scope. - myTeamMembers, teamLimits, teamMemberCounts, teamMembers: List -> std::vector. - GetMemberIndexToSwitchTeams and GetOverpopulatedTeams (both protected) take std::vector, as do the four locals passed to them. - Both RemoveAtIndexFast sites are reproduced as explicit swap-removes. DeleteMember's is unobservable through the public API, since nothing exposes the index of myTeamMembers and every lookup is by NetworkID; it is kept because it is the cheaper operation. RemoveTeamMember's does matter: that order decides which member is moved when teams are rebalanced. - Adds a file-local ReplaceAt for DataStructures::List::Replace(input, filler, position), which assigns at an index and grows the list filling any gap. std::vector::resize(n, filler) gives the same result. 9 tests covering the client-side membership list: request, cancel, request-any, delete, duplicate-request handling, independence of several members, and 64 members with every other one deleted to drive the swap path. The suite deliberately does not claim to pin the swap-remove on myTeamMembers: swap and shift are indistinguishable through the public API there, and the test file says so rather than implying coverage it does not have. The host-side teamMembers order, which does affect rebalancing, is driven by received packets and is not reachable hermetically.
…tainers Part of #60, stage 2. Both members are protected. - filterList: OrderedList<int, FilterSet*, FilterSetComp> -> std::vector kept sorted by filterSetID, searched through the new IndexForFilterSetID (lower_bound). GetFilterSetIDByIndex publishes this index, so the ordering stays. - systemList: DataStructures::Hash<AddressOrGUID, FilteredSystem, 2048, AddressOrGUID::ToInteger> -> std::unordered_map with a hasher reusing that same ToInteger, so bucketing and key equality are unchanged. - GetSystemCount and DeleteFilterSet now iterate the map instead of materialising two parallel lists through GetAsList. DeleteFilterSet erases while iterating via the iterator returned by erase. - Update() still takes a snapshot into two std::vectors rather than iterating in place, because the timeout callbacks can close connections and reach back into systemList. The DataStructures::List copy it replaces had the same effect; dropping the snapshot would have invalidated the iterator. Inherited deliberately, and now written down: AddressOrGUID::operator== matches when EITHER the guid or the address matches, while ToInteger hashes on the guid when one is set and the address otherwise. Two keys that compare equal can hash differently and miss each other. DataStructures::Hash had the same hazard for the same reason, so this is not a regression -- but changing key equality would change which systems a filter set matches, which is not a stage 2 decision. FilterSet::allowedRPC4 stays an OrderedList: it is a public member of a public struct.
Part of #60, stage 2. All six members are protected; TCPInterface now has no live DataStructures:: use of the stage 2 containers (one commented-out line remains). - headPush, tailPush: Queue<Packet*> -> std::deque, push_back/pop_front. FIFO within each queue, and ReceiveInt drains headPush before tailPush. - completedConnectionAttempts, failedConnectionAttempts: Queue<SystemAddress> -> std::deque, same shape, still under their mutexes. - messageHandlerList: List -> std::vector. DetachPlugin's swap-remove was already written out by hand and is kept. - blockingSocketList: List -> std::vector, RemoteAtIndexFast reproduced as a swap-remove; that order is not published. - activeSSLConnections: List -> std::vector with std::find. - PacketizedTCP derives from TCPInterface and reads messageHandlerList, so two uses there move with it. 7 tests. These are worth having beyond the migration: PushBackPacket and Receive are public and reachable without a socket, so the head/tail ordering contract is testable directly. Started with maxIncomingConnections of 0, which creates no listen socket -- only the update thread -- so the suite stays hermetic with no bind and no traffic. Mutation-checked: making the head queue LIFO fails 2 of the 7. The cases push several packets per queue on purpose; one packet per queue would pass against a reversed queue.
Part of #60, stage 2. Small by design: most of Table's container surface is public and therefore stage 3. - AddRowColumns and QueryRow are protected, so their DataStructures::List<unsigned> parameters become std::vector<unsigned>, along with the three locals passed to them and the file-static _columnIndices the sort callback reads. Left alone deliberately: - Table::columns stays a DataStructures::List because GetColumns() is public and returns a const reference to it. Changing the member would break that signature, and holding a converted copy to return a reference from is worse than waiting for stage 3. - Row::cells is a public member of a public struct. - table_serializer.cpp was checked and has nothing in scope: every DataStructures:: reference there is in a public static signature. Covered by the 18 existing Table tests, which exercise query, sort and the serialization round trip.
DecodeTeamAssigned's three DataStructures::List<TM_Team*> out-parameters are a protected signature, so they were in scope for stage 2 and I missed them -- found by classifying every remaining reference in Source/include by access section rather than by grepping file totals. They become std::vector, along with the three locals its only caller passes. team_manager.h now has no stage 2 container reference left in any protected or private section; the five that remain are the public getters, which stage 3 moves.
…plit packets Written before stage 2 of #60 moves ReliabilityLayer's protected containers, because two of them had no coverage that could distinguish a correct migration from a broken one: - datagramHistory is a sliding window indexed by (datagramNumber - datagramHistoryPopCount) in WRAPPING uint24_t arithmetic. Every existing test sends few enough datagrams that the pop count stays at or near zero, where an off-by-one in that arithmetic is invisible. The new sustained-traffic case pushes 300 small messages through 10% loss, so the window advances and pops hundreds of times and resend lookups land at varying offsets, then asserts reliable-ordered delivery of all 300 in order. - splitPacketChannelList is an ordered list keyed by split-packet id, and every existing test has exactly ONE split message in flight. With a single element any ordering or lookup passes, so the container's job was untested. Three new cases put two and then three split messages in flight at once, with and without loss, and assert each reassembles as itself rather than being conflated with another. These are not MTU black hole tests, so they are marked off in the file with their own explanation; they live here to reuse the existing two-layer fake-socket rig with simulated time rather than duplicate it. All four pass against the current implementation.
…ainers Part of #60, stage 2. Eleven protected members plus three locals. - outputQueue, datagramHistory, hasReceivedPacketQueue, delayList: DataStructures::Queue -> std::deque. - splitPacketChannelList: OrderedList<SplitPacketIdType, SplitPacketChannel*, SplitPacketChannelComp> -> std::vector kept sorted, with IndexForSplitPacketId (lower_bound) and InsertSplitPacketChannel. Both reuse SplitPacketChannelComp rather than reimplementing its key extraction, which differs under PREALLOCATE_LARGE_MESSAGES. - unreliableWithAckReceiptHistory and the five per-update send lists: List -> std::vector, with Preallocate becoming reserve. Three things in here are behaviour or semantics, not mechanics: 1. hasReceivedPacketQueue's AllocationSize()/Compress() compaction is removed. It existed only because the hand-rolled ring buffer never shrank; std::deque releases blocks as it drains, so there is nothing to compact. 2. uint24_t + size_t is ambiguous where uint24_t + unsigned was not, because uint24_t has both operator uint32_t() and an implicit uint24_t(const uint32_t&). size() returns size_t where Size() returned unsigned, so the datagram-boundary arithmetic is cast back to unsigned int to keep the original overload resolution. Same root cause as the RangeSum bug fixed in stage 1. 3. delayList's PushAtHead(dat, i) inserts at an arbitrary index to keep the list ordered by sendTime; it is now insert(begin() + i, dat), not push_front. My first attempt at this was wrong and the new tests caught it. InsertSplitPacketChannel originally read the split id back out of the channel, but the call site inserts the channel before populating its split packet list, so it keyed on uninitialised memory and segfaulted. Running the same test against the pre-migration source passed, which pinned it on this change rather than on a pre-existing bug or a bad test. The id is now a parameter. What these tests do NOT cover, stated so nobody assumes otherwise: I could not construct a case that distinguishes a sorted insert from an append for splitPacketChannelList. Split ids are handed out in ascending order, so appending stays sorted by accident; adding datagram reordering to the rig and going to two, three and eight concurrent split messages all still passed against an appending mutant. The sorted insert is kept because it mirrors the original and keeps lookup logarithmic, but it is correct by construction, not by test. Also fixes replica_manager3's constructedReplicaList.HasData, which only compiles under _DEBUG and so was missed until the Linux Debug build. Verified: macOS full suite 408 tests exit 0; macOS Debug build and unit suite clean; Linux Docker Debug 408/408 and Release 408/408 per CLAUDE.md, which is what caught the three _DEBUG-only compile errors in the first place. Left for the peer migration: the DataStructures::List<PluginInterface2*> parameters threaded through this file belong to peer's pluginListTS/NTS and have to move with them.
Covers eight of RakPeer's protected containers (issue #60): banList List<BanStruct*> -> std::vector securityExceptionList List<RakString> -> std::vector socketList List<RakNetSocket2*> -> std::vector SocketQueryOutput::sockets List<RakNetSocket2*> -> std::vector packetReturnQueue Queue<Packet*> -> std::deque requestedConnectionQueue Queue<RequestedConnectionStruct*> -> std::deque bufferedPacketsQueue Queue<RNS2RecvStruct*> -> std::deque bufferedPacketsFreePool Queue<RNS2RecvStruct*> -> std::deque plus the local drain queue in ClearRequestedConnectionList. Tests first: Tests/Unit/PeerListsTests.cpp, 21 hermetic cases covering the ban list (exact and wildcard matches, swap-remove, in-place timeout update, the expiry sweep), the security exception list (multi-match removal, clear-on-null), the packet return buffer (PushBackPacket at head and tail, interleaved) and the socket list (one socket per descriptor, in index order). No Connect(), no loopback traffic; the only use of time is a ban created already expired. Removal semantics preserved exactly: * banList and securityExceptionList wrote their swap-remove out by hand (`list[i] = list[size-1]; RemoveAtIndex(size-1)`), which is `list[i] = list.back(); pop_back()`. Order is not part of their contract, but dropping the swapped-in element is a real bug, and the tests catch it. * DataStructures::Queue::RemoveAtIndex is a *shift*-remove, not a swap-remove, so requestedConnectionQueue's four removal sites became `erase(begin() + i)`, not pop_back. The queue order is not observable from outside, so this is preserved by construction rather than by test. * packetReturnQueue's `PushAtHead(packet, 0, ...)` is push_front; Receive pops the front. Both directions are pinned. Two redundant clears dropped: GetSocket's `output` is a fresh local filled at most once, and GetSockets already clears the caller's list at the top of the function, so clearing again inside the copy loop was dead. With them gone, removing the surviving clear fails a test. Mutation-tested both before and after the migration -- eight mutations, all killed, same tests on both sides: banList swap-remove loses the swapped-in element -> use-after-free in RemovingAMiddleBan... expiry sweep advances past the swapped-in element -> ExpiredBansAreSwept... AddToBanList appends instead of deduplicating -> ReAddingAnIp... (x2) PushBackPacket's head push becomes a tail push -> ...AtHeadJumpsTheQueue Receive pops the tail (buffer becomes a stack) -> ...AtTailIsFifo exception removal advances past the swapped element -> ...RemovesAllMatching socket query answers with only the first socket -> GetSockets... (x3) GetSockets stops clearing the caller's list -> ...ClearsTheCallersList Not migrated, and why: pluginListTS/pluginListNTS and the CallPluginCallbacks parameter stay DataStructures::List. They are handed straight to ReliabilityLayer::Update, UpdateAndForceACKs and HandleSocketReceiveFromConnectedPlayer, whose signatures are public; moving the members without those would mean converting at every call site. Same shape as Table::columns being blocked by the public GetColumns(). Those belong to the public-API stage. remoteSystemIndexPool, packetAllocationPool, bufferedCommands and socketQueryOutput are MemoryPool/ThreadsafeAllocatingQueue, not stage-2 types. Every migrated member keeps its existing mutex and every access stays inside the same critical section; no operation moved across a lock. Verified: macOS full suite 429/429; local Debug build clean + Debug unit 362/362; Linux Docker Debug 429/429 and Release 429/429. Claude-Session: https://claude.ai/code/session_011kRj5nodTJrxyoHY7y6R6c
The page promised more than the library delivers, and the std-container migration (issue #60) makes the gap worth writing down precisely rather than leaving readers to find it. Corrections: * _USE_RAK_MEMORY_OVERRIDE defaults to 0 (defines.h), and the page never said so. In a default build OP_NEW / OP_NEW_ARRAY / OP_DELETE expand to plain new/delete and SetMalloc never sees them. Only the direct rakMalloc_Ex / rakRealloc_Ex / rakFree_Ex calls -- packet payloads, RakString storage, the ban list's IP strings -- are redirected unconditionally. * The standard containers the library now uses internally allocate through std::allocator and are never redirected. Added the comparison that matters: this is not a regression, because DataStructures::List allocated its backing array with OP_NEW_ARRAY, which is already a plain new[] in a default build. The change is observable only in a build that sets _USE_RAK_MEMORY_OVERRIDE=1 *and* installs a custom allocator; pointed at replacing global operator new as the way to cover everything. * RAKNET_MEMORY_TRACKING does not exist anywhere in the tree. Dropped, along with the two _RAKNET_SUPPORT_* entries that have nothing to do with memory. Documented _USE_RAK_MEMORY_OVERRIDE in their place. * "Memory leak detection helpers" -- there are none. Replaced with a pointer to the sanitizers, which also see the std containers. * The tracking example would not compile: the _Ex callbacks take `unsigned int line`, not `int`. Fixed, and added SetRealloc_Ex, whose absence silently left reallocations on the default path. * Pool class names were header-style (DS_MemoryPool<T>, DS_BytePool, DS_ThreadSafeAllocatingQueue). Replaced with the real names (DataStructures::MemoryPool<T>, ::BytePool, ::ThreadsafeAllocatingQueue<T>) and the header each lives in. Verified: sphinx-build exits 0 with no new warning on the page. Claude-Session: https://claude.ai/code/session_011kRj5nodTJrxyoHY7y6R6c
ThreadPool::Clear() locks runThreadsMutex to read runThreads, and unlocks it again only in the branch taken when threads *are* running. On the other branch it clears the three queues and returns with the mutex still held. SimpleMutex is not recursive, so the next lock of runThreadsMutex deadlocks: StartThreads, StopThreads, Pause, WasStarted, and the destructor, which calls StopThreads. A user who calls the documented "clears internal buffers" API on a pool that is not running cannot destroy that pool afterwards. Nothing inside the library calls Clear() -- send_to_thread and file_list_transfer use ClearInput() and StopThreads() -- which is why it survived. thread_pool.h is a public header, so it is reachable. Found while writing Tests/Unit/ThreadPoolQueueTests.cpp (14 cases) ahead of moving the three protected queues to std::deque. The new ClearDoesNotLeaveRunThreadsMutexHeld case hangs against the unfixed header -- verified by reverting the one-line fix and re-running it -- and passes with it. A hang is a worse failure signal than an assertion, but it is the only one this bug produces; ctest's timeout catches it. The other 13 cases pin the queue semantics the migration has to preserve: FIFO order on both queues, GetOutput popping the head, RemoveInputAtIndex and RemoveOutputAtIndex shifting later elements down rather than swapping the last one into the hole, ClearInput and ClearOutput not touching each other's queue, and two fixed-seed differential runs (4000 mixed add/pop/remove-at-index operations each) against a std::deque reference model. Claude-Session: https://claude.ai/code/session_011kRj5nodTJrxyoHY7y6R6c
inputFunctionQueue Queue<OutputType (*)(InputType, bool*, void*)> -> std::deque inputQueue Queue<InputType> -> std::deque outputQueue Queue<OutputType> -> std::deque Issue #60. Tests landed first in 2928ffb, which also fixed the Clear() lock leak they found. Semantics preserved: * DataStructures::Queue::RemoveAtIndex is a shift-remove, so RemoveInputAtIndex and RemoveOutputAtIndex became erase(begin() + index), not pop_back. * It also silently no-ops when the index is past the end, while deque::erase there is undefined. The bound check moved into the two callers, alongside the RakAssert the old container did under _DEBUG. * Queue::Pop and Queue::operator[] asserted their preconditions under _DEBUG and read past the end otherwise. GetOutput, GetInputAtIndex and GetOutputAtIndex now carry those asserts explicitly -- GetOutput's precondition (HasOutput() first) is the one its doc comment already states. * The worker thread takes the head of both paired queues under inputQueueMutex, in the same critical section as before. New: six worker-thread cases in Tests/Integration/ThreadPoolWorkerTests.cpp. The pairing between inputFunctionQueue and inputQueue is not observable from the public API without a worker -- a removal from only one of them produces no size mismatch, just every later job running with its neighbour's callback -- so the tests tag each callback's output differently and assert the exact sequence. Integration rather than unit because they need a real thread and a deadline poll. Not flaky: 10 repeats, 60/60. Six mutations, all killed, each attribution re-verified against a freshly built binary: paired removal drops only inputQueue -> RemovingAnInputKeepsEveryOther..., RemovingTheHeadInputKeepsTheRest... (unit survives, by design) RemoveInputAtIndex -> swap-remove -> RemoveInputAtIndexShiftsLaterInputs..., RemoveInputAtTheHeadAndAtTheTail, InputQueueMatchesADequeModel... worker pops the tail, not the head -> all five ordering cases RemoveOutputAtIndex -> swap-remove -> RemoveOutputAtIndexShiftsLater..., OutputQueueMatchesADequeModel... ClearInput also clears outputQueue -> ClearInputLeavesTheOutputQueueAlone worker ignores returnOutput -> CallbacksThatSuppressOutputPushNothing thread_pool.h no longer includes ds_queue.h. Two headers in DependentExtensions (AutopatcherServer.h, Lobby2Server.h) were getting DataStructures::Queue transitively through it and now include it themselves. thread_pool.h has no DataStructures:: references left. Verified: macOS full suite 449/449; Debug build clean + Debug unit 376/376. Claude-Session: https://claude.ai/code/session_011kRj5nodTJrxyoHY7y6R6c
…e in 35f8f0c 35f8f0c's migration of tcp_interface.cpp replaced DataStructures::List::GetIndexOf with std::find arithmetic but kept the old "not found" comparison. GetIndexOf returned (unsigned)-1 when the value was absent; `std::find(...) - begin()` returns size(). Every one of the four converted lookups therefore tested the wrong thing, and in two of them the test is inverted: StartSSLClient `if (index==(unsigned)-1) push_back(...)` can never be true, so an address was never added to activeSSLConnections and IsSSLActive always said false. DetachPlugin `if (index!=MAX_UNSIGNED_LONG)` is always true, so detaching a plugin that was never attached ran messageHandlerList[size()]->OnDetach() -- one past the end, or a dereference of nothing on an empty list -- and then swap-removed a plugin that was still attached. CloseConnection same always-true test, then erase(begin() + size()). SocketConnect same always-true test, and the replacement turned one statement into two without adding braces, so pop_back() ran unconditionally -- including on an empty list -- while the swap-assignment was the only thing guarded. All four now compare iterators against end() and drop the index entirely, so there is no sentinel left to get wrong. The StartSSLClient and CloseConnection sites are inside OPEN_SSL_CLIENT_SUPPORT==1, which defaults to 0 and is not compiled here; they are fixed but not covered by a test, and I am not claiming otherwise. The other two are always compiled. I audited every index lookup this branch converted. These four are the only ones that got it wrong: the team_manager and team_balancer sites go through an IndexOf helper that preserves (unsigned)-1, and the sorted containers use lower_bound with an explicit key comparison. No raw `std::find(...) - begin()` remains anywhere in Source/. New: Tests/Unit/TCPInterfacePluginTests.cpp, 6 hermetic cases over the plugin list, which had no tests. Against the broken DetachPlugin, three segfault and three pass: AttachThenDetachCallsEachHookOnce survives (index 0 of 1 is valid) DetachingTheMiddleOfThreeKeepsTheOtherTwo survives DetachingNullIsIgnored survives AttachingTheSamePluginTwiceAttachesItOnce crash DetachingAnUnattachedPluginLeavesTheAttachedOneAlone crash DetachingFromAnEmptyListIsANoOp crash The three that survive are the shape the rest of the codebase already uses -- detaching a plugin you attached, where the wrong sentinel happens to give the right index. That is why nothing caught this. Claude-Session: https://claude.ai/code/session_011kRj5nodTJrxyoHY7y6R6c
…rt a failed bind
Three defects, all in Start/Stop, found while writing the first tests
PacketizedTCP has ever had: every test after the first one in the same process
failed, with the client connecting successfully and the server never reporting
the incoming connection.
**Stop() closed descriptor 0, once per unused connection slot.**
RemoteClient's constructor sets `socket = 0`, and 0 is this class's "no socket"
value -- RemoteClient::SetActive already guards on it. Stop() did not, and
closed every slot unconditionally. On POSIX 0 is a valid descriptor: stdin in a
normal process. A server started with maxIncomingConnections of 4 and one
connection called close(0) three times.
The second-order effect is what the tests actually tripped over. Once
descriptor 0 is free, the next socket created *anywhere in the process* is
handed number 0, and every `listenSocket != 0` check then reads it as absent.
A second TCPInterface started, returned true, and never accepted anything;
instrumenting CreateListenSocket showed `listenSocket=0` on the second cycle
against `listenSocket=3` on the first. Stop() also skipped closing that
listener, leaking it.
Fixed by guarding on `socket != 0`, matching SetActive. Using 0 as the sentinel
for a descriptor remains wrong in principle, and is still how listenSocket is
handled throughout this file; with Stop() no longer closing descriptors it does
not own, a normal process keeps stdin on 0 and the collision is unreachable.
Worth replacing properly, not in this commit.
**Stop() did not clear blockingSocketList.**
It closed each descriptor in the list and left them there, so a second Stop(),
or the one the destructor performs, closed the same numbers again. Descriptor
numbers are reused, so the second close can land on a socket something else has
since opened. The list is now cleared.
**Start() discarded CreateListenSocket's return value.**
A failed bind -- the port already in use -- left Start() returning true with
nothing listening and the caller unable to find out. Start() now tears down what
it allocated and returns false, and an interface whose Start failed can be
stopped and started again.
New: Tests/Integration/TCPInterfaceLifecycleTests.cpp, 5 cases, plus two
reusable helpers. TCPInterface::Start takes a port and exposes no accessor for
the one it bound, so a listening test cannot use port 0 and read the result back
the way the peer tests do; TestHelpers::FindFreeTcpPort binds a throwaway socket
to port 0 and reports the assigned port, which beats adding another hard-coded
port to a suite that is already RUN_SERIAL because of them.
That alone was flaky: 1 failure in 90 repeats, `Start` returning false. The
probe binds 127.0.0.1 while TCPInterface binds INADDR_ANY, and the OS reissues
ephemeral port numbers soon after a test releases them, so a bind can hit a port
in use on another interface or still in TIME_WAIT from a previous test's
accepted connection -- TCPInterface sets no SO_REUSEADDR.
TestHelpers::StartTcpOnAFreePort retries the port selection, which absorbs that
without retrying anything a test asserts. 168/168 across 12 repeats of both new
suites afterwards.
Each fix is pinned independently, by reverting one at a time against a build I
checked each time:
revert the Stop() guard + clear -> all 5 fail
(the two descriptor-0 cases directly; the
other three because the freed descriptor 0
is reassigned and read as "no socket")
revert the Start() return -> StartFailsWhenThePortIsAlreadyInUse,
AnInterfaceWhoseStartFailedCanStartAgain
The two descriptor-0 cases are POSIX-only: on Windows descriptor 0 is not a
socket handle, so they are compiled out there and the behavioural three-cycle
case carries that platform.
Claude-Session: https://claude.ai/code/session_011kRj5nodTJrxyoHY7y6R6c
waitingPackets Queue<Packet*> -> std::deque connections Map<SystemAddress, ByteQueue*> -> std::map _newIncomingConnections Queue<SystemAddress> -> std::deque _lostConnections Queue<SystemAddress> -> std::deque _failedConnectionAttempts Queue<SystemAddress> -> std::deque _completedConnectionAttempts Queue<SystemAddress> -> std::deque Issue #60. packetized_tcp.h now has no stage-2 DataStructures:: references; DataStructures::ByteQueue stays, it is not one of the types in scope. `connections` was only ever used as a dictionary: every index came from a key lookup (Has, then GetIndexAtKey, then operator[]), so std::map::find and an iterator replace the whole pattern and the positional accessors disappear. DataStructures::Map kept its entries in key order and so does std::map, but nothing here depended on that. New: Tests/Integration/PacketizedTCPFramingTests.cpp, 9 cases. PacketizedTCP had no tests at all, and none of this is reachable without a real connection -- Send() on an address with no entry in `connections` returns without sending, and the notification queues are fed from TCPInterface's own lock-free queues -- so they run over loopback TCP. They cover a single message's exact length, a batch of twelve in one segment, a 200KB payload reassembled across segments, a small message behind a large one, both directions at once, close and failed-attempt notifications, and two clients at once. Five mutations; three killed, two survived and I am not going to pretend otherwise: waitingPackets drained as a stack -> ABatchOfMessagesInOneSegment..., ASmallMessageAfterALargeOne... reassembled messages queued at head -> same two AddToConnectionList does not insert -> six of the nine reassembly buffer looked up as the first map entry -> SURVIVES with small payloads, because two complete messages written into one buffer still read back correctly in sequence. It only breaks when the reads interleave, so TwoClientsAreFramedSeparately now sends 180KB of 'A' from one client and 180KB of 'B' from the other and asserts both arrive byte-exact; that kills it. RemoveFromConnectionList erases nothing -> SURVIVES. The lost-connection notification is pushed before the removal, so nothing observable changes; the entry leaks until ClearAllConnections. Making it observable needs the same address to reconnect, which means controlling the client's ephemeral port. Not covered. Writing the 180KB two-client case also turned up that PacketizedTCP emits an ID_DOWNLOAD_PROGRESS packet every 64KB while a message is incomplete -- my collector was counting those as messages. It now skips and counts them separately, and the next commit pins that path. AddToConnectionList keeps DataStructures::Map::SetNew's duplicate-key assert and additionally frees the existing buffer instead of overwriting the pointer, which is what SetNew did under the assert in a release build. Claude-Session: https://claude.ai/code/session_011kRj5nodTJrxyoHY7y6R6c
…the progress packet's bitSize
Three defects the new framing tests turned up.
**Stop() freed the queued messages and left the freed pointers in the queue.**
`for (i...) DeallocatePacket(waitingPackets[i]);` with no clear afterwards. A
second Stop() -- or the one a caller makes before destroying the object --
freed every one of them again, and a Receive() after Stop() popped a dangling
pointer, ran it past the plugin chain and handed it to the caller.
**~PacketizedTCP() never freed the queue at all.**
It called ClearAllConnections(), which frees the per-connection reassembly
buffers, and left waitingPackets alone. Receive() reassembles everything
available and returns one message, so any queued remainder leaked on
destruction. Both now go through one ClearWaitingPackets().
**ID_DOWNLOAD_PROGRESS carried the wrong bitSize.**
The progress packet is built with its own length (1 + 12 + 65536 = 65549
bytes), but took `bitSize = BYTES_TO_BITS(incomingPacket->length)` -- the bit
count of whatever TCP segment happened to trigger it. length and bitSize
disagreed on every progress packet. Now taken from its own length.
Tests: four cases added to Tests/Integration/PacketizedTCPFramingTests.cpp.
LargeTransfersEmitWellFormedProgressPackets sends 4MB and asserts progress
packets are emitted and that each one's bitSize matches its own length.
The size is what makes it deterministic: the progress branch only runs
while a message is still incomplete after Receive() has drained what was
available, so a payload that fits in the socket buffers can complete in one
go and emit nothing. A first attempt used two 180KB senders and failed
about one run in ten for exactly that reason. 4MB with a sleep-free poll:
20/20.
ReceiveAfterStopReturnsNothing six messages sent, one Receive
(which queues the other five), Stop, then Receive must return nothing.
StoppingTwiceWithQueuedMessagesIsSafe the same, stopped twice.
DestroyingTheInstanceWithQueuedMessagesIsClean destroys with five messages
queued, no Stop and no drain -- the scenario for the destructor leak.
Mutation-verified, each against a build I checked:
bitSize taken from the incoming packet again -> LargeTransfersEmitWellFormed...
the clear() removed from ClearWaitingPackets -> ReceiveAfterStopReturnsNothing
and StoppingTwiceWith... both
abort on a double free
(SIGABRT), and
LargeTransfers... fails its
assertions
-> ABatchOfMessages... survives,
so the existing cases would
not have caught it
The destructor leak is verified by LeakSanitizer, not by an assertion: an
ASAN+LSAN Debug build on Linux runs the whole suite, including
DestroyingTheInstanceWithQueuedMessagesIsClean, with no leak reported.
Verified: macOS full suite 473/473; Debug build clean + Debug unit 382/382;
Linux ASAN+LSAN clean over all three new suites (24 tests).
Claude-Session: https://claude.ai/code/session_011kRj5nodTJrxyoHY7y6R6c
…ddrinfo leak
Found by an ASAN+LSAN build in Docker, which reported
`heap-use-after-free ... in TCPInterface::SocketConnect`. All three predate this
branch -- they come in with the SLikeNet import -- and all three are on the
default connect path.
**ConnectionAttemptLoop used the thread argument after deleting it.**
MafiaNet::OP_DELETE(s, _FILE_AND_LINE_);
...
tcpInterface->SocketConnect(str1, ..., socketFamily, s->bindAddress);
systemAddress, tcpInterface and socketFamily are copied out of `s` first;
bindAddress is not, and is read after the delete. This is the read ASAN caught,
and it happens on every non-blocking Connect -- which is the default. bindAddress
is now copied into a local before the delete.
**Connect()'s thread-creation error path does the same thing.**
MafiaNet::OP_DELETE(s, _FILE_AND_LINE_);
failedConnectionAttempts.push_back(s->systemAddress);
Only reached when creating the connect thread fails, so it is not routinely hit,
but it is the same mistake. The address is now read out first.
**SocketConnect leaked its getaddrinfo result on every call.**
The walk looking for the first IPv4 address advanced `curAddress`, which was the
only pointer to the head of the list, and no path called freeaddrinfo. The head
is now kept in a separate variable and freed on every exit, including the three
early returns, and the `err != 0` check moved ahead of the dereference it used to
sit behind.
Verified: ASAN+LSAN Debug build on Linux, 24 tests across
PacketizedTCPFraming, TCPInterfaceLifecycle and TCPInterfacePlugins, exit 0 with
no AddressSanitizer error and no leak report -- against the use-after-free it
reported before the fix. macOS full suite 473/473.
A note on how this was nearly missed: CLAUDE.md's Docker recipe does
`cp -r /src /work` before configuring. The tree now carries ~780MB of build
output in build/ and build-debug/, and that copy silently truncated
tcp_interface.cpp inside the container -- gcc reported "unterminated #if" for a
file that compiles fine. Building straight from the read-only mount with
`cmake -S /src -B /b` avoids the copy entirely and is what produced the result
above.
Claude-Session: https://claude.ai/code/session_011kRj5nodTJrxyoHY7y6R6c
…ecipe The Docker verification recipe copied the source tree into the container (`cp -r /src /work`) before configuring. A local build/ directory is a few hundred MB -- this tree carries ~780MB across build/ and build-debug/ -- and once the container's writable layer fills, the copy truncates files silently. The symptom is gcc reporting "unterminated #if" in a file that compiles fine everywhere else, which is easy to read as a real portability problem and waste time on. CMake never writes into the source tree, so `-S /src` against the read-only mount works and there is nothing to copy for. Recipe updated, with the trap written down. Also added the ASAN+LSAN variant, since that is what turned up the use-after-free in TCPInterface::SocketConnect that nothing else was going to find, and LeakSanitizer only exists on Linux so it belongs in this section rather than in a local workflow. Claude-Session: https://claude.ai/code/session_011kRj5nodTJrxyoHY7y6R6c
…ent or after CloseEvent On POSIX, SignaledEvent's condition variable and mutex are created by InitEvent, not by the constructor, but SetEvent called pthread_cond_broadcast unconditionally and WaitOnEvent called pthread_cond_timedwait unconditionally. Broadcasting on an uninitialised pthread_cond_t is undefined, and glibc does not fail -- it blocks forever in __condvar_quiesce_and_switch_g1. The caller hangs, with no diagnostic. This is reachable through a public API. ThreadPool::AddInput signals quitAndIncomingDataEvents and nothing requires StartThreads first, so Tests/Unit/ThreadPoolQueueTests.cpp -- which exercises the queues without starting a worker, deliberately -- hung on the first AddInput in the Linux Debug gate. gdb on the stuck process: #3 ___pthread_cond_broadcast (cond=0xffffc73cd200) #4 MafiaNet::SignaledEvent::SetEvent (...) signaled_event.cpp:112 #5 ThreadPool<int, int>::AddInput (...) thread_pool.h:434 #6 ThreadPoolQueues_RemoveInputAtTheHeadAndAtTheTail_Test::TestBody It is platform-specific, which is why it was never hit: on Windows ::SetEvent(INVALID_HANDLE_VALUE) just fails, and on macOS the uninitialised broadcast happens to return. Fixed in SignaledEvent rather than in ThreadPool, because the hole belongs to the primitive and any caller can fall into it. An isInitialized flag, written by InitEvent and CloseEvent and read under the existing isSignaledMutex, now gates the two pthread calls; the Windows paths gain the equivalent INVALID_HANDLE_VALUE checks they were missing in SetEvent and WaitOnEvent. Two further undefined cases are closed with it: a second InitEvent without a CloseEvent no longer re-runs pthread_cond_init on a live condition variable, and CloseEvent without a preceding InitEvent no longer destroys one that was never created. SetEvent still records isSignaled when there is no condition variable, so a signal raised before InitEvent is delivered to the first WaitOnEvent after it rather than lost. New: Tests/Unit/SignaledEventLifecycleTests.cpp, 8 cases walking the lifecycle out of order -- signal and wait before init, init twice, close without init, init again after close, signal after close. SignaledEvent had no tests. Proved on Linux, in a Debug container, by making the broadcast unconditional again and rebuilding: with the guard SignaledEventLifecycle 8/8, ThreadPoolQueues 14/14 without the guard both time out on their first case (SetEventBeforeInitEventReturns, AddInputAppendsInFifoOrder) A hang is a poor failure signal, but it is the only one this bug produces; the timing assertions in the new tests exist to turn it into a failure, not to measure anything, and are loose by two orders of magnitude. Claude-Session: https://claude.ai/code/session_011kRj5nodTJrxyoHY7y6R6c
…nnection it cannot take
The Linux Release gate aborted eleven tests with
*** bit out of range 0 - FD_SETSIZE on fd_set ***: terminated
while Debug passed all of them. This is the Debug/Release split CLAUDE.md warns
about, and the asymmetry is the whole story: glibc fortifies fd_set under -O2 and
aborts on an out-of-range bit, while an unfortified Debug build writes outside
the set and carries on.
**accept() failure was read as success.**
newSock = accept__(sts->listenSocket, ...);
if (newSock != 0)
{
...
remoteClients[i].socket = newSock;
remoteClients[i].SetActive(true);
accept() returns -1 on POSIX and INVALID_SOCKET on Windows; only this class uses
0 for "no socket". A failed accept -- which happens routinely when a listener is
shut down while the update thread is in its loop, exactly what the Start/Stop
cycle tests do -- therefore stored -1 as a client's socket and marked the slot
active. The next pass through the select loop called FD_SET(-1, &readFD).
The file already carries the bruises from this: two comments read "calling
FD_ISSET with -1 as socket (that's what 0 is set to) produces a bus error under
Linux 64-Bit", and the loops were given `socketCopy != 0` guards. Those guard the
symptom; the -1 was still being stored.
Fixed with a file-local IsUsableSocket() that knows what each platform reports,
now used at all three places that check a socket: accept, socket() in
SocketConnect (`sockfd < 0`, which can never be true on Windows where SOCKET is
unsigned) and socket() in CreateListenSocket (`(int)listenSocket == -1`, which
only worked because of the cast and accepted a socket numbered 0 that this class
cannot represent).
**A connection accepted with every slot full was leaked.**
The recovery branch tested `newRemoteClientIndex == -1`, which the loop above
cannot produce -- it leaves the index at remoteClientsLength -- so it never ran,
and the accepted socket was never closed: a descriptor leak per refused
connection, and a client left connected to a server that would never service it.
Had the branch fired it closed `sts->listenSocket`, the listener, instead of the
connection it could not accommodate. Now tracked with an explicit flag, and it
closes newSock.
New: TCPInterfaceLifecycle.AConnectionRefusedForLackOfSlotsIsClosed -- one slot,
two clients; the second must see its connection closed, and the listener must
still accept once the slot frees.
Proved in a Linux Release container, reverting one half at a time:
with both fixes 19/19 across TCPInterfaceLifecycle and
PacketizedTCPFraming
revert the accept check abort, "bit out of range 0 - FD_SETSIZE"
revert the full-slots close abort (closing the listener feeds the same
select loop a dead descriptor)
Verified: Linux Release 19/19 on the two affected suites; macOS 30/30 across the
three new suites.
Claude-Session: https://claude.ai/code/session_011kRj5nodTJrxyoHY7y6R6c
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stage 2 of the
DataStructures::→ std container migration (#60): every container in a protected or private section, plus the locals and internal signatures that move with them. No public API changes.Based on
release/v0.23.0, following PR #71 (stage 1).Subsystems migrated
statistics_historyOrderedList→ sortedvector,Hash→unordered_maprpc4_pluginHash→unordered_map,OrderedList→ sortedvector,Queue→dequereplica_manager3vector(swap-remove preserved)team_manager,team_balancervector; both now free ofDataStructures::message_filterOrderedList→ sortedvector,Hash→unordered_maptcp_interfacevector/dequeds_tablevectorreliability_layerpeervector/dequethread_pooldeque; now free ofDataStructures::packetized_tcpQueue×5 →deque,Map→std::map; now free of stage-2 typesProtected/private references across the public headers: 133 → 80.
Tests
12 new files, 132 cases. Eight of these subsystems had no tests at all. Tests land before the migration they cover, and every migration is mutation-tested — mutations listed per commit, including the ones that survived and why.
Bugs found and fixed
Nine, each with a test that fails against the old code.
rpc4_pluginUnregisterLocalCallbackreturned false after a successful removalstatistics_history~TrackedObjectiterated an empty local list, leaking every queue it ownedthread_poolClear()returned holdingrunThreadsMutex— the next lock, including the destructor's, deadlockedsignaled_eventSetEvent/WaitOnEventused the condition variable beforeInitEventcreated it. Undefined; on glibc it hangs forever. Reachable viaThreadPool::AddInputtcp_interfaceaccept()failure (−1) read as success and stored as a client socket →FD_SET(-1, …). Aborts under Release fortification, corrupts silently in Debugtcp_interfacetcp_interfaceStop()closed descriptor 0 once per unused slot (stdin), then the next socket in the process got number 0 and was read as "no socket"tcp_interfacegetaddrinfo's list leaked on every attemptpacketized_tcpStop()left freed pointers queued (double free, danglingReceive); the destructor leaked the queue;ID_DOWNLOAD_PROGRESScarried another packet'sbitSizeOne I introduced and fixed in this branch: commit
35f8f0c2replacedGetIndexOfwithstd::findarithmetic intcp_interfacebut kept the old(unsigned)-1comparison, wherestd::find(...) - begin()yieldssize(). Four lookups tested the wrong thing, two of them inverted —DetachPluginon an unattached plugin indexed past the end and swap-removed a live plugin. Fixed in09ca549b, with an audit of every index lookup this branch converted; no rawstd::find(...) - begin()remains inSource/.Not migrated, and why
pluginListTS/pluginListNTSinpeer, and theCallPluginCallbacksparameter — handed straight toReliabilityLayer::Updateand friends, whose signatures are public. Same shape asTable::columnsbehind the publicGetColumns(). Public-API stage.cloud_server(17),fully_connected_mesh2(7) and the smaller plugins — need multi-peer harnesses that do not exist yet.MemoryPool,ThreadsafeAllocatingQueue,ByteQueue— not stage-2 types.Also
docs/advanced/custom-memory-management.rstpromised allocator coverage the containers never had._USE_RAK_MEMORY_OVERRIDEdefaults to 0, soOP_NEW/OP_NEW_ARRAYare plainnewand the std containers usestd::allocator; corrected, including that this is not a regression, sinceDataStructures::Listalready allocated throughOP_NEW_ARRAY. DroppedRAKNET_MEMORY_TRACKING, which does not exist.CLAUDE.md's Docker recipe copied the source tree into the container. With ~780MB of local build output that copy silently truncated a source file — gcc reporting "unterminated #if" on a file that compiles everywhere. Now builds from the read-only mount, with the sanitizer recipe added.Verification
Release was not green at first: it aborted 11 tests while Debug passed, which is how the
accept()bug surfaced. New integration tests were repeat-run for flakiness (10–20×); one flake in my own port helper was root-caused to ephemeral-port reuse againstTCPInterface's lack ofSO_REUSEADDRand fixed by retrying port selection, not by retrying an assertion.Closes #60 for the protected/private surface.
https://claude.ai/code/session_011kRj5nodTJrxyoHY7y6R6c