Copilot commented on code in PR #3615:
URL: https://github.com/apache/brpc/pull/3615#discussion_r4348731540
##########
test/resource_pool_unittest.cpp:
##########
@@ -277,6 +282,43 @@ TEST_F(ResourcePoolTest, get_perf) {
std::cout << describe_resources<SilentObj>() << std::endl;
}
+struct FreeChunkCountArgs {
+ ResourceId<FreeChunkCountObject> first;
+ ResourceId<FreeChunkCountObject> second;
+};
+
+void* add_two_free_chunks(void* arg) {
+ FreeChunkCountArgs* args = static_cast<FreeChunkCountArgs*>(arg);
+ get_resource(&args->first);
+ get_resource(&args->second);
+ return_resource(args->first);
+ return_resource(args->second);
+ return nullptr;
+}
+
+TEST_F(ResourcePoolTest, free_chunk_count_tracks_push_and_pop) {
+ // Keep the main LocalPool alive so worker teardown does not clear chunks.
+ ResourceId<FreeChunkCountObject> guard = { 0 };
+ ASSERT_NE(nullptr, get_resource(&guard));
+ FreeChunkCountArgs args = { { 0 }, { 0 } };
+ pthread_t tid;
+ ASSERT_EQ(0, pthread_create(&tid, nullptr, add_two_free_chunks, &args));
+ pthread_join(tid, nullptr);
Review Comment:
The return value of `pthread_join` isn’t checked, which can hide failures
and produce confusing downstream assertions. Consider asserting `pthread_join`
returns 0 so the test clearly fails if the worker thread can’t be joined.
##########
test/object_pool_unittest.cpp:
##########
@@ -243,6 +248,40 @@ TEST_F(ObjectPoolTest, get_perf) {
std::cout << describe_objects<SilentObj>() << std::endl;
}
+struct FreeChunkCountArgs {
+ FreeChunkCountObject* first;
+ FreeChunkCountObject* second;
+};
+
+void* add_two_free_chunks(void* arg) {
+ FreeChunkCountArgs* args = static_cast<FreeChunkCountArgs*>(arg);
+ args->first = get_object<FreeChunkCountObject>();
+ args->second = get_object<FreeChunkCountObject>();
+ return_object(args->first);
+ return_object(args->second);
+ return nullptr;
+}
+
+TEST_F(ObjectPoolTest, free_chunk_count_tracks_push_and_pop) {
+ // Keep the main LocalPool alive so worker teardown does not clear chunks.
+ FreeChunkCountObject* guard = get_object<FreeChunkCountObject>();
+ FreeChunkCountArgs args = { nullptr, nullptr };
+ pthread_t tid;
+ ASSERT_EQ(0, pthread_create(&tid, nullptr, add_two_free_chunks, &args));
+ pthread_join(tid, nullptr);
+
+ FreeChunkCountObject* first = get_object<FreeChunkCountObject>();
+ FreeChunkCountObject* second = get_object<FreeChunkCountObject>();
Review Comment:
`get_object<FreeChunkCountObject>()` results aren’t asserted non-null before
being used/returned later; if allocation fails, the test can crash or invoke UB
(e.g., `return_object(guard)` / comparisons). Add `ASSERT_NE(nullptr, guard)`
and also assert `first`/`second` are non-null before the equality checks and
returns.
##########
test/object_pool_unittest.cpp:
##########
@@ -243,6 +248,40 @@ TEST_F(ObjectPoolTest, get_perf) {
std::cout << describe_objects<SilentObj>() << std::endl;
}
+struct FreeChunkCountArgs {
+ FreeChunkCountObject* first;
+ FreeChunkCountObject* second;
+};
+
+void* add_two_free_chunks(void* arg) {
+ FreeChunkCountArgs* args = static_cast<FreeChunkCountArgs*>(arg);
+ args->first = get_object<FreeChunkCountObject>();
+ args->second = get_object<FreeChunkCountObject>();
+ return_object(args->first);
+ return_object(args->second);
+ return nullptr;
+}
+
+TEST_F(ObjectPoolTest, free_chunk_count_tracks_push_and_pop) {
+ // Keep the main LocalPool alive so worker teardown does not clear chunks.
+ FreeChunkCountObject* guard = get_object<FreeChunkCountObject>();
+ FreeChunkCountArgs args = { nullptr, nullptr };
+ pthread_t tid;
+ ASSERT_EQ(0, pthread_create(&tid, nullptr, add_two_free_chunks, &args));
+ pthread_join(tid, nullptr);
Review Comment:
The return value of `pthread_join` isn’t checked, which can mask thread
lifecycle failures and make the test flaky/harder to diagnose. Prefer
`ASSERT_EQ(0, pthread_join(tid, nullptr))` (or equivalent) to fail fast with a
clear signal.
--
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]
