Destroy keytable pool and tls key when Server::Start() fails early by darion-yaphet · Pull Request #3590 · apache/brpc

darion-yaphet · 2026-10-08T05:48:57Z

What problem does this PR solve?

Problem Summary:

Server::StartInternal() allocates a keytable pool and, when thread-local data is configured, a TLS key before the server reaches RUNNING. If startup fails early—for example, because the requested port is already in use—RevertServerStatus calls Stop() and Join(), but both are no-ops before RUNNING.

As a result, the keytable pool and TLS key were leaked. Retrying Start() on the same Server could overwrite the pool pointer and make the previous allocation permanently unreachable. The same leak also affected destruction of a server whose startup had failed.

What is changed and the side effects?

Changed:

Add Server::DestroyPreStartAllocations() to release the TLS key and keytable pool when startup fails before RUNNING.
Invoke the cleanup from both startup rollback and Server destruction.
Add a regression test covering port-conflict startup failure, repeated failure, and a subsequent successful retry.

Side effects:

Performance effects: None expected. The cleanup runs only on startup-failure paths or destruction of a never-started server.
Breaking backward compatibility: None.

Check List:

Changes compile with CMake: cmake --build build --target brpc_server_unittest -j6
Added a regression test.
Test passed: ./build/test/brpc_server_unittest --gtest_filter=ServerTest.sanity

Copilot

🟡 Changes recommended

Destroy the keytable pool before deleting the TLS key and expand tests to exercise TLS cleanup.

2 open findings

Delete TLS key after destroying the key table pool · New
Configure TLS data factory to exercise key cleanup regression · New

What changed in this PR

This PR fixes leaks from server startup failures before reaching RUNNING and adds retry regression coverage.

Changes:

Adds pre-start allocation cleanup during rollback and destruction.
Adds port-conflict and retry regression tests.
Declares cleanup support for startup rollback.

File	Summary
`test/brpc_server_unittest.cpp`	Adds failed-start retry coverage, but does not exercise TLS cleanup.
`src/brpc/server.h`	Declares the pre-start cleanup helper and rollback access.
`src/brpc/server.cpp`	Implements startup-failure cleanup; cleanup ordering can leak TLS data.

🧠 Review effort: Lite

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

StartInternal() allocates _keytable_pool (and the tls key when a thread_local_data_factory is set) unconditionally, but on failures before _status reaches RUNNING - e.g. tcp_listen() fails because the port is in use - RevertServerStatus runs Stop(0)+Join(), both of which return immediately when the server is not RUNNING. The pool and the key leaked on every failed attempt, and a retry overwrote _keytable_pool with a fresh pool, permanently leaking the previous one. ~Server() had the same problem for a failed-start server. Add Server::DestroyPreStartAllocations() (no-op once RUNNING/STOPPING, where Join() handles cleanup) and call it from RevertServerStatus and ~Server(). Unlike Join() the pool struct is deleted too, since no connection can exist before RUNNING. Verified in ServerTest.sanity: after a port-conflict failure, _keytable_pool is nullptr and the tls key is invalid (on the old code both leak and the test fails), including across a failed retry. Full brpc_server_unittest suite shows no new failures.

Copilot

🔵 Needs a closer look

The pre-start keytable pool cleanup may leak platform resources by leaving its rwlock undestroyed.

0 open findings

2 resolved since last review

Delete TLS key after destroying the key table pool
Configure TLS data factory to exercise key cleanup regression

🧠 Review effort: Lite

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

wwbmmm · 2026-10-08T12:38:56Z

LGTM

darion-yaphet · 2026-10-09T16:00:14Z

The init bthreads themselves are joined inside StartInternal(), and task_runner() calls return_keytable() before bumping the version, so they're done with the pool by the time we reach a failure path.

But anything spawned from bthread_init_fn with the inherited attr carries can outlive the failed Start(). It still calls borrow_keytable()/return_keytable() on that pool. With delete _keytable_pool that's a heap-use-after-free. ASan confirms it.

Fixed by doing what Join() does: call bthread_keytable_pool_destroy() but don't delete the struct. A destroyed pool makes late return_keytable() calls just delete their KeyTable, and the struct stays valid. The TLS data and reserved KeyTables are still released, which was the main leak. I also added failed_start_keeps_keytable_pool_for_init_bthreads, which spawns such a child from bthread_init_fn and fails Start() with EADDRINUSE. It fails under ASan with the old code and passes now.

(bthread_getattr(bthread_self(), ...)) carry keytable_pool == _keytable_pool and may outlive a failed Server::Start(). They still call return_keytable() on exit, so deleting the pool struct in DestroyPreStartAllocations() causes a heap-use-after-free (reported by ASan at key.cpp:394). Do the same as Join(): destroy the pool contents with bthread_keytable_pool_destroy() but don't delete the struct. A destroyed pool makes late return_keytable() calls delete their KeyTable directly. The leftover struct is small, only leaked on failed starts, and already annotated in StartInternal(). Add failed_start_keeps_keytable_pool_for_init_bthreads, which spawns such a child from bthread_init_fn and fails Start() with EADDRINUSE.

Copilot

🟡 Changes recommended

The heap-allocated keytable pool object remains leaked during failed startup cleanup.

1 open finding

Failed Start leaks the allocated keytable pool object · New

🧠 Review effort: Lite

wwbmmm

[replied by brpc-oncall robot] The production-side fix is correct, but the new regression test relies on a fixed sleep and therefore does not deterministically exercise the use-after-free; please synchronize the child bthread with the main thread.

Replace the fixed sleep with a CountdownEvent so the child bthread continues only after the failed Start() returns and the Server is destroyed. This ensures that the regression test exercises access to the destroyed keytable pool. Use a nonfatal Start() assertion so the child is still signaled and joined if startup unexpectedly succeeds.

darion-yaphet · 2026-10-16T05:45:47Z

Addressed the feedback: the child now waits on a CountdownEvent signaled only after the failed Start() returns and the Server is destroyed, so it always touches the destroyed pool.

@chenBright @wwbmmm PTAL.

Centralize keytable pool and TLS key cleanup for normal shutdown and failed-start recovery while preserving their destruction order and pool lifetime constraints.

Copilot

🟡 Changes recommended

Pre-start cleanup still has pool-lifetime leaks, TLS data loss, and a race with outliving init bthreads.

2 open issues

_{Review effort: Lite}

Copilot

🟡 Changes recommended

The reviewed teardown has a pool-object leak and an unsynchronized data race during concurrent keytable access.

2 open issues

1 resolved since last review

Early key deletion leaks outliving bthread TLS data

_{Review effort: Lite}

chenBright · 2026-10-18T14:03:01Z

+        if (_tl_options.tls_key != INVALID_BTHREAD_KEY) {
+            CHECK_EQ(0, bthread_keytable_pool_destroy_and_delete_key(
+                            _keytable_pool, _tl_options.tls_key));
+            _tl_options.tls_key = INVALID_BTHREAD_KEY;


thread_local_data_factory is non-owned and is only documented as needing to remain valid while the server is running. Keeping the TLS key valid does not keep its raw factory pointer alive. After a failed Start() or shutdown, the caller may destroy the factory, but an outliving bthread can still invoke DestroyServerTLS() through that pointer.

chenBright · 2026-10-18T14:03:52Z

+        if (m->attr.keytable_pool != nullptr) {
+            bthread_keytable_pool_release(m->attr.keytable_pool);
+        }


Pool destruction holds the pool write lock while invoking cached TLS destructors. If a destructor signals and joins a same-pool child that has no KeyTable, that child now needs the same write lock in bthread_keytable_pool_release() before it can finish. The cleanup thread waits for the child, while the child waits for the cleanup thread’s lock.

chenBright · 2026-10-18T14:05:29Z

+    if (pool == nullptr) {
+        return 0;
+    }
+    pthread_rwlock_wrlock(&pool->rwlock);


Every live-pool bthread now acquires the same pool write lock during creation and exit, including servers without a TLS factory. This adds serialization to normal processing, beyond startup-failure cleanup

darion-yaphet · 2026-10-19T04:59:31Z

Thanks for pointing these out. I agree this PR has grown beyond its original goal of cleaning up allocations when Server::Start() fails before RUNNING. The deferred TLS key deletion and pool reference tracking introduce broader lifecycle and performance concerns.

I’ll move that work into a separate PR to address factory lifetime, teardown deadlocks, and normal-path locking overhead together. I’ll rework this PR around the original startup-failure cleanup and regression tests, and verify that the reduced scope is independently safe.

chenBright

LGTM

chenBright requested a lite review from Copilot October 8, 2026 06:05

Copilot started reviewing on behalf of chenBright October 8, 2026 06:06

View session

Copilot AI reviewed Oct 8, 2026

View reviewed changes

Comment thread src/brpc/server.cpp Outdated

Comment thread test/brpc_server_unittest.cpp Outdated

darion-yaphet force-pushed the fix/server-start-early-failure-cleanup branch from ea780aa to d353964 Compare October 8, 2026 06:30

wwbmmm requested a lite review from Copilot October 8, 2026 07:01

Copilot started reviewing on behalf of wwbmmm October 8, 2026 07:01

View session

Copilot AI reviewed Oct 8, 2026

View reviewed changes

chenBright reviewed Oct 9, 2026

View reviewed changes

Comment thread src/brpc/server.cpp Outdated

wwbmmm requested a lite review from Copilot October 10, 2026 02:49

Copilot started reviewing on behalf of wwbmmm October 10, 2026 02:49

View session

Copilot AI reviewed Oct 10, 2026

View reviewed changes

Comment thread src/brpc/server.cpp

chenBright reviewed Oct 10, 2026

View reviewed changes

Comment thread test/brpc_server_unittest.cpp Outdated

wwbmmm reviewed Oct 12, 2026

View reviewed changes

Comment thread src/brpc/server.cpp

Comment thread test/brpc_server_unittest.cpp Outdated

chenBright reviewed Oct 17, 2026

View reviewed changes

Comment thread src/brpc/server.cpp

chenBright reviewed Oct 17, 2026

View reviewed changes

Comment thread src/brpc/server.cpp

refactor(server): share thread-local cleanup

2a9a2f0

Centralize keytable pool and TLS key cleanup for normal shutdown and failed-start recovery while preserving their destruction order and pool lifetime constraints.

chenBright requested a lite review from Copilot October 18, 2026 04:02

Copilot started reviewing on behalf of chenBright October 18, 2026 04:03

View session

Copilot AI reviewed Oct 18, 2026

View reviewed changes

Comment thread src/brpc/server.cpp

chenBright requested a lite review from Copilot October 18, 2026 09:32

Copilot started reviewing on behalf of chenBright October 18, 2026 09:33

View session

Copilot AI reviewed Oct 18, 2026

View reviewed changes

Comment thread src/bthread/key.cpp Outdated

chenBright reviewed Oct 18, 2026

View reviewed changes

darion-yaphet force-pushed the fix/server-start-early-failure-cleanup branch from 4b5f71b to 2a9a2f0 Compare October 19, 2026 05:28

chenBright approved these changes Oct 19, 2026

View reviewed changes

chenBright merged commit 8252e8e into apache:master Oct 19, 2026
50 checks passed

Conversation

darion-yaphet commented Oct 8, 2026

What problem does this PR solve?

What is changed and the side effects?

Check List:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

wwbmmm commented Oct 8, 2026

Uh oh!

Uh oh!

darion-yaphet commented Oct 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

wwbmmm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

darion-yaphet commented Oct 16, 2026

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

chenBright Oct 18, 2026

Choose a reason for hiding this comment

Uh oh!

chenBright Oct 18, 2026

Choose a reason for hiding this comment

Uh oh!

chenBright Oct 18, 2026

Choose a reason for hiding this comment

Uh oh!

darion-yaphet commented Oct 19, 2026

Uh oh!

chenBright left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants