Copilot commented on code in PR #3590:
URL: https://github.com/apache/brpc/pull/3590#discussion_r4215466866
##########
src/brpc/server.cpp:
##########
@@ -1370,6 +1374,26 @@ int Server::Join() {
return 0;
}
+void Server::DestroyPreStartAllocations() {
+ if (_status == RUNNING || _status == STOPPING) {
+ // Handled by Join().
+ return;
+ }
+ // Delete tls_key created in StartInternal().
+ if (_tl_options.tls_key != INVALID_BTHREAD_KEY) {
+ CHECK_EQ(0, bthread_key_delete(_tl_options.tls_key));
+ _tl_options.tls_key = INVALID_BTHREAD_KEY;
+ }
+ if (_keytable_pool != nullptr) {
+ CHECK_EQ(0, bthread_keytable_pool_destroy(_keytable_pool));
+ // Unlike Join(), no connection (thus no keytable user) can exist
+ // when the start failed before RUNNING, so the pool struct can be
+ // deleted as well.
+ delete _keytable_pool;
+ _keytable_pool = nullptr;
+ }
Review Comment:
The TLS key is deleted before `_keytable_pool` is destroyed.
`bthread_key_delete` clears the registered destructor, and `KeyTable::clear`
consults that metadata when the pool is destroyed, so any reserved or
already-created thread-local objects will be freed without calling
`DestroyServerTLS` and will leak. Destroy the pool first, as `Join()` does,
then delete the key.
##########
test/brpc_server_unittest.cpp:
##########
@@ -261,6 +261,24 @@ TEST_F(ServerTest, sanity) {
ASSERT_EQ(-1, server.Start(99999, nullptr));
ASSERT_EQ(0, server.Start(8613, nullptr));
}
+ {
+ // A start failing before RUNNING must release the keytable pool and
+ // the tls key allocated in StartInternal(), and a retry must not
+ // overwrite (and leak) the previous pool.
+ brpc::Server server1;
+ ASSERT_EQ(0, server1.Start(8614, nullptr));
+ brpc::Server server2;
+ ASSERT_EQ(-1, server2.Start(8614, nullptr)); // port in use
+ ASSERT_TRUE(server2._keytable_pool == nullptr);
+ ASSERT_EQ(INVALID_BTHREAD_KEY, server2._tl_options.tls_key);
Review Comment:
This regression never creates a TLS key: `ServerOptions` defaults
`thread_local_data_factory` to `nullptr`, and `StartInternal()` calls
`bthread_key_create2` only when that option is set. Consequently the `tls_key`
assertion is tautological and this test does not exercise the new
key/destructor cleanup (or reserved TLS data); pass a `DataFactory`-backed
`ServerOptions` to the failing starts and cover destruction of a failed
instance as well.
--
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]
