Value-type fixes: thread-safe to_string + optional connected_address (#15)#31
Merged
Merged
Conversation
…threadsafe GUID ToString (#15) Adds modern value-type accessors in a new mafianet/guid_util.h: - std::string to_string(const RakNetGUID&): owns its buffer, thread-safe, built on the existing thread-safe ToString(char*, size_t). - std::optional<SystemAddress> connected_address(RakPeerInterface&, const RakNetGUID&): maps the UNASSIGNED_SYSTEM_ADDRESS sentinel to std::nullopt (facade wrapper, not in RakPeerInterface). Removes the non-thread-safe static-buffer RakNetGUID::ToString(void) member and migrates all ~70 GUID call sites across the library and samples to MafiaNet::to_string(g).c_str(). AddressOrGUID::ToString(bool) now self-contains its rotating buffer instead of delegating to the removed member. SystemAddress/AddressOrGUID ToString calls untouched. Adds GuidUtilTest covering thread-safety (16 threads) and the nullopt-on-sentinel contract.
WalkthroughAdds ChangesGUID Utility API and Migration
DisconnectReasonTest Reliability with Retry Logic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Source/include/mafianet/guid_util.h (1)
24-25: ⚡ Quick winUse project-qualified include paths in this public header.
guid_util.his part of the public API surface; please include project headers using themafianet/...form for consistency with the repository policy.Proposed change
-#include "types.h" -#include "peerinterface.h" +#include "mafianet/types.h" +#include "mafianet/peerinterface.h"As per coding guidelines, "Include paths in C++ code should use the 'mafianet/...' format for public API headers".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/include/mafianet/guid_util.h` around lines 24 - 25, The public header guid_util.h is using relative include paths for types.h and peerinterface.h instead of the project-qualified format. Update both include directives to use the mafianet/ prefix (e.g., change `#include` "types.h" to `#include` "mafianet/types.h" and `#include` "peerinterface.h" to `#include` "mafianet/peerinterface.h") to maintain consistency with the repository's public API header guidelines.Source: Coding guidelines
Samples/Tests/GuidUtilTest.h (1)
17-27: ⚡ Quick winScope
GuidUtilTestunderMafiaNetinstead of exporting a globalusing namespacefrom the header.This header currently injects
MafiaNetsymbols into every includer and keepsGuidUtilTestin the global namespace. If tests are not intentionally exempt from the namespace rule, wrap the type innamespace MafiaNet { ... }and remove the using-directive.♻️ Proposed change
-using namespace MafiaNet; - /// Verifies the modern value-type accessors from "mafianet/guid_util.h" (issue /// `#15`): /// - to_string(guid) returns the same text as the legacy thread-safe /// ToString(char*, size_t) member, handles the UNASSIGNED sentinel, and is /// safe to call concurrently from many threads (no shared static buffer). /// - connected_address(peer, guid) maps the UNASSIGNED_SYSTEM_ADDRESS "none" /// sentinel to std::nullopt and otherwise forwards the legacy lookup. - -class GuidUtilTest : public TestInterface +namespace MafiaNet { +class GuidUtilTest : public TestInterface { public: GuidUtilTest(void); ~GuidUtilTest(void); int RunTest(DataStructures::List<RakString> params, bool isVerbose, bool noPauses); RakString GetTestName(); RakString ErrorCodeToString(int errorCode); void DestroyPeers(); private: DataStructures::List<RakPeerInterface *> destroyList; }; +} // namespace MafiaNetAs per coding guidelines, "Use the MafiaNet namespace (or MNet macro shorthand) exclusively throughout the library for all code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Samples/Tests/GuidUtilTest.h` around lines 17 - 27, The GuidUtilTest header currently uses a global `using namespace MafiaNet;` directive which pollutes the global namespace for all includers. Instead, remove the `using namespace MafiaNet;` statement and wrap the GuidUtilTest class definition and its associated documentation comment within an explicit `namespace MafiaNet { ... }` block. This keeps the symbols scoped properly according to the coding guidelines that require all code to use the MafiaNet namespace exclusively.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/src/FullyConnectedMesh2.cpp`:
- Line 365: The printf statement that logs the popping participant is using dot
notation to access the rakNetGuid member of a pointer element from the
fcm2ParticipantList array. Since fcm2ParticipantList contains pointers, the
member access operator must be changed from the dot operator (.) to the arrow
operator (->) when accessing rakNetGuid on the pointer element at index
fcm2ParticipantList.Size()-1.
---
Nitpick comments:
In `@Samples/Tests/GuidUtilTest.h`:
- Around line 17-27: The GuidUtilTest header currently uses a global `using
namespace MafiaNet;` directive which pollutes the global namespace for all
includers. Instead, remove the `using namespace MafiaNet;` statement and wrap
the GuidUtilTest class definition and its associated documentation comment
within an explicit `namespace MafiaNet { ... }` block. This keeps the symbols
scoped properly according to the coding guidelines that require all code to use
the MafiaNet namespace exclusively.
In `@Source/include/mafianet/guid_util.h`:
- Around line 24-25: The public header guid_util.h is using relative include
paths for types.h and peerinterface.h instead of the project-qualified format.
Update both include directives to use the mafianet/ prefix (e.g., change
`#include` "types.h" to `#include` "mafianet/types.h" and `#include` "peerinterface.h"
to `#include` "mafianet/peerinterface.h") to maintain consistency with the
repository's public API header guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc4c6c40-2813-4f2a-af32-09c1461526e4
📒 Files selected for processing (28)
Samples/ChatExample/Client/Chat Example Client.cppSamples/ChatExample/Server/Chat Example Server.cppSamples/ComprehensivePCGame/ComprehensivePCGame.cppSamples/FCMHost/FCM2HostTest.cppSamples/FCMHostSimultaneous/FCM2HostSimultaneousTest.cppSamples/FCMVerifiedJoinSimultaneous/FCM2VerifiedJoinSimultaneousTest.cppSamples/LANServerDiscovery/LANServerDiscovery.cppSamples/NATCompleteClient/main.cppSamples/OfflineMessagesTest/OfflineMessagesTest.cppSamples/RakVoice/main.cppSamples/ReadyEvent/ReadyEventSample.cppSamples/ReplicaManager3/main.cppSamples/ReplicatedLogin/ReplicatedLogin.cppSamples/Router2/Router2Sample.cppSamples/SteamLobby/main.cppSamples/TeamManager/TeamManagerTest.cppSamples/Tests/GuidUtilTest.cppSamples/Tests/GuidUtilTest.hSamples/Tests/IncludeAllTests.hSamples/Tests/OfflineMessagesConvertTest.cppSamples/Tests/Tests.cppSource/CMakeLists.txtSource/include/mafianet/guid_util.hSource/include/mafianet/mafianet.hSource/include/mafianet/types.hSource/src/FullyConnectedMesh2.cppSource/src/RakNetTypes.cppSource/src/guid_util.cpp
…h bounded retry Delivering ID_DISCONNECTION_NOTIFICATION to Receive() is a reliable ACK round-trip whose completion depends on both peers' internal network threads being scheduled. On a CPU-starved CI runner running the whole suite in one process, a single connect->close->wait cycle can intermittently miss its window, failing Case 1 with 'client never received ID_DISCONNECTION_NOTIFICATION'. Wrap each case's connect->close->receive in a bounded retry (3 attempts) with a ResetConnection helper that silently tears down both sides between attempts so the fresh reconnect can't race an 'already connected' rejection. Only the transient 'never received' miss is retried; wrong payload failures fail hard immediately, so a real regression in the reason-payload feature is still caught. Verified the retry/reconnect path recovers via temporary fault injection (forced first two attempts to miss -> third reconnects and delivers, content checks still pass), plus 0 failures across 150 in-process runs and 60 single-process runs under heavy CPU oversubscription.
fcm2ParticipantList is a List<FCM2Participant*>, so the popped-participant debug printf must access rakNetGuid via -> not . — the dot form does not compile when DEBUG_FCM2 is defined. The block is excluded from normal builds, so CI never caught it.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Samples/Tests/DisconnectReasonTest.cpp (1)
146-163: 💤 Low valueConsider checking
Read(gotCode)return value for clearer error classification.The
Read(gotCode)on line 150 doesn't check its return value. While the length check at line 139 ensures payload data exists, checking the return would allow reporting a deserialization failure (deserialErr) rather than a potentially misleading "code did not round-trip" (codeErr) if parsing goes wrong.♻️ Suggested improvement
- readBack.Read(gotCode); - bool textOk = gotText.Deserialize(&readBack); + bool codeOk = readBack.Read(gotCode); + bool textOk = codeOk && gotText.Deserialize(&readBack); client->DeallocatePacket(note); ResetConnection(server, client, clientGuid, serverPort); // Content mismatches are hard failures: a real regression in the reason // payload would fail these identically on every attempt, so they are never // retried away. - if (!textOk) + if (!codeOk || !textOk) return deserialErr;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Samples/Tests/DisconnectReasonTest.cpp` around lines 146 - 163, The Read method call on gotCode does not check its return value, which means if deserialization fails, the code will incorrectly classify the error as a code mismatch (codeErr) rather than a deserialization failure (deserialErr). Add a check for the return value of Read(gotCode) immediately after the call, similar to how the Deserialize method is checked for gotText, and return deserialErr if the Read operation fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Samples/Tests/DisconnectReasonTest.cpp`:
- Around line 146-163: The Read method call on gotCode does not check its return
value, which means if deserialization fails, the code will incorrectly classify
the error as a code mismatch (codeErr) rather than a deserialization failure
(deserialErr). Add a check for the return value of Read(gotCode) immediately
after the call, similar to how the Deserialize method is checked for gotText,
and return deserialErr if the Read operation fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9329f367-fbbc-439e-9141-efe3a08792f0
📒 Files selected for processing (2)
Samples/Tests/DisconnectReasonTest.cppSource/src/FullyConnectedMesh2.cpp
✅ Files skipped from review due to trivial changes (1)
- Source/src/FullyConnectedMesh2.cpp
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.
Closes #15.
Summary
Adds modern, additive value-type accessors and moves the legacy non-thread-safe GUID stringifier into them (per maintainer direction, overriding the issue's "additive only" framing).
New —
mafianet/guid_util.hstd::string to_string(const RakNetGUID&)— allocates, owns its buffer, thread-safe; built on the existing thread-safeToString(char*, size_t).std::optional<SystemAddress> connected_address(RakPeerInterface&, const RakNetGUID&)— maps theUNASSIGNED_SYSTEM_ADDRESS"none" sentinel tostd::nullopt. Thin facade wrapper, not added toRakPeerInterface.Source/CMakeLists.txtand themafianet.humbrella header.Moved/removed
RakNetGUID::ToString(void)member + impl. Its role now lives into_string.AddressOrGUID::ToString(bool)(which delegated to the removed member) now self-contains its rotating buffer, preserving itsconst char*API/behavior.MafiaNet::to_string(g).c_str(). AllSystemAddress/AddressOrGUIDToStringcalls are untouched. Includes 3 samples excluded from the default build (SteamLobby,RakVoice,ReplicatedLogin), migrated by hand.Tests —
GuidUtilTest(registered, passing)to_stringmatches the legacy member output and the UNASSIGNED label.connected_addressreturnsnulloptfor the sentinel and an unknown GUID, and mirrorsGetSystemAddressFromGuidfor the engaged case.Acceptance criteria
to_string(guid)is safe to call concurrently (no shared buffer).optionalaccessor returnsstd::nulloptwhere the legacy API returns the sentinel.Not included
std::string_viewoverloads forchar*+lenpairs) — explicitly optional and outside the acceptance criteria; deferred.Verification
./build/Samples/Tests/Tests GuidUtilTest→ pass;SystemAddressAndGuidTest,PeerGuidTest→ pass.Summary by CodeRabbit
New Features
MafiaNet::to_string())MafiaNet::connected_address())Refactor
RakNetGUID::ToString()APITests
GuidUtilTest(including correctness + concurrency validation)DisconnectReasonTestrobustness with bounded retries and extended timeouts