fix(channel): Prevent leaked SocketMap references during reinitializa…

[darion-yaphet]

What problem does this PR solve?

Issue Number: N/A

Problem Summary:

Reinitializing a Channel could leave stale SocketMap references because each successful direct initialization inserted a socket entry, while destruction released only the final derived key. Failed reinitialization could also partially overwrite the active Channel configuration.

What is changed and the side effects?

Changed:

• Preserve the exact SocketMapKey for the active single-server Channel.
• Release the previous socket-map reference only after a new initialization succeeds.
• Build initialization state locally before committing it to Channel.
• Keep the prior Channel state intact when reinitialization fails.
• Add regression tests for repeated direct initialization and failed reinitialization.

Side effects:

• Reinitializing a Channel now correctly replaces direct-server state and releases obsolete socket references.
• Channel::Init() remains non-thread-safe; callers must not reinitialize while RPCs are in flight.
• Performance effects: One small allocation is added per active single-server Channel to retain its exact socket-map key. Initialization is not on the RPC hot path.
• Breaking backward compatibility: None intended. Reinitialization remains supported and now has correct resource lifecycle behavior.

———

Check List:

• Changes compile with the project C++14 configuration.
• Added focused regression tests.
• Ran relevant ChannelTest cases successfully.

[Copilot]

Pull request overview

This PR fixes brpc::Channel reinitialization for direct (single-server) channels by ensuring the exact SocketMapKey used during initialization is preserved and properly released, preventing leaked SocketMap references and avoiding partial state overwrites when reinitialization fails.

Changes:

• Track and remove the precise SocketMapKey for the active single-server channel state to prevent stale SocketMap references after reinit/destruction.
• Rework initialization to build ChannelOptions/protocol function pointers locally and only commit to the Channel on success, keeping prior state intact on failure.
• Add unit tests covering repeated direct initialization and failed reinitialization behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
test/brpc_channel_unittest.cpp	Adds regression tests for socket-map reference release on reinit and state preservation on failed reinit.
src/brpc/channel.h	Introduces internal single-server state storage (unique_ptr) and a reset helper for single-server lifecycle.
src/brpc/channel.cpp	Implements stored SocketMapKey lifecycle management and commits init state only after success to avoid partial overwrites.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/brpc/channel.cpp:497

• Init(ns_url, lb_name, ...) commits _options and protocol function pointers before validating client_host and before lb->Init(). If client_host is invalid or lb->Init() fails, Init() returns -1 but the channel’s options/protocol callbacks have already been overwritten while _scheme/_service_name/_lb remain unchanged. This creates a partially-updated channel configuration after a failed reinit.

Suggested fix: keep the parsed URL pieces and initialized_options local, and only assign _options/callbacks and swap _scheme/_service_name after lb->Init() succeeds (similar to how you defer ResetSingleServer() today).

    _options = initialized_options.options;
    _serialize_request = initialized_options.serialize_request;
    _pack_request = initialized_options.pack_request;
    _get_method_name = initialized_options.get_method_name;
    _preferred_index = initialized_options.preferred_index;

src/brpc/channel.cpp:416

• InitSingle() assigns _options and protocol function pointers before validating server_addr_and_port.port / client_host and before the SocketMap insert+commit block. If any of the subsequent checks fail and the function returns -1, the channel keeps using the previously-initialized socket-map entry but its options are partially overwritten, leaving the channel in an inconsistent state after a failed reinitialization (contrary to the PR description’s “keep prior state intact on failure”).

Consider keeping initialized_options purely local and only committing _options/_serialize_request/etc after all validation succeeds and you’re ready to ResetSingleServer() + swap in the new state (or add a scoped rollback guard to restore the previous members on every early-return).

This issue also appears on line 493 of the same file.

    _options = initialized_options.options;
    _serialize_request = initialized_options.serialize_request;
    _pack_request = initialized_options.pack_request;
    _get_method_name = initialized_options.get_method_name;
    _preferred_index = initialized_options.preferred_index;

test/brpc_channel_unittest.cpp:2353

• This test currently asserts that channel.options().client_host equals the invalid client_host from a failed Init() call. If the intent is to keep the prior channel state intact when reinitialization fails, options should remain unchanged after the failed init.

Capturing the original client_host and asserting it remains the same also avoids locking in inconsistent runtime behavior where options() no longer matches the socket that is actually in use.

        EXPECT_EQ(endpoint, channel._server_address);
        EXPECT_EQ(original_id, channel._server_id);
        EXPECT_EQ(invalid_options.client_host, channel.options().client_host);
        EXPECT_TRUE(channel.SingleServer());

[wwbmmm]

LGTM

[chenBright]

Instead of managing the SocketMap key lifecycle for re-init, I'd suggest simply rejecting re-initialization once Init() has succeeded. A Channel that's initialized exactly once inserts exactly one socket entry and never mutates _options afterward, so the signature recomputed in ~Channel() always matches the insertion key and the reference stays balanced — no extra state needed. Failed inits before the first success can still be retried.

[darion-yaphet]

Instead of managing the SocketMap key lifecycle for re-init, I'd suggest simply rejecting re-initialization once Init() has succeeded. A Channel that's initialized exactly once inserts exactly one socket entry and never mutates _options afterward, so the signature recomputed in ~Channel() always matches the insertion key and the reference stays balanced — no extra state needed. Failed inits before the first success can still be retried.

Thanks for the suggestion! That makes total sense.

Rejecting re-initialization after a successful Init() keeps Channel simple without extra heap allocation or state tracking, while still allowing failed Init() attempts to be retried. I will update the PR to follow this approach.