fix: concurrent Lazy::Get should get correct result#634
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Lazy::Get() so that callers always observe the correct cached initialization outcome (success or failure) across repeated/concurrent calls by caching the full Result<T> instead of only caching the value on success.
Changes:
- Update
Lazyto storemutable Result<T>and cache initialization errors as well as values viastd::call_once. - Add unit tests covering non-default-constructible lazy values and error reuse behavior.
- Register the new test file in the CMake
util_testtarget.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/iceberg/util/lazy.h |
Cache the entire Result<T> in the Lazy instance to ensure correct behavior after failed initialization and to avoid requiring T to be default-constructible. |
src/iceberg/test/lazy_test.cc |
Adds tests validating successful caching for non-default-constructible types and reuse of initialization errors. |
src/iceberg/test/CMakeLists.txt |
Adds lazy_test.cc to the util_test target sources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Fixes iceberg::Lazy::Get() so that initialization failures are correctly persisted and returned on subsequent calls (including across threads), instead of potentially returning an unintended “success” result.
Changes:
- Store the initialized state as
mutable Result<T> value_so both success and error outcomes are cached. - Simplify
Get()tocall_once-initializevalue_and then return either the cached error or a reference to the cached value. - Add unit tests for non-default-constructible values and for reusing initialization errors; register the new test in the util test target.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/iceberg/util/lazy.h |
Cache initialization result (Result<T>) and return the correct cached error/value after std::call_once. |
src/iceberg/test/lazy_test.cc |
Adds coverage for non-default-constructible cached values and for error reuse across calls. |
src/iceberg/test/CMakeLists.txt |
Adds lazy_test.cc to the util_test target. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1e6b2a2 to
52e59af
Compare
|
@wgtmac and I have talked about the Windows CI failure yesterday. You can try the following or just wait Gang to trigger the action manually. |
52e59af to
3cccd00
Compare
|
Thanks @HuaHuaY for the fix and @zhjwpku @kamcheungting-db for the review! BTW, glad to see you here, @kamcheungting-db :) |
No description provided.