feat: carry a disconnect reason in ID_DISCONNECTION_NOTIFICATION / CloseConnection#24
Conversation
…ON (#23) Add an optional reasonData BitStream to CloseConnection() so a graceful disconnect can tell the remote peer *why* it was dropped. The payload is appended right after the 1-byte ID_DISCONNECTION_NOTIFICATION ID, so the receiver reads it from packet->data+1 (length packet->length-1). Appending bytes after the ID is wire-backward-compatible. On the receive side the raw reliability-layer buffer is freed before the user-facing notification is synthesized, so the reason bytes are stashed on the RemoteSystemStruct when the notification arrives and copied into the delivered packet once outstanding ACKs are flushed. The stash buffer is freed/cleared on delivery, on slot reuse, and on every teardown path via ClearDisconnectReason(). Only graceful disconnects carry a reason; locally-synthesized notifications (ID_CONNECTION_LOST, timeout/dead-connection) stay payload-less, so consumers must tolerate a zero-length body. Adds DisconnectReasonTest covering both the reason-carrying and reasonless (bare 1-byte) cases over loopback. Verified clean under AddressSanitizer. Closes #23
|
Warning Review limit reached
More reviews will be available in 20 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR implements optional disconnect-reason payloads on ChangesDisconnect Reason Feature
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/src/RakPeer.cpp (1)
1640-1673:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDon't coerce unresolved closes onto slot 0.
If
CloseConnection()is called beforeStartup(), afterShutdown(), or for a target that no longer resolves,GetIndexFromSystemAddress()returns-1, Line 1660 rewrites that to0, and Line 1669 immediately readsremoteSystemList[0]. That is a crash whenremoteSystemList == 0/maximumNumberOfPeers == 0, and when the array does exist it can still pick the wrong peer's socket andsystemIndex.Proposed fix
void RakPeer::CloseConnection( const AddressOrGUID target, bool sendDisconnectionNotification, unsigned char orderingChannel, PacketPriority disconnectionNotificationPriority, const MafiaNet::BitStream *reasonData ) { + if (remoteSystemList == 0 || endThreads == true) + return; + const SystemAddress address = (target.systemAddress == UNASSIGNED_SYSTEM_ADDRESS) ? GetSystemAddressFromGuid(target.rakNetGuid) : target.systemAddress; - int remoteSystemListIndex = GetIndexFromSystemAddress(address); - // fallback to index 0 (i.e. preserve old behavior for now) - // `#med` - review this whole design here - if (remoteSystemListIndex == -1) { - remoteSystemListIndex = 0; - } + int remoteSystemListIndex = + target.rakNetGuid != UNASSIGNED_RAKNET_GUID + ? GetIndexFromGuid(target.rakNetGuid) + : GetIndexFromSystemAddress(address); + if (remoteSystemListIndex == -1) + return; RakNetSocket2 *closeSocket = remoteSystemList[remoteSystemListIndex].rakNetSocket;🤖 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/src/RakPeer.cpp` around lines 1640 - 1673, CloseConnection currently coerces a -1 result from GetIndexFromSystemAddress into 0 and then unconditionally reads remoteSystemList[remoteSystemListIndex], which can crash or target the wrong peer; change the logic to NOT rewrite -1 to 0 and to only dereference remoteSystemList when remoteSystemListIndex != -1: if GetIndexFromSystemAddress returns -1, attempt to select a closeSocket from socketList (if any) but do not read remoteSystemList[0] or otherwise assume a valid systemIndex; if no socket is available, return early; when remoteSystemListIndex is valid use remoteSystemList[remoteSystemListIndex].rakNetSocket as before and then call CloseConnectionInternal2 with the chosen socket.
🤖 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.
Outside diff comments:
In `@Source/src/RakPeer.cpp`:
- Around line 1640-1673: CloseConnection currently coerces a -1 result from
GetIndexFromSystemAddress into 0 and then unconditionally reads
remoteSystemList[remoteSystemListIndex], which can crash or target the wrong
peer; change the logic to NOT rewrite -1 to 0 and to only dereference
remoteSystemList when remoteSystemListIndex != -1: if GetIndexFromSystemAddress
returns -1, attempt to select a closeSocket from socketList (if any) but do not
read remoteSystemList[0] or otherwise assume a valid systemIndex; if no socket
is available, return early; when remoteSystemListIndex is valid use
remoteSystemList[remoteSystemListIndex].rakNetSocket as before and then call
CloseConnectionInternal2 with the chosen socket.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9466dab-eb27-4dad-8b32-122f1ac4e05f
📒 Files selected for processing (7)
Samples/Tests/DisconnectReasonTest.cppSamples/Tests/DisconnectReasonTest.hSamples/Tests/IncludeAllTests.hSamples/Tests/Tests.cppSource/include/mafianet/peer.hSource/include/mafianet/peerinterface.hSource/src/RakPeer.cpp
Add a 'Disconnect with a reason' section to the connecting guide showing how to attach a reason BitStream to CloseConnection() and read it back from the notification packet, plus a cross-referencing note in the disconnect-debugging handler example. The CloseConnection() API reference picks up the reasonData parameter from the updated header doc comment.
Add Case 3 to DisconnectReasonTest exercising the GetNumberOfBytesUsed() > 0 guard in NotifyAndFlagForShutdown: a non-null but empty reason BitStream must still yield a bare 1-byte ID_DISCONNECTION_NOTIFICATION, distinct from the nullptr path.
GetIndexFromSystemAddress returns -1 when the target isn't in the remote system list. The old code rewrote that to 0 and then read remoteSystemList[0].rakNetSocket unconditionally, which targets an unrelated peer's slot (or crashes if the list is unallocated). Resolve the socket without assuming a valid index: use the slot's socket only when the index is valid, otherwise fall back to the primary socket and bail out if none exists. Guard the downstream systemIndex assignment so it no longer casts -1.
1 participant
Closes #23.
Summary
Adds an optional
reasonDatapayload toRakPeerInterface::CloseConnection()so a graceful disconnect can tell the remote peer why it was dropped, instead of racing a separate application RPC against the disconnect.The reason rides right after the 1-byte
ID_DISCONNECTION_NOTIFICATIONID; the receiver reads it frompacket->data+1(lengthpacket->length-1), exactly like any other message body. It's a flexible superset — send a single enum byte if you want minimal overhead, or an enum + custom string for"Kicked: <reason>":The non-obvious part
The issue assumed the payload would flow straight through, but it would not have: when the notification arrives over the network, the raw reliability-layer buffer is freed, and the user-facing packet is re-synthesized fresh as a bare 1-byte message after ACKs flush. So the reason bytes are stashed on
RemoteSystemStructon receipt and copied into the synthesized packet. The buffer is freed via a singleClearDisconnectReason()helper on delivery's teardown, on slot reuse, and at shutdown (plus a one-time zero-init at allocation).The notification is sent
RELIABLE_ORDERED(unchanged), so the reason is ordered relative to prior messages — eliminating the race the downstream workaround had.Scope / compatibility
ID_CONNECTION_LOST, timeout/dead-connection) stay payload-less, so consumers must tolerate a zero-length body.data[0]are unaffected; an old sender yields a bare 1-byte notification.RakPeeris the onlyRakPeerInterfaceimplementor, so the signature change touches nothing else.Tests
Adds
DisconnectReasonTest(registered in the suite) over loopback:RakString; asserts the notification carries the payload and round-trips exactly.nullptr); asserts a bare 1-byte notification (packet->length == 1).Verification:
PeerConnectDisconnectTest,PeerConnectDisconnectWithCancelPendingTest,DroppedConnectionConvertTest,EightPeerTest,ManyClientsOneServerBlockingTest) all pass.Summary by CodeRabbit
New Features
Tests