Skip to content

Commit c31463a

Browse files
gophergogocaleb2h
authored andcommitted
Defer closed-connection destruction via Dispatcher::deferredDelete (#212)
onConnectionEvent(RemoteClose|LocalClose) runs inside the connection's own callback loop. Destroying the connection synchronously triggered a use-after-free once the loop resumed. The previous workaround wrapped the unique_ptr in a shared_ptr and posted it — now replaced with the dispatcher's deferred-delete queue, which Connection already supports via DeferredDeletable. Also adds a dispatcher-thread assert at the top of onConnectionEvent so any off-thread synthesis of connection events trips immediately instead of silently corrupting state.
1 parent e9268d7 commit c31463a

1 file changed

Lines changed: 14 additions & 12 deletions

File tree

src/mcp_connection_manager.cc

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#include "mcp/mcp_connection_manager.h"
22

3+
#include <cassert>
34
#include <cstring>
45
#include <iostream>
56
#include <sstream>
@@ -790,6 +791,11 @@ void McpConnectionManager::onResponse(const jsonrpc::Response& response) {
790791
}
791792

792793
void McpConnectionManager::onConnectionEvent(network::ConnectionEvent event) {
794+
// Connection events are raised from the connection's own callback loop,
795+
// which runs on the dispatcher thread. Anything else would mean a caller
796+
// synthesised the event off-thread — a bug, not a transport event.
797+
assert(dispatcher_.isThreadSafe() && "onConnectionEvent off dispatcher");
798+
793799
const char* event_name = "unknown";
794800
switch (event) {
795801
case network::ConnectionEvent::Connected:
@@ -859,20 +865,16 @@ void McpConnectionManager::onConnectionEvent(network::ConnectionEvent event) {
859865
return;
860866
}
861867

862-
// Connection closed - clean up state
863-
connected_ = false;
864-
// CRITICAL FIX: Defer connection destruction
868+
// Connection closed - clean up state.
865869
// We are being called from within the connection's callback loop
866-
// (raiseConnectionEvent). Destroying the connection here would cause
867-
// use-after-free when the callback loop continues to iterate. Use post() to
868-
// defer destruction until after current callback.
870+
// (raiseConnectionEvent). Destroying the connection synchronously here
871+
// would be a use-after-free once the callback loop resumes. Hand it to
872+
// the dispatcher's deferred-delete queue instead — Connection derives
873+
// from DeferredDeletable, so the dispatcher owns teardown on the next
874+
// event-loop iteration, after all in-flight callbacks unwind.
875+
connected_ = false;
869876
if (active_connection_) {
870-
auto conn_to_delete = std::make_shared<network::ConnectionPtr>(
871-
std::move(active_connection_));
872-
dispatcher_.post([conn_to_delete]() {
873-
// Connection is destroyed when lambda and shared_ptr go out of scope
874-
conn_to_delete->reset();
875-
});
877+
dispatcher_.deferredDelete(std::move(active_connection_));
876878
}
877879
}
878880

0 commit comments

Comments
 (0)