chenBright commented on code in PR #3590:
URL: https://github.com/apache/brpc/pull/3590#discussion_r4307315084
##########
src/brpc/server.cpp:
##########
@@ -1370,6 +1359,41 @@ int Server::Join() {
return 0;
}
+void Server::DestroyPreStartAllocations() {
+ if (_status == RUNNING || _status == STOPPING) {
+ // Handled by Join().
+ return;
+ }
+ DestroyThreadLocalResources();
+}
+
+void Server::DestroyThreadLocalResources() {
+ if (_keytable_pool != nullptr) {
+ // Keep the TLS key valid until every bthread created with this pool
+ // has returned its KeyTable. Deleting it sooner would disable
+ // DestroyServerTLS() for an init bthread that outlives this server.
+ 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;
Review Comment:
`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.
##########
src/bthread/task_group.cpp:
##########
@@ -410,6 +411,9 @@ void TaskGroup::task_runner(intptr_t skip_remained) {
tls_bls_ptr->keytable = nullptr;
m->local_storage.keytable = nullptr; // optional
}
+ if (m->attr.keytable_pool != nullptr) {
+ bthread_keytable_pool_release(m->attr.keytable_pool);
+ }
Review Comment:
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.
##########
src/bthread/key.cpp:
##########
@@ -499,12 +514,67 @@ int
bthread_keytable_pool_destroy(bthread_keytable_pool_t* pool) {
if (g) {
g->current_task()->local_storage.keytable = old_kt;
}
+
+ bthread_key_t key_to_delete = INVALID_BTHREAD_KEY;
+ pthread_rwlock_wrlock(&pool->rwlock);
+ pool->destroy_complete = 1;
+ if (pool->delete_deferred_key && pool->active_bthreads == 0) {
+ pool->delete_deferred_key = 0;
+ key_to_delete = pool->deferred_key;
+ pool->deferred_key = INVALID_BTHREAD_KEY;
+ }
+ pthread_rwlock_unlock(&pool->rwlock);
+ if (key_to_delete != INVALID_BTHREAD_KEY) {
+ CHECK_EQ(0, bthread_key_delete(key_to_delete));
+ }
// TODO: return_keytable may race with this function, we don't destroy
// the mutex right now.
// pthread_mutex_destroy(&pool->mutex);
return 0;
}
+int bthread_keytable_pool_destroy(bthread_keytable_pool_t* pool) {
+ return destroy_keytable_pool(pool, INVALID_BTHREAD_KEY, false);
+}
+
+int bthread_keytable_pool_destroy_and_delete_key(
+ bthread_keytable_pool_t* pool, bthread_key_t key) {
+ return destroy_keytable_pool(pool, key, true);
+}
+
+int bthread_keytable_pool_addref(bthread_keytable_pool_t* pool) {
+ if (pool == nullptr) {
+ return 0;
+ }
+ pthread_rwlock_wrlock(&pool->rwlock);
Review Comment:
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
--
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]
