wwbmmm commented on code in PR #3689:
URL: https://github.com/apache/brpc/pull/3689#discussion_r4500925466
##########
test/bthread_id_unittest.cpp:
##########
@@ -112,6 +116,111 @@ 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;
Review Comment:
[replied by brpc-oncall robot] Now that JoinBeforeWaitHookGuard exists,
`join_races_with_destroy` should use it too instead of assigning/clearing the
globals by hand here and at the end of the test. As written, if either
`bthread_id_trylock` or `bthread_id_unlock_and_destroy` fails and the ASSERT
returns early, the hook stays installed and the waiter keeps spinning in
`pause_join_before_wait()` on `args`, which is destroyed together with the test
frame — so the following tests would call `bthread_id_join()` and spin forever
on a dangling pointer. Please switch to the guard and let it also release the
waiter (e.g. set `allow_wait`/`start` in its destructor before clearing the
hook) so that a failed assertion cannot leave a spinning thread behind.
##########
src/bthread/id.cpp:
##########
@@ -107,6 +113,15 @@ struct PendingError {
PendingError() : id(INVALID_BTHREAD_ID), error_code(0), location(nullptr)
{}
};
+inline uint32_t LoadButex(const butil::atomic<int>* butex) {
Review Comment:
[replied by brpc-oncall robot] Free functions in this file follow snake_case
(id_value, id_status, make_id, get_version); please rename these helpers to
load_butex/store_butex for consistency. Also, the file-scope `using
bthread::LoadButex; using bthread::StoreButex;` ahead of the extern "C" block
is unnecessary — the neighbouring butex_* calls are already qualified with
`bthread::`, so qualifying these two as well keeps the global namespace clean.
--
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]
