Stabilize timing-sensitive bthread tests and fix the butex interruption race

[chenBright]

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

Running unit tests in parallel exposes timing assumptions that can cause intermittent failures:

• Fixed sleeps do not guarantee that workers have entered the expected waiting state.
• Tight elapsed-time assertions fail under normal scheduling delays.
• An increased connection count does not guarantee that an accepted socket has been published to the connection list.
• Some shutdown paths can lose wakeups or leave worker threads unjoined.
• An interruption between pthread butex waiter registration and the underlying wait can return an incorrect error code.
• Multi-tag tests compare prefilled zero values instead of validating the actual tags.

What is changed and the side effects?

Changed:

• Replace fixed sleeps with explicit synchronization, state observation, and completion acknowledgements.
• Check timeout, interruption, wakeup, and completion semantics instead of narrow scheduling-time windows.
• Retain timeout guards and checks that waits do not complete prematurely.
• Fix the pthread butex interruption race while preserving genuine value-mismatch and timeout errors.
• Wait for accepted sockets to appear in the connection list before checking their buffer settings.
• Improve stop-flag synchronization, thread cleanup, and state reset for repeated test execution.
• Correct the result containers and assertions in multi-tag tests.
• Replace public-network connection dependencies with local TCP listeners and add a Linux-specific connection timeout case.

Side effects:

• Performance effects:

• Breaking backward compatibility:

---

Check List:

• Please make sure your changes are compilable.
• When providing us with a new feature, it is best to add related tests.
• Please follow Contributor Covenant Code of Conduct.

[Copilot]

🟡 Changes recommended

Unresolved test-synchronization and cleanup issues remain, including a critical timer-thread cleanup path.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Stabilizes timing-sensitive bthread tests and fixes a pthread-backed butex interruption race.

Changes:

• Replaces fixed sleeps with state polling and completion synchronization.
• Improves cleanup, socket readiness checks, and multi-tag assertions.
• Preserves butex timeout and value-mismatch error semantics.

File summaries

File	Reviewed changes and findings
test/bthread_work_stealing_queue_unittest.cpp	Makes stop-state synchronization atomic and resettable.
test/bthread_unittest.cpp	Observes sleep registration before stopping. moderate (1 vote): Fatal assertion cleanup can leave the worker alive.
test/bthread_timer_thread_unittest.cpp	Adds timer state waits. critical (1 vote): Fatal assertions can skip timer cleanup while callbacks reference stack state. moderate (1 vote): _npending is not reset per round.
test/bthread_rwlock_unittest.cpp	Synchronizes lock-state tests. moderate (1 vote each): Fixed reader delays do not guarantee contention, and non-fatal writer registration checks can allow the ordering test to pass spuriously.
test/bthread_mutex_unittest.cpp	Waits for mutex contention. moderate (1 vote): Fatal timeout cleanup can leave a locked mutex and live bthread.
test/bthread_futex_unittest.cpp	Adds bounded waiter cleanup. moderate (1 vote): Cleanup-induced EWOULDBLOCK results are not accepted when appropriate.
test/bthread_fd_unittest.cpp	Improves interruption and local connection tests. moderate (3 votes): pthread_kill may return ESRCH during the completion race.
test/bthread_cond_unittest.cpp	Resets shared condition-test state.
test/bthread_butex_unittest.cpp	Adds interruption and timing tests. moderate (1 vote each): Synchronization does not establish waiter registration, and a non-fatal timer-registration check can allow the interruption test to pass without validating its precondition.
test/bthread_butex_multi_tag_unittest.cpp	Validates actual task tags.
test/brpc_socket_unittest.cpp	Waits for accepted socket publication before checking settings.
src/bthread/butex.cpp	Fixes pthread butex interruption race handling.

Review details

Suppressed comments (8)

test/bthread_butex_unittest.cpp:258

• The gate only acknowledges execution immediately before butex_wait; it does not establish that the waiter has been registered, nor does it hold the task between registration and futex_wait_private. Consequently bthread_stop can occur before registration or after the wait has started, and 100 repetitions can still pass without exercising the new EWOULDBLOCK-to-EINTR path. Add a deterministic synchronization point around waiter registration and the underlying wait.

        arg->entering_wait->signal();
    }
    timespec ts = butil::milliseconds_from_now(arg->wait_msec);
    int rc = bthread::butex_wait(arg->butex, arg->expected_val,
                               arg->wait_msec < 0 ? nullptr : &ts);

test/bthread_butex_unittest.cpp:449

• EXPECT_TRUE is non-fatal here, so if timer registration never appears the test continues to bthread_stop. For the normal-stack case that stop is then consumed before bthread_usleep, which still produces the expected ESTOP; the test can therefore pass without validating interruption after sleep registration. Record the failure and make it fail after cleanup, or otherwise make this precondition mandatory while preserving cleanup.

                EXPECT_TRUE(sleeping) << "Timed out waiting for sleep registration";

test/bthread_futex_unittest.cpp:124

• The failure cleanup below sets lock1 to 1 to release waiters that had not registered yet. Such a waiter returns -1 with EWOULDBLOCK because the futex value no longer matches, but this helper unconditionally expects rc == 0, so a missed registration produces extra assertion failures and obscures the intended deadline failure. Accept the value-mismatch result only when the cleanup store has occurred.

    EXPECT_EQ(0, rc);

test/bthread_mutex_unittest.cpp:62

• When the deadline expires, ASSERT_EQ returns from the test before unlocking m or joining th1; locker is still blocked on the mutex, so this failure path leaks a live bthread and can contaminate later tests. Use a non-fatal check here so the existing cleanup always runs.

    ASSERT_EQ(257u, state->load(butil::memory_order_relaxed));

test/bthread_rwlock_unittest.cpp:536

• The new timed acquisition still relies on the preceding fixed 50 ms sleep to create a continuous reader load. Under scheduler delay, the writer can acquire before any reader has entered, so the test passes without exercising the starvation scenario it claims to cover. Wait on an explicit reader-ready count (and then keep readers active) before starting the writer.

    // A timed acquisition also makes the failure path reachable when the
    // writer really starves, so readers can be stopped and joined safely.
    timespec deadline = butil::seconds_from_now(10);
    int rc = bthread_rwlock_timedwrlock(&rw, &deadline);
    EXPECT_EQ(0, rc) << "Writer starved under concurrent readers";

test/bthread_rwlock_unittest.cpp:358

• EXPECT_TRUE lets the test proceed when the writer was not observed in writer_wait_count. It then starts the reader and may still pass based on scheduling, so the ordering assertion no longer proves the documented writer-priority race. Make registration a mandatory precondition while retaining cleanup if the wait times out.

    EXPECT_TRUE(WaitForRWLockState([&] {
        return reinterpret_cast<butil::atomic<unsigned>*>(rw.writer_wait_count)
            ->load(butil::memory_order_relaxed) == 1;
    }));

test/bthread_timer_thread_unittest.cpp:342

• _npending is the heap size published by the timer thread and is not reset per round. After round 0 it remains at least kBatch, so on later rounds this predicate is already true before the newly scheduled near-term task is consumed; the test can unschedule the batch while it is still in buckets and pass without exercising the sweep path. Wait on a per-round callback acknowledgement before unscheduling.

        ASSERT_TRUE(WaitUntil([&] {
            return timer_thread._npending.load(butil::memory_order_relaxed) >=
                   static_cast<int64_t>(kBatch);
        }));

test/bthread_unittest.cpp:527

• If the sleep-registration deadline expires, this fatal assertion returns before bthread_stop and bthread_join; the 60-second worker is then left alive, contrary to the cleanup comment and potentially affecting the rest of the test process. This should be non-fatal so the stop/join cleanup still executes.

    ASSERT_TRUE(sleeping);

• Files reviewed: 12/12 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.