wwbmmm commented on code in PR #3675:
URL: https://github.com/apache/brpc/pull/3675#discussion_r4500967859
##########
src/butil/object_pool_inl.h:
##########
@@ -520,7 +532,8 @@ class BAIDU_CACHELINE_ALIGNMENT ObjectPool {
if (nullptr == b) {
continue;
}
- for (size_t k = 0; k < b->nitem; ++k) {
+ const size_t nitem =
b->nitem.load(butil::memory_order_relaxed);
Review Comment:
[replied by brpc-oncall robot] Here the count is read with
`memory_order_relaxed`, while `describe_objects()` and
`unpoison_all_objects_before_leak_check()` use `memory_order_acquire` for the
same field. This path is stronger than a statistics read: it dereferences and
destructs the constructed objects (`obj->~T()`), and the block may have been
filled by another thread whose construction was published by the release store
in `BAIDU_OBJECT_POOL_GET`. A relaxed load gives no ordering with respect to
that release store, so the destructor may observe object memory without a
happens-before edge. Please use `memory_order_acquire` here as well (it is free
on x86 and matches the intent elsewhere), or add a comment explaining why
relaxed is provably sufficient in this path.
##########
test/object_pool_unittest.cpp:
##########
@@ -109,6 +111,41 @@ TEST_F(ObjectPoolTest, change_config) {
std::cout << describe_objects<MyObject>() << std::endl;
}
+struct DescribeObjectsArgs {
+ std::atomic<bool>* started;
+ std::atomic<bool>* done;
+};
+
+void* create_objects_while_describing(void* arg) {
+ DescribeObjectsArgs* args = static_cast<DescribeObjectsArgs*>(arg);
+ args->started->store(true, std::memory_order_release);
+ for (int i = 0; i < 100000; ++i) {
+ (void)get_object<DescribeRaceObject>();
+ }
+ args->done->store(true, std::memory_order_release);
+ return nullptr;
+}
+
+TEST_F(ObjectPoolTest, describe_while_creating_objects) {
+ // Keep the pool alive while the worker's LocalPool is being destroyed.
+ DescribeRaceObject* guard = get_object<DescribeRaceObject>();
+ ASSERT_NE(nullptr, guard);
+ std::atomic<bool> started(false);
+ std::atomic<bool> done(false);
+ DescribeObjectsArgs args = { &started, &done };
+ pthread_t tid;
+ ASSERT_EQ(0, pthread_create(&tid, nullptr,
+ create_objects_while_describing, &args));
+ while (!started.load(std::memory_order_acquire)) {
+ }
+ while (!done.load(std::memory_order_acquire)) {
+ describe_objects<DescribeRaceObject>();
Review Comment:
[replied by brpc-oncall robot] This test only exercises the concurrent path;
it has no assertion on the observed value, and there is no TSan job in CI (only
ASan), so it will silently pass even if `nitem` is reverted to a plain
non-atomic `size_t`. Suggest asserting the functional result after the worker
finishes, e.g. `ASSERT_EQ(100001u,
describe_objects<DescribeRaceObject>().item_num)` after `pthread_join` (1 guard
object + 100000 worker objects, blocks are never freed), so the case also
catches a broken/incomplete item count on regular builds. Optionally note in a
comment that the data race itself is only detected under TSan.
--
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]
