Clean up McpServer connection-callback surface#220
Merged
Conversation
added 2 commits
April 20, 2026 16:16
McpServer inherited from network::ConnectionCallbacks and overrode onEvent / onAboveWriteBufferHighWatermark / onBelowWriteBufferLowWatermark, but nothing ever registered an McpServer instance as a connection's ConnectionCallbacks. Per-TCP-connection events go through the ConnectionLifecycleCallbacks adapter added in onNewConnection, which carries the closing connection's identity. The stdio event path reaches McpServer::onConnectionEvent through ServerProtocolCallbacks, not through ConnectionCallbacks::onEvent. Removing the base and its three overrides deletes dead code and makes the class interface honest about which callback surfaces it actually uses. onConnectionEvent itself stays: it is still the stats-and- forwarding path for stdio transports via ServerProtocolCallbacks. All McpServer/McpClient/StdioEcho/TcpEcho integration tests still pass.
num_connections_ was a private atomic that tracked accept/close on the dispatcher thread but was never read -- no member function exposed it, no test inspected it, and no external caller had a hook to reach it. The real connection count visible to operators has always been server_stats_.connections_active, which getServerStats() already surfaces and which is incremented and decremented on exactly the same code paths. Keeping two parallel counters that only differ in reachability is a maintenance trap: the next time a path changes, one of them drifts. Removing the shadow counter leaves a single source of truth. All McpServer/McpClient/StdioEcho/TcpEcho integration tests still pass.
gophergogo
force-pushed
the
chore/audit-server-connection-event
branch
from
afedafc to
18b1dac
Compare
caleb2h
approved these changes
Apr 20, 2026
caleb2h
pushed a commit
that referenced
this pull request
Apr 20, 2026
McpServer inherited from network::ConnectionCallbacks and overrode onEvent / onAboveWriteBufferHighWatermark / onBelowWriteBufferLowWatermark, but nothing ever registered an McpServer instance as a connection's ConnectionCallbacks. Per-TCP-connection events go through the ConnectionLifecycleCallbacks adapter added in onNewConnection, which carries the closing connection's identity. The stdio event path reaches McpServer::onConnectionEvent through ServerProtocolCallbacks, not through ConnectionCallbacks::onEvent. Removing the base and its three overrides deletes dead code and makes the class interface honest about which callback surfaces it actually uses. onConnectionEvent itself stays: it is still the stats-and- forwarding path for stdio transports via ServerProtocolCallbacks. All McpServer/McpClient/StdioEcho/TcpEcho integration tests still pass.
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
2 participants
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.
Summary
Two small, mechanical follow-ups to PR #219 that trim dead code from the
McpServerconnection-callback surface now that TCP accepts are properly counted inonNewConnection.ConnectionCallbacksinheritance.McpServerinherited fromnetwork::ConnectionCallbacksand overrodeonEvent,onAboveWriteBufferHighWatermark, andonBelowWriteBufferLowWatermark, but nothing ever registered anMcpServerinstance as a connection's callbacks. Per-TCP-connection events go through theConnectionLifecycleCallbacksadapter (which carries the closing connection's identity); the stdio path reachesMcpServer::onConnectionEventthroughServerProtocolCallbacks, not throughConnectionCallbacks::onEvent.onConnectionEventitself stays — it is still the live stats-and-forwarding entry point for stdio transports.num_connections_counter. It was a private atomic bumped on accept and decremented on close with no reader anywhere ininclude/,src/,tests/, orexamples/.server_stats_.connections_activeis the real, publicly-exposed count viagetServerStats()and lives on the same code paths. One source of truth instead of two.Test plan
ctest -R 'McpServer|McpClient|McpInitialize|StdioEcho|TcpEcho'— 12/12 pass