Skip to content

fix(server): synchronize bthread initialization handoff - #3740

Open
songgaoye wants to merge 1 commit into
apache:masterfrom
songgaoye:fix/server-bthread-init-race
Open

songgaoye wants to merge 1 commit into
apache:masterfrom
songgaoye:fix/server-bthread-init-race

Conversation

@songgaoye

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

Server::StartInternal() creates temporary bthreads to run
ServerOptions::bthread_init_fn.

The initialization bthread writes BthreadInitArgs::done and repeatedly reads
BthreadInitArgs::stop, while the startup thread reads done and writes
stop. These fields were ordinary bools, so the concurrent accesses formed
C++ data races. In particular, observing completion did not safely publish the
init callback result, and the init bthread was not synchronized with the
shutdown request.

What is changed and the side effects?

Changed:

  • Make BthreadInitArgs::done and stop atomic.
  • Publish init completion with a release store after writing result.
  • Consume completion with an acquire load before proceeding with startup.
  • Synchronize the stop request with release/acquire operations.
  • Add a barrier-based regression test covering the init callback completion,
    startup, stop, and join handoff.

The new test reports the original race under TSAN on the unmodified code. The
targeted BthreadInitEntry / StartInternal report disappears after this
change.

Side effects:

  • Performance effects:

    This affects only temporary server-startup bthreads. It adds atomic accesses
    to their completion and stop polling paths, not RPC request processing.

  • Breaking backward compatibility:

    None.


Check List:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Human confirmation is needed because correctness depends on runtime scheduling and TSAN coverage across supported architectures.

No open issues 🎉

What changed in this PR

This PR fixes data races during server startup by synchronizing completion and shutdown handoff for temporary bthreads used by bthread_init_fn. It also adds a regression test covering the initialization, startup, stop, and join sequence.

Changes:

  • Makes done and stop atomic with appropriate release/acquire ordering.
  • Publishes the initialization callback result before startup proceeds.
  • Adds a controlled regression test for the bthread initialization handoff.
File Description
test/​brpc_server_unittest.cpp Adds a regression test covering synchronized bthread initialization, startup, stop, and join sequencing.
src/​brpc/​server.cpp Uses atomic completion and stop signaling to synchronize temporary initialization bthreads with server startup.

Review effort: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants