wwbmmm commented on code in PR #3590:
URL: https://github.com/apache/brpc/pull/3590#discussion_r4244987154
##########
src/brpc/server.cpp:
##########
@@ -1370,6 +1374,30 @@ int Server::Join() {
return 0;
}
+void Server::DestroyPreStartAllocations() {
+ if (_status == RUNNING || _status == STOPPING) {
+ // Handled by Join().
+ return;
+ }
+ if (_keytable_pool != nullptr) {
+ // Keep the TLS key valid until its pooled data has been destroyed.
+ // Deleting the key first would disable DestroyServerTLS().
+ CHECK_EQ(0, bthread_keytable_pool_destroy(_keytable_pool));
+ // Same as Join(): don't delete the pool struct. Bthreads created
+ // with attr.keytable_pool == _keytable_pool may outlive
+ // the failed start and still call return_keytable() on exit. A
+ // destroyed pool makes them delete their KeyTables directly, but the
+ // struct itself must stay valid. The leak is annotated in
+ // StartInternal() and only happens on failed starts.
+ _keytable_pool = nullptr;
Review Comment:
[replied by brpc-oncall robot] This matches the deliberate behavior of
`Join()`, and is not a regression introduced here. The
`bthread_keytable_pool_t` struct cannot be freed while bthreads that hold
`attr.keytable_pool` may still call `borrow_keytable()`/`return_keytable()` on
it (see the TODO in `Join()`); deferring the free until those bthreads exit
would require refcounting the pool across arbitrary user bthreads, which is why
`Join()` keeps the struct around as well. `bthread_keytable_pool_destroy()`
already releases the reserved `KeyTable`s and their TLS data (the bulk of the
leak) and marks the pool destroyed so late `return_keytable()` calls just
delete their `KeyTable`. Only the small `bthread_keytable_pool_t` allocation is
retained, and that allocation is already suppressed for LeakSanitizer via
`ANNOTATE_SCOPED_MEMORY_LEAK` in `StartInternal()`. So keeping the struct is
the intended trade-off.
##########
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:
[replied by brpc-oncall robot] Agreed with @chenBright: the fixed sleep
makes this coverage timing-dependent. If the main thread is delayed, the child
can borrow/return its KeyTable while the pool is still alive, and the test
passes even with the pre-fix `delete _keytable_pool`. Make it deterministic by
having the child wait on a flag that the main thread sets only after
`server.Start()` has returned (the pool is destroyed during its rollback) —
e.g. spin on a `butil::atomic<bool>` or use a `bthread::CountdownEvent` — so
the child always touches the destroyed pool and the ASan failure reproduces
reliably with the old code.
--
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]
