Conversation
product-auto-label
Bot
added
the
api: storage
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the 'HedgedReadRecordsMetricsOnGlobalMeterProvider' unit test in 'connection_impl_test.cc'. It replaces a generic '.WillRepeatedly' mock expectation with a sequence of explicit '.WillOnce' expectations to strictly verify the thread execution order (primary vs. hedge). Additionally, it removes the 'primary_closed' promise/future synchronization and instead flushes the single-threaded read pool at the end of the test using a synchronous read to guarantee that any background tasks and captured references are fully cleaned up before teardown. I have no feedback to provide.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16517 +/- ##
==========================================
- Coverage 92.37% 92.36% -0.02%
==========================================
Files 2262 2262
Lines 217100 217115 +15
==========================================
- Hits 200548 200535 -13
- Misses 16552 16580 +28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| }); | ||
| return StatusOr<std::unique_ptr<ObjectReadSource>>(std::move(source)); | ||
| }) | ||
| .WillOnce([primary_thread](auto&, auto const&, |
There was a problem hiding this comment.
The CI is failing because on slower runners, a second hedge timer can occasionally fire. If that happens, this extra hedge request consumes your 3rd .WillOnce() (which was meant for the final sync read) and trips the thread ID assertion since it runs on the background thread.
| StatusOr<std::unique_ptr<ObjectReadSource>> sync_source = | ||
| client->ReadObject(ReadObjectRangeRequest("test-bucket", "sync")); | ||
| ASSERT_THAT(sync_source, IsOk()); | ||
| ASSERT_THAT((*sync_source)->Read(buffer.data(), buffer.size()), IsOk()); |
There was a problem hiding this comment.
IIUC, the "sync" read does not eliminate the teardown race because it still runs with hedging enabled, which means it will enqueue a new task on read_pool_ that captures self = shared_from_this(), and it does not flush hedge_pool_.
Since winning tasks on both pools unblock the caller before the worker thread finishes and destroys its captured client reference, a preempted worker on either pool can still outlive the test scope and self-detach.
|
I'm just going to skip this test for now in a separate PR. |
This branch was successfully deployed
3 participants
Attempts to address mock destruction issue observed in:
https://storage.googleapis.com/cloud-cpp-community-publiclogs/logs/google-cloud-cpp/main/fb87c707d22dde4155387df2701c86733c5e3460/fedora-latest-bazel-libcxx-__default__/log-5f855e4f-5bce-463c-a2dd-0e12aebcfd4c.txt