chenBright commented on code in PR #3590:
URL: https://github.com/apache/brpc/pull/3590#discussion_r4236646169
##########
test/brpc_server_unittest.cpp:
##########
@@ -307,6 +325,94 @@ TEST_F(ServerTest, sanity) {
ASSERT_EQ(0, server.Join());
}
+TEST_F(ServerTest, failed_start_cleans_reserved_thread_local_data) {
+ CountingTLSDataFactory factory;
+ brpc::ServerOptions options;
+ options.thread_local_data_factory = &factory;
+ options.reserved_thread_local_data = 4;
+ brpc::Server listener;
+ ASSERT_EQ(0, listener.Start("127.0.0.1:0", nullptr));
+ brpc::Server server;
+
+ for (size_t attempt = 1; attempt <= 2; ++attempt) {
+ ASSERT_EQ(-1, server.Start(listener.listen_address(), &options));
+ EXPECT_TRUE(server._keytable_pool == nullptr);
+ EXPECT_EQ(INVALID_BTHREAD_KEY, server._tl_options.tls_key);
+ EXPECT_EQ(attempt * options.reserved_thread_local_data,
+ factory.created.load(butil::memory_order_relaxed));
+ EXPECT_EQ(factory.created.load(butil::memory_order_relaxed),
+ factory.destroyed.load(butil::memory_order_relaxed));
+ }
+
+ // A successful retry must create a fresh key and pool.
+ ASSERT_EQ(0, server.Start("127.0.0.1:0", &options));
+ EXPECT_TRUE(server._keytable_pool != nullptr);
+ EXPECT_NE(INVALID_BTHREAD_KEY, server._tl_options.tls_key);
+ EXPECT_EQ(3 * options.reserved_thread_local_data,
+ factory.created.load(butil::memory_order_relaxed));
+ ASSERT_EQ(0, server.Stop(0));
+ ASSERT_EQ(0, server.Join());
+ EXPECT_EQ(INVALID_BTHREAD_KEY, server._tl_options.tls_key);
+ EXPECT_EQ(factory.created.load(butil::memory_order_relaxed),
+ factory.destroyed.load(butil::memory_order_relaxed));
+}
+
+TEST_F(ServerTest, failed_start_destruction_cleans_thread_local_data) {
+ CountingTLSDataFactory factory;
+ brpc::ServerOptions options;
+ options.thread_local_data_factory = &factory;
+ options.reserved_thread_local_data = 4;
+ brpc::Server listener;
+ ASSERT_EQ(0, listener.Start("127.0.0.1:0", nullptr));
+ {
+ brpc::Server server;
+ ASSERT_EQ(-1, server.Start(listener.listen_address(), &options));
+ // No successful Start(), Stop() or Join() before destruction.
+ }
+ EXPECT_EQ(options.reserved_thread_local_data,
+ factory.created.load(butil::memory_order_relaxed));
+ EXPECT_EQ(factory.created.load(butil::memory_order_relaxed),
+ factory.destroyed.load(butil::memory_order_relaxed));
+}
+
+static bthread_key_t g_init_child_key = INVALID_BTHREAD_KEY;
+static bthread_t g_init_child = INVALID_BTHREAD;
+
+static void* InitChildOutlivingFailedStart(void*) {
+ // Outlive the failed Start(), then borrow and return a KeyTable of the
+ // server's keytable pool.
+ bthread_usleep(100 * 1000);
Review Comment:
`bthread_usleep(100 * 1000)` does not guarantee that the child accesses the
pool after the failed `Start()` and `Server` destruction. If the parent is
delayed, the child may finish while the pool is still valid
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
