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

docs/cn/client.md

@@ -7,6 +7,7 @@ Echo的[client端代码](https://github.com/apache/brpc/blob/master/example/echo
 # 事实速查
 
 - Channel.Init()是线程不安全的。
+- 一个Channel只能成功初始化一次。Init()失败后可以重试。
 - Channel.CallMethod()是线程安全的，一个Channel可以被所有线程同时使用。
 - Channel可以分配在栈上。
 - Channel在发送异步请求后可以析构。
@@ -32,6 +33,8 @@ channel.Init(..., &options);
 ```
 注意Channel不会修改options，Init结束后不会再访问options。所以options一般就像上面代码中那样放栈上。Channel.options()可以获得channel在使用的所有选项。
 
+Init失败后可以重试；一旦成功，Channel的目标和选项即固定，之后的所有Init调用都会返回-1。需要使用不同的目标或配置时，请新建一个Channel。
+
 Init函数分为连接一台服务器和连接服务集群。
 
 # 连接一台服务器

docs/en/client.md

@@ -7,6 +7,8 @@
 # Quick facts
 
 - Channel.Init() is not thread-safe.
+- A Channel can be initialized successfully only once. Failed Init() calls may
+  be retried.
 - Channel.CallMethod() is thread-safe and a Channel can be used by multiple threads simultaneously.
 - Channel can be put on stack.
 - Channel can be destructed just after sending asynchronous request.
@@ -32,6 +34,10 @@ channel.Init(..., &options);
 ```
 Note that Channel neither modifies `options` nor accesses `options` after completion of Init(), thus options can be put on stack safely as in above code. Channel.options() gets options being used by the Channel.
 
+Init() may be retried after a failure. Once it succeeds, the Channel's target
+and options are fixed and every later Init() call returns -1. Create a new
+Channel to use a different target or configuration.
+
 Init() can connect one server or a cluster(multiple servers).
 
 # Connect to a server

src/brpc/channel.cpp

@@ -254,6 +254,10 @@ int Channel::InitChannelOptions(const ChannelOptions* options) {
 
 int Channel::Init(const char* server_addr_and_port,
                   const ChannelOptions* options) {
+    if (_server_id != INVALID_SOCKET_ID || _lb != NULL) {
+        LOG(ERROR) << "Channel=" << this << " has already been initialized";
+        return -1;
+    }
     GlobalInitializeOrDie();
     butil::EndPoint point;
     const AdaptiveProtocolType& ptype = (options ? options->protocol : _options.protocol);
@@ -287,6 +291,10 @@ int Channel::Init(const char* server_addr_and_port,
 
 int Channel::Init(const char* server_addr, int port,
                   const ChannelOptions* options) {
+    if (_server_id != INVALID_SOCKET_ID || _lb != NULL) {
+        LOG(ERROR) << "Channel=" << this << " has already been initialized";
+        return -1;
+    }
     GlobalInitializeOrDie();
     butil::EndPoint point;
     const AdaptiveProtocolType& ptype = (options ? options->protocol : _options.protocol);
@@ -358,6 +366,10 @@ int Channel::InitSingle(const butil::EndPoint& server_addr_and_port,
                         const char* raw_server_address,
                         const ChannelOptions* options,
                         int raw_port) {
+    if (_server_id != INVALID_SOCKET_ID || _lb != NULL) {
+        LOG(ERROR) << "Channel=" << this << " has already been initialized";
+        return -1;
+    }
     GlobalInitializeOrDie();
     if (InitChannelOptions(options) != 0) {
         return -1;
@@ -410,6 +422,10 @@ int Channel::Init(const char* ns_url,
         // Treat ns_url as server_addr_and_port
         return Init(ns_url, options);
     }
+    if (_server_id != INVALID_SOCKET_ID || _lb != NULL) {
+        LOG(ERROR) << "Channel=" << this << " has already been initialized";
+        return -1;
+    }
     GlobalInitializeOrDie();
     if (InitChannelOptions(options) != 0) {
         return -1;

src/brpc/channel.h

@@ -184,6 +184,9 @@ friend class SelectiveChannel;
 
     DISALLOW_COPY_AND_ASSIGN(Channel);
 
+    // Init() may be retried after failure, but a successful initialization is
+    // final: subsequent calls return -1.
+
     // Connect this channel to a single server whose address is given by the
     // first parameter. Use default options if `options' is NULL.
     int Init(butil::EndPoint server_addr_and_port, const ChannelOptions* options);

test/brpc_channel_unittest.cpp

@@ -2316,6 +2316,55 @@ TEST_F(ChannelTest, init_as_single_server) {
     }
 }
 
+TEST_F(ChannelTest, reject_reinitialization_after_successful_init) {
+    butil::EndPoint first_endpoint;
+    butil::EndPoint second_endpoint;
+    ASSERT_EQ(0, str2endpoint("127.0.0.1:59347", &first_endpoint));
+    ASSERT_EQ(0, str2endpoint("127.0.0.1:59348", &second_endpoint));
+    const brpc::SocketMapKey first_key(first_endpoint);
+    const brpc::SocketMapKey second_key(second_endpoint);
+
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init(first_endpoint, NULL));
+        brpc::SocketId id;
+        ASSERT_EQ(0, brpc::SocketMapFind(first_key, &id));
+        ASSERT_EQ(channel._server_id, id);
+        ASSERT_EQ(-1, channel.Init(first_endpoint, NULL));
+        ASSERT_EQ(-1, channel.Init(second_endpoint, NULL));
+        ASSERT_EQ(-1, channel.Init("unknown://unknown", "rr", NULL));
+    }
+
+    brpc::SocketId id;
+    EXPECT_NE(0, brpc::SocketMapFind(first_key, &id));
+    EXPECT_NE(0, brpc::SocketMapFind(second_key, &id));
+}
+
+TEST_F(ChannelTest, retry_init_after_failed_init) {
+    butil::EndPoint endpoint;
+    ASSERT_EQ(0, str2endpoint("127.0.0.1:59349", &endpoint));
+    const brpc::SocketMapKey key(endpoint);
+
+    {
+        brpc::Channel channel;
+        brpc::ChannelOptions invalid_options;
+        invalid_options.client_host = "not a valid client host";
+        ASSERT_EQ(-1, channel.Init(endpoint, &invalid_options));
+        EXPECT_EQ(brpc::INVALID_SOCKET_ID, channel._server_id);
+
+        brpc::ChannelOptions valid_options;
+        ASSERT_EQ(0, channel.Init(endpoint, &valid_options));
+        EXPECT_NE(brpc::INVALID_SOCKET_ID, channel._server_id);
+        EXPECT_EQ(endpoint, channel._server_address);
+        brpc::SocketId id;
+        ASSERT_EQ(0, brpc::SocketMapFind(key, &id));
+        ASSERT_EQ(channel._server_id, id);
+    }
+
+    brpc::SocketId id;
+    EXPECT_NE(0, brpc::SocketMapFind(key, &id));
+}
+
 TEST_F(ChannelTest, init_using_unknown_naming_service) {
     brpc::Channel channel;
     ASSERT_EQ(-1, channel.Init("unknown://unknown", "unknown", NULL));
@@ -2391,73 +2440,138 @@ TEST_F(ChannelTest, parse_hostname) {
     brpc::ChannelOptions opt;
     opt.succeed_without_server = false;
     opt.protocol = brpc::PROTOCOL_HTTP;
-    brpc::Channel channel;
 
-    ASSERT_EQ(-1, channel.Init("", 8888, &opt));
-    ASSERT_EQ("", channel._service_name);
-    ASSERT_EQ(-1, channel.Init("", &opt));
-    ASSERT_EQ("", channel._service_name);
-
-    ASSERT_EQ(0, channel.Init("http://127.0.0.1", 8888, &opt));
-    ASSERT_EQ("127.0.0.1:8888", channel._service_name);
-    ASSERT_EQ(0, channel.Init("http://127.0.0.1:8888", &opt));
-    ASSERT_EQ("127.0.0.1:8888", channel._service_name);
-
-    ASSERT_EQ(0, channel.Init("localhost", 8888, &opt));
-    ASSERT_EQ("localhost:8888", channel._service_name);
-    ASSERT_EQ(0, channel.Init("localhost:8888", &opt));
-    ASSERT_EQ("localhost:8888", channel._service_name);
-
-    ASSERT_EQ(0, channel.Init("http://www.baidu.com", &opt));
-    ASSERT_EQ("www.baidu.com", channel._service_name);
-    ASSERT_EQ(0, channel.Init("http://www.baidu.com:80", &opt));
-    ASSERT_EQ("www.baidu.com:80", channel._service_name);
-    ASSERT_EQ(0, channel.Init("http://www.baidu.com", 80, &opt));
-    ASSERT_EQ("www.baidu.com:80", channel._service_name);
-    ASSERT_EQ(0, channel.Init("http://www.baidu.com:8888", &opt));
-    ASSERT_EQ("www.baidu.com:8888", channel._service_name);
-    ASSERT_EQ(0, channel.Init("http://www.baidu.com", 8888, &opt));
-    ASSERT_EQ("www.baidu.com:8888", channel._service_name);
-    ASSERT_EQ(0, channel.Init("http://www.baidu.com", "rr", &opt));
-    ASSERT_EQ("www.baidu.com", channel._service_name);
-    ASSERT_EQ(0, channel.Init("http://www.baidu.com:80", "rr", &opt));
-    ASSERT_EQ("www.baidu.com:80", channel._service_name);
-    ASSERT_EQ(0, channel.Init("http://www.baidu.com:8888", "rr", &opt));
-    ASSERT_EQ("www.baidu.com:8888", channel._service_name);
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(-1, channel.Init("", 8888, &opt));
+        ASSERT_EQ("", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(-1, channel.Init("", &opt));
+        ASSERT_EQ("", channel._service_name);
+    }
+
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("http://127.0.0.1", 8888, &opt));
+        ASSERT_EQ("127.0.0.1:8888", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("http://127.0.0.1:8888", &opt));
+        ASSERT_EQ("127.0.0.1:8888", channel._service_name);
+    }
+
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("localhost", 8888, &opt));
+        ASSERT_EQ("localhost:8888", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("localhost:8888", &opt));
+        ASSERT_EQ("localhost:8888", channel._service_name);
+    }
+
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("http://www.baidu.com", &opt));
+        ASSERT_EQ("www.baidu.com", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("http://www.baidu.com:80", &opt));
+        ASSERT_EQ("www.baidu.com:80", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("http://www.baidu.com", 80, &opt));
+        ASSERT_EQ("www.baidu.com:80", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("http://www.baidu.com:8888", &opt));
+        ASSERT_EQ("www.baidu.com:8888", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("http://www.baidu.com", 8888, &opt));
+        ASSERT_EQ("www.baidu.com:8888", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("http://www.baidu.com", "rr", &opt));
+        ASSERT_EQ("www.baidu.com", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("http://www.baidu.com:80", "rr", &opt));
+        ASSERT_EQ("www.baidu.com:80", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("http://www.baidu.com:8888", "rr", &opt));
+        ASSERT_EQ("www.baidu.com:8888", channel._service_name);
+    }
 
     opt.mutable_ssl_options()->verify.verify_mode = brpc::VerifyMode::VERIFY_PEER;
     opt.mutable_ssl_options()->verify.verify_depth = 1;
     opt.mutable_ssl_options()->verify.ca_file_path = "cert1.crt";
-    ASSERT_EQ(0, channel.Init("https://www.baidu.com", &opt));
-    ASSERT_EQ("www.baidu.com", channel._service_name);
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("https://www.baidu.com", &opt));
+        ASSERT_EQ("www.baidu.com", channel._service_name);
 #if defined(USE_MESALINK) || \
     (!defined(OPENSSL_IS_BORINGSSL) && OPENSSL_VERSION_NUMBER < 0x10002000L)
-    ASSERT_TRUE(channel._options.ssl_options().verify.expected_peer_name.empty());
+        ASSERT_TRUE(channel._options.ssl_options().verify.expected_peer_name.empty());
 #else
-    ASSERT_EQ("www.baidu.com",
-              channel._options.ssl_options().verify.expected_peer_name);
+        ASSERT_EQ("www.baidu.com",
+                  channel._options.ssl_options().verify.expected_peer_name);
 #endif
-    ASSERT_EQ(0, channel.Init("https://www.baidu.com:443", &opt));
-    ASSERT_EQ("www.baidu.com:443", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("https://www.baidu.com:443", &opt));
+        ASSERT_EQ("www.baidu.com:443", channel._service_name);
 #if defined(USE_MESALINK) || \
     (!defined(OPENSSL_IS_BORINGSSL) && OPENSSL_VERSION_NUMBER < 0x10002000L)
-    ASSERT_TRUE(channel._options.ssl_options().verify.expected_peer_name.empty());
+        ASSERT_TRUE(channel._options.ssl_options().verify.expected_peer_name.empty());
 #else
-    ASSERT_EQ("www.baidu.com",
-              channel._options.ssl_options().verify.expected_peer_name);
+        ASSERT_EQ("www.baidu.com",
+                  channel._options.ssl_options().verify.expected_peer_name);
 #endif
-    ASSERT_EQ(0, channel.Init("https://www.baidu.com", 443, &opt));
-    ASSERT_EQ("www.baidu.com:443", channel._service_name);
-    ASSERT_EQ(0, channel.Init("https://www.baidu.com:1443", &opt));
-    ASSERT_EQ("www.baidu.com:1443", channel._service_name);
-    ASSERT_EQ(0, channel.Init("https://www.baidu.com", 1443, &opt));
-    ASSERT_EQ("www.baidu.com:1443", channel._service_name);
-    ASSERT_EQ(0, channel.Init("https://www.baidu.com", "rr", &opt));
-    ASSERT_EQ("www.baidu.com", channel._service_name);
-    ASSERT_EQ(0, channel.Init("https://www.baidu.com:443", "rr", &opt));
-    ASSERT_EQ("www.baidu.com:443", channel._service_name);
-    ASSERT_EQ(0, channel.Init("https://www.baidu.com:1443", "rr", &opt));
-    ASSERT_EQ("www.baidu.com:1443", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("https://www.baidu.com", 443, &opt));
+        ASSERT_EQ("www.baidu.com:443", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("https://www.baidu.com:1443", &opt));
+        ASSERT_EQ("www.baidu.com:1443", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("https://www.baidu.com", 1443, &opt));
+        ASSERT_EQ("www.baidu.com:1443", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("https://www.baidu.com", "rr", &opt));
+        ASSERT_EQ("www.baidu.com", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("https://www.baidu.com:443", "rr", &opt));
+        ASSERT_EQ("www.baidu.com:443", channel._service_name);
+    }
+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init("https://www.baidu.com:1443", "rr", &opt));
+        ASSERT_EQ("www.baidu.com:1443", channel._service_name);
+    }
 
     const char *address_list[] =  {
         "10.127.0.1:1234",