darion-yaphet commented on code in PR #3590:
URL: https://github.com/apache/brpc/pull/3590#discussion_r4245174241
##########
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
Good catch, thanks. You're right that the fixed sleep only makes the
ordering likely, not guaranteed. I'll rework the test so the child waits on an
atomic flag that the main thread sets only after the failed Start() has
returned and the Server has been destroyed, so the child always touches the
pool after destruction. I'll push the update shortly.
--
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]
