chenBright commented on code in PR #3689:
URL: https://github.com/apache/brpc/pull/3689#discussion_r4454648318
##########
test/bthread_id_unittest.cpp:
##########
@@ -112,6 +116,51 @@ TEST(BthreadIdTest, join_before_destroy) {
ASSERT_EQ(1UL, non_null_ret);
}
+struct JoinRaceArgs {
+ bthread_id_t id;
+ std::atomic<bool> join_butex_read;
+ std::atomic<bool> allow_wait;
+};
+
+void pause_join_before_wait(void* arg) {
+ JoinRaceArgs* args = static_cast<JoinRaceArgs*>(arg);
+ args->join_butex_read.store(true, std::memory_order_release);
+ while (!args->allow_wait.load(std::memory_order_acquire)) {
+ std::this_thread::yield();
+ }
+}
+
+void* join_race_waiter(void* arg) {
+ JoinRaceArgs* args = static_cast<JoinRaceArgs*>(arg);
+ EXPECT_EQ(0, bthread_id_join(args->id));
+ return nullptr;
+}
+
+TEST(BthreadIdTest, join_races_with_destroy) {
+ bthread_id_t id;
+ ASSERT_EQ(0, bthread_id_create(&id, nullptr, nullptr));
+ JoinRaceArgs args;
+ args.id = id;
+ args.join_butex_read.store(false, std::memory_order_relaxed);
+ args.allow_wait.store(false, std::memory_order_relaxed);
+ bthread::g_bthread_id_join_before_wait_for_test = pause_join_before_wait;
+ bthread::g_bthread_id_join_before_wait_arg_for_test = &args;
+
+ pthread_t waiter;
+ ASSERT_EQ(0, pthread_create(&waiter, nullptr, join_race_waiter, &args));
+ while (!args.join_butex_read.load(std::memory_order_acquire)) {
+ std::this_thread::yield();
+ }
+ void* data = nullptr;
+ ASSERT_EQ(0, bthread_id_trylock(id, &data));
+ ASSERT_EQ(0, bthread_id_unlock_and_destroy(id));
+ args.allow_wait.store(true, std::memory_order_release);
+ ASSERT_EQ(0, pthread_join(waiter, nullptr));
Review Comment:
`bthread_id_unlock_and_destroy()` completes before the release store to
`allow_wait`. The waiter enters `butex_wait()` only after a successful acquire,
so the destruction write happens-before the butex state read that needs to be
covered. The read before the hook is already protected by `meta->mutex`. With
this ordering, even reverting the state write to a plain assignment would not
cause the data race in question, so this test cannot protect the atomic access
fix. Consider keeping this case to verify the old-value waiting behavior, and
adding a TSAN regression case that lets the waiter proceed before the
destruction write, then verifying that reverting the atomic access makes TSAN
detect the race.
--
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]
