Repository navigation
Networking: Defer the client peer shutdown out of MafiaNet dispatch #316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -121,13 +121,16 @@ namespace Framework::Networking { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| return {}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _peer->Shutdown(kShutdownBlockDurationMs, 0, MafiaNet::Priority::Immediate); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Called from inside a MafiaNet callback (MafiaHub/Framework#315): Update() shuts the peer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // down once dispatch has unwound. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (_dispatching) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _shutdownPending = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ShutdownPeer(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Logging::GetLogger(FRAMEWORK_INNER_NETWORKING)->debug("Disconnecting from the server..."); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Peer shutdown fires TwoWayAuthentication::Clear(), wiping the registered build token; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // drop the cache so the next Connect()'s Init() re-registers instead of no-op'ing. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _registeredToken.clear(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (_onPlayerDisconnectedCallback) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Locally initiated: there is no inbound packet, so pass null rather than a stale _packet. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _onPlayerDisconnectedCallback(nullptr, DisconnectionReason::GRACEFUL_SHUTDOWN, ""); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -137,8 +140,17 @@ namespace Framework::Networking { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| return {}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void NetworkClient::ShutdownPeer() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _peer->Shutdown(kShutdownBlockDurationMs, 0, MafiaNet::Priority::Immediate); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Peer shutdown fires TwoWayAuthentication::Clear(), wiping the registered build token; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // drop the cache so the next Connect()'s Init() re-registers instead of no-op'ing. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _registeredToken.clear(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void NetworkClient::ResetConnectionState() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (_peer) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // A pending shutdown closes every connection itself, after the plugins have unwound. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (_peer && !_shutdownPending) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| MafiaNet::SystemAddress systems[kMaxConnections]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| unsigned short count = kMaxConnections; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (_peer->GetConnectionList(systems, &count)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -174,7 +186,14 @@ namespace Framework::Networking { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _dispatching = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| NetworkPeer::Update(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _dispatching = false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (_shutdownPending) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _shutdownPending = false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ShutdownPeer(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+189
to
+196
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '65,115p' code/framework/src/networking/network_peer.cpp
sed -n '175,210p' code/framework/src/networking/network_client.cpp
sed -n '20,95p' code/tests/modules/download_disconnect_ut.hRepository: MafiaHub/Framework Length of output: 6687 🏁 Script executed: set -u
printf '%s\n' '--- symbol locations ---'
rg -n -F --glob '*.{cpp,h,hpp}' -- 'NetworkClient::Disconnect' code/framework code/tests || true
rg -n -F --glob '*.{cpp,h,hpp}' -- 'NetworkClient::Update' code/framework code/tests || true
rg -n -F --glob '*.{cpp,h,hpp}' -- 'ShutdownPeer' code/framework code/tests || true
rg -n -F --glob '*.{cpp,h,hpp}' -- '_dispatching' code/framework code/tests || true
rg -n -F --glob '*.{cpp,h,hpp}' -- 'PumpUntil' code/tests || true
printf '%s\n' '--- network client definitions ---'
sed -n '1,240p' code/framework/src/networking/network_client.cpp
printf '%s\n' '--- network client declarations ---'
sed -n '1,240p' code/framework/include/framework/networking/network_client.h 2>/dev/null || true
sed -n '1,240p' code/framework/src/networking/network_client.h 2>/dev/null || true
printf '%s\n' '--- peer caller loop / update entrypoints ---'
rg -n -F --glob '*.{cpp,h,hpp}' -- 'client->Update' code/framework code/tests || true
rg -n -F --glob '*.{cpp,h,hpp}' -- 'Update();' code/framework/src/networking code/tests/modules/download_disconnect_ut.h || true
printf '%s\n' '--- test harness relevant definitions ---'
rg -n -F --glob '*.{cpp,h,hpp}' -- 'PumpUntil' code/tests || true
rg -n -F --glob '*.{cpp,h,hpp}' -- 'Update()' code/tests/modules/download_disconnect_ut.h || trueRepository: MafiaHub/Framework Length of output: 21016 🌐 Web query:
💡 Result: Finalize the pending shutdown when dispatch throws.
Catch the exception after Suggested fix--- "a/code/framework/src/networking/network_client.cpp"
+++ "b/code/framework/src/networking/network_client.cpp"
@@ -186,9 +186,19 @@
return;
}
_dispatching = true;
- NetworkPeer::Update();
+ try {
+ NetworkPeer::Update();
+ }
+ catch (...) {
+ _dispatching = false;
+ if (_shutdownPending) {
+ _shutdownPending = false;
+ ShutdownPeer();
+ }
+ throw;
+ }
_dispatching = false;
if (_shutdownPending) {
_shutdownPending = false;📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bool NetworkClient::HandlePacket(uint8_t packetID, MafiaNet::Packet *packet) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| /* | ||
| * MafiaHub OSS license | ||
| * Copyright (c) 2026, MafiaHub. All rights reserved. | ||
| * | ||
| * This file comes from MafiaHub, hosted at https://github.com/MafiaHub/Framework. | ||
| * See LICENSE file in the source repository for information regarding licensing. | ||
| */ | ||
|
|
||
| #pragma once | ||
|
|
||
| #include "connection_admission_ut.h" | ||
|
|
||
| #include <mafianet/FileList.h> | ||
| #include <mafianet/FileListTransfer.h> | ||
| #include <mafianet/FileListTransferCBInterface.h> | ||
|
|
||
| // MafiaHub/Framework#315: the join refusal disconnects from inside FileListTransfer's completion | ||
| // callback, and the peer shutdown frees the receiver and packet pool that DecodeSetHeader and | ||
| // RakPeer::Receive still hold. | ||
| class DisconnectingDownloadHandler final: public MafiaNet::FileListTransferCBInterface { | ||
| public: | ||
| // Thrown when the shutdown has already freed what the callers hold: unwinding out of | ||
| // RakPeer::Receive is the only way past the freed receiver and packet without touching them. | ||
| struct TornDownUnderCaller {}; | ||
|
|
||
| Framework::Networking::NetworkClient *client = nullptr; | ||
| unsigned short setID = 0; | ||
| int completions = 0; | ||
| int dereferences = 0; | ||
| bool receiverSurvived = false; | ||
|
|
||
| bool OnFile(OnFileStruct *) override { | ||
| return true; | ||
| } | ||
|
|
||
| void OnFileProgress(FileProgressStruct *) override {} | ||
|
|
||
| bool OnDownloadComplete(DownloadCompleteStruct *) override { | ||
| ++completions; | ||
| (void)client->Disconnect(); | ||
| receiverSurvived = client->GetFileListTransfer()->IsHandlerActive(setID); | ||
| if (!receiverSurvived) { | ||
| throw TornDownUnderCaller {}; | ||
| } | ||
| // FileListTransfer then dereferences and deletes the receiver it still holds. | ||
| return false; | ||
| } | ||
|
|
||
| void OnDereference() override { | ||
| ++dereferences; | ||
| } | ||
| }; | ||
|
|
||
| MODULE(download_disconnect, { | ||
| using Rig = ConnectionAdmissionRig; | ||
|
|
||
| IT("keeps the download receiver alive when the completion callback disconnects", { | ||
| // Outlives the rig: a receiver left behind by a failed step still points at it. | ||
| DisconnectingDownloadHandler handler; | ||
| Rig rig; | ||
| EQUALS(rig.StartServer(false), true); | ||
| EQUALS(rig.Connect(Rig::IdentityPayload("Downloader", "")), true); | ||
| EQUALS(rig.PumpUntil([&] { return rig.seen.clientConnected && rig.server->GetPlayerCount() == 1u; }), true); | ||
|
|
||
| handler.client = rig.client.get(); | ||
| handler.setID = rig.client->GetFileListTransfer()->SetupReceive(&handler, false, rig.client->GetPeer()->GetSystemAddressFromIndex(0)); | ||
| EQUALS(handler.setID != static_cast<unsigned short>(-1), true); | ||
|
|
||
| // An empty set: the header alone completes it, through DecodeSetHeader, which is how a | ||
| // download ends when the client already has every file. | ||
| MafiaNet::FileList nothing; | ||
| MafiaNet::FileListTransfer sender; | ||
| sender.Send(¬hing, rig.server->GetPeer(), rig.server->GetPeer()->GetSystemAddressFromIndex(0), handler.setID, MafiaNet::Priority::High, 0); | ||
|
|
||
| bool tornDown = false; | ||
| try { | ||
| rig.PumpUntil([&] { return handler.completions > 0 && rig.seen.clientDisconnected; }); | ||
| } | ||
| catch (const DisconnectingDownloadHandler::TornDownUnderCaller &) { | ||
| tornDown = true; | ||
| } | ||
|
|
||
| EQUALS(handler.completions, 1); | ||
| EQUALS(tornDown, false); | ||
| EQUALS(handler.receiverSurvived, true); | ||
| // Released once, by FileListTransfer, and not again by the shutdown. | ||
| EQUALS(handler.dereferences, 1); | ||
| EQUALS(rig.seen.clientDisconnected, true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: MafiaHub/Framework
Length of output: 11855
🏁 Script executed:
Repository: MafiaHub/Framework
Length of output: 21330
🏁 Script executed:
Repository: MafiaHub/Framework
Length of output: 21279
🏁 Script executed:
Repository: MafiaHub/Framework
Length of output: 20090
Reject reconnect attempts while
_shutdownPendingis true.The disconnected callback itself runs before
ResetConnectionState(), so a directConnect()call from that callback is rejected by the current state check. However, a dispatch callback can callDisconnect(), continue after it returns, and then callConnect(). The new connection can start while_shutdownPendingremains set.Update()then shuts down the peer and leaves the client inCONNECTING.No repository callback documents or uses reconnect during dispatch. Reject this sequence explicitly.
Suggested fix
Utils::Result<void, Error> NetworkClient::Connect(const std::string &host, int32_t port, const std::string &password, const std::string &sessionPayload) { + if (_shutdownPending) { + return Error("Cannot connect: a peer shutdown is pending"); + } if (_state != PeerState::DISCONNECTED) { return Error("Cannot connect: the client is already connected"); }🤖 Prompt for AI Agents