store writer txn instead of write_req when write_req is processed#192
Conversation
WalkthroughRefactor: the writer state in ReaderWriterCntl was changed from an atomic pointer to a tagged 64-bit atomic that can encode either a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant ReaderWriterCntl
participant Queue
Client->>ReaderWriterCntl: AddWriter(write_req)
alt store pointer
ReaderWriterCntl->>ReaderWriterCntl: writer_ = EncodeWriterPtr(write_req)
else store txn-after-processing
ReaderWriterCntl->>ReaderWriterCntl: writer_ = EncodeWriterTxn(txn_id)
end
ReaderWriterCntl->>Queue: enqueue if needed
Client->>ReaderWriterCntl: FinishReader(reader_req)
ReaderWriterCntl->>ReaderWriterCntl: load writer_ (IsWriterPtr / Decode)
alt writer is ptr
ReaderWriterCntl->>ReaderWriterCntl: transition writer -> ProcessedWriter, store txn encoding
ReaderWriterCntl->>Queue: enqueue writer
else writer is txn
ReaderWriterCntl->>ReaderWriterCntl: validate txn id
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
include/cc/reader_writer_cntl.h(2 hunks)src/cc/reader_writer_cntl.cpp(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: thweetkomputer
Repo: eloqdata/tx_service PR: 150
File: include/cc/local_cc_shards.h:626-631
Timestamp: 2025-10-09T03:56:58.811Z
Learning: For the LocalCcShards class in include/cc/local_cc_shards.h: Writer locks (unique_lock) should continue using the original meta_data_mux_ (std::shared_mutex) rather than fast_meta_data_mux_ (FastMetaDataMutex) at this stage. Only reader locks may use the FastMetaDataMutex wrapper.
📚 Learning: 2025-10-09T03:56:58.811Z
Learnt from: thweetkomputer
Repo: eloqdata/tx_service PR: 150
File: include/cc/local_cc_shards.h:626-631
Timestamp: 2025-10-09T03:56:58.811Z
Learning: For the LocalCcShards class in include/cc/local_cc_shards.h: Writer locks (unique_lock) should continue using the original meta_data_mux_ (std::shared_mutex) rather than fast_meta_data_mux_ (FastMetaDataMutex) at this stage. Only reader locks may use the FastMetaDataMutex wrapper.
Applied to files:
src/cc/reader_writer_cntl.cppinclude/cc/reader_writer_cntl.h
📚 Learning: 2025-09-25T06:15:41.186Z
Learnt from: liunyl
Repo: eloqdata/tx_service PR: 142
File: include/tx_req_result.h:140-159
Timestamp: 2025-09-25T06:15:41.186Z
Learning: When cc_notify_ is true in TxResult, both Finish() and Wait() are guaranteed to be called on the same thread, eliminating concurrency concerns and making relaxed memory ordering sufficient for status_combo_ updates.
Applied to files:
src/cc/reader_writer_cntl.cppinclude/cc/reader_writer_cntl.h
githubzilla
force-pushed
the
fix_reader_writer_cntl
branch
from
9a009fd to
ae0b73f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cc/reader_writer_cntl.cpp (1)
89-110: Critical: Guard against nullptr before dereferencing (similar issue at lines 114-128).The assertion on lines 93-95 validates writer existence in debug builds, but there's a race condition that can lead to undefined behavior in release builds:
- Thread A calls
FinishWriter, storesnullptrtowriter_(line 198)- Before Thread A sets status to
Invalid(line 199), Thread B callsAddWriter- Thread B observes stale
PendingWriterstatus and loadsnullptrfromwriter_(line 89)- In release builds, lines 97-100 dereference
nullptrwithout protectionApply this defensive fix for both the PendingWriter case (lines 97-100) and the ProcessedWriter case (lines 122-125):
- uint64_t writer_txn = - std::holds_alternative<CcRequestBase *>(writer) - ? std::get<CcRequestBase *>(writer)->Txn() - : std::get<uint64_t>(writer); + uint64_t writer_txn; + if (std::holds_alternative<CcRequestBase *>(writer)) + { + CcRequestBase *write_req_ptr = std::get<CcRequestBase *>(writer); + if (write_req_ptr == nullptr) + { + return AddWriterResult::Invalid; + } + writer_txn = write_req_ptr->Txn(); + } + else + { + writer_txn = std::get<uint64_t>(writer); + }
🧹 Nitpick comments (1)
src/cc/reader_writer_cntl.cpp (1)
198-199: Consider strengthening memory ordering for the final state transition.The two relaxed stores create a window where other threads might observe
writer_ == nullptrbefore seeingWriteStatus::Invalid. While the critical nullptr dereference issue is addressed inAddWriter, usingmemory_order_releasehere would provide clearer happens-before semantics for the state transition.- writer_.store(nullptr, std::memory_order_relaxed); - cntl_block_.store({0, WriteStatus::Invalid}, std::memory_order_relaxed); + writer_.store(nullptr, std::memory_order_release); + cntl_block_.store({0, WriteStatus::Invalid}, std::memory_order_release);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CMakeLists.txt(2 hunks)include/cc/reader_writer_cntl.h(2 hunks)src/cc/reader_writer_cntl.cpp(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: thweetkomputer
Repo: eloqdata/tx_service PR: 150
File: include/cc/local_cc_shards.h:626-631
Timestamp: 2025-10-09T03:56:58.811Z
Learning: For the LocalCcShards class in include/cc/local_cc_shards.h: Writer locks (unique_lock) should continue using the original meta_data_mux_ (std::shared_mutex) rather than fast_meta_data_mux_ (FastMetaDataMutex) at this stage. Only reader locks may use the FastMetaDataMutex wrapper.
📚 Learning: 2025-10-09T03:56:58.811Z
Learnt from: thweetkomputer
Repo: eloqdata/tx_service PR: 150
File: include/cc/local_cc_shards.h:626-631
Timestamp: 2025-10-09T03:56:58.811Z
Learning: For the LocalCcShards class in include/cc/local_cc_shards.h: Writer locks (unique_lock) should continue using the original meta_data_mux_ (std::shared_mutex) rather than fast_meta_data_mux_ (FastMetaDataMutex) at this stage. Only reader locks may use the FastMetaDataMutex wrapper.
Applied to files:
src/cc/reader_writer_cntl.cppinclude/cc/reader_writer_cntl.h
📚 Learning: 2025-09-25T06:15:41.186Z
Learnt from: liunyl
Repo: eloqdata/tx_service PR: 142
File: include/tx_req_result.h:140-159
Timestamp: 2025-09-25T06:15:41.186Z
Learning: When cc_notify_ is true in TxResult, both Finish() and Wait() are guaranteed to be called on the same thread, eliminating concurrency concerns and making relaxed memory ordering sufficient for status_combo_ updates.
Applied to files:
src/cc/reader_writer_cntl.cppinclude/cc/reader_writer_cntl.h
🧬 Code graph analysis (1)
src/cc/reader_writer_cntl.cpp (1)
include/cc/reader_writer_cntl.h (1)
ccs_(68-93)
🔇 Additional comments (6)
CMakeLists.txt (1)
227-227: LGTM!Adding the
atomiclibrary is necessary to supportstd::atomic<std::variant<CcRequestBase *, uint64_t>>on platforms where atomic operations on types larger than the native word size require runtime support from libatomic.include/cc/reader_writer_cntl.h (2)
6-8: LGTM!The new includes are necessary for the variant-based writer state representation. The
<variant>header provides the std::variant type, and"cc_req_base.h"provides the full definition of CcRequestBase needed for the variant.
69-69: LGTM!The transition from
std::atomic<CcRequestBase *>tostd::atomic<std::variant<CcRequestBase *, uint64_t>>enables storing either a request pointer or a transaction identifier, supporting more flexible writer state management. Initializing withnullptrcorrectly constructs the CcRequestBase* alternative.src/cc/reader_writer_cntl.cpp (3)
51-76: LGTM!The logic correctly handles the variant: it checks for the CcRequestBase* alternative, validates the pointer is non-null before dereferencing, and stores the transaction number after successfully transitioning to ProcessedWriter status.
150-166: LGTM!The two-phase update pattern is correct: store the request pointer first (line 150) for potential use by
FinishReader, then replace it with the transaction number (line 164) after successfully transitioning toProcessedWriterstatus. This ensures both code paths (caller processing or last-reader processing) have the necessary information.
172-187: LGTM!The validation logic correctly handles both variant alternatives: checking the pointer and its transaction number for the CcRequestBase* case, or directly comparing the transaction number for the uint64_t case. Early returns prevent incorrect cleanup of mismatched transactions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
include/cc/reader_writer_cntl.h (1)
8-8: Consider removing this include to reduce compilation dependencies.Since the
writer_variant only stores aCcRequestBase*(pointer), the forward declaration at line 13 should be sufficient. Including the full header increases compilation dependencies unnecessarily unless the implementation requires the complete type definition for operations beyond pointer storage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
include/cc/reader_writer_cntl.h(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: thweetkomputer
Repo: eloqdata/tx_service PR: 150
File: include/cc/local_cc_shards.h:626-631
Timestamp: 2025-10-09T03:56:58.811Z
Learning: For the LocalCcShards class in include/cc/local_cc_shards.h: Writer locks (unique_lock) should continue using the original meta_data_mux_ (std::shared_mutex) rather than fast_meta_data_mux_ (FastMetaDataMutex) at this stage. Only reader locks may use the FastMetaDataMutex wrapper.
📚 Learning: 2025-10-09T03:56:58.811Z
Learnt from: thweetkomputer
Repo: eloqdata/tx_service PR: 150
File: include/cc/local_cc_shards.h:626-631
Timestamp: 2025-10-09T03:56:58.811Z
Learning: For the LocalCcShards class in include/cc/local_cc_shards.h: Writer locks (unique_lock) should continue using the original meta_data_mux_ (std::shared_mutex) rather than fast_meta_data_mux_ (FastMetaDataMutex) at this stage. Only reader locks may use the FastMetaDataMutex wrapper.
Applied to files:
include/cc/reader_writer_cntl.h
📚 Learning: 2025-09-25T06:15:41.186Z
Learnt from: liunyl
Repo: eloqdata/tx_service PR: 142
File: include/tx_req_result.h:140-159
Timestamp: 2025-09-25T06:15:41.186Z
Learning: When cc_notify_ is true in TxResult, both Finish() and Wait() are guaranteed to be called on the same thread, eliminating concurrency concerns and making relaxed memory ordering sufficient for status_combo_ updates.
Applied to files:
include/cc/reader_writer_cntl.h
MrGuin
force-pushed
the
fix_reader_writer_cntl
branch
from
0288109 to
da0a984
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
include/cc/reader_writer_cntl.h(3 hunks)src/cc/reader_writer_cntl.cpp(7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: thweetkomputer
Repo: eloqdata/tx_service PR: 150
File: include/cc/local_cc_shards.h:626-631
Timestamp: 2025-10-09T03:56:58.811Z
Learning: For the LocalCcShards class in include/cc/local_cc_shards.h: Writer locks (unique_lock) should continue using the original meta_data_mux_ (std::shared_mutex) rather than fast_meta_data_mux_ (FastMetaDataMutex) at this stage. Only reader locks may use the FastMetaDataMutex wrapper.
📚 Learning: 2025-10-09T03:56:58.811Z
Learnt from: thweetkomputer
Repo: eloqdata/tx_service PR: 150
File: include/cc/local_cc_shards.h:626-631
Timestamp: 2025-10-09T03:56:58.811Z
Learning: For the LocalCcShards class in include/cc/local_cc_shards.h: Writer locks (unique_lock) should continue using the original meta_data_mux_ (std::shared_mutex) rather than fast_meta_data_mux_ (FastMetaDataMutex) at this stage. Only reader locks may use the FastMetaDataMutex wrapper.
Applied to files:
src/cc/reader_writer_cntl.cppinclude/cc/reader_writer_cntl.h
📚 Learning: 2025-09-25T06:15:41.186Z
Learnt from: liunyl
Repo: eloqdata/tx_service PR: 142
File: include/tx_req_result.h:140-159
Timestamp: 2025-09-25T06:15:41.186Z
Learning: When cc_notify_ is true in TxResult, both Finish() and Wait() are guaranteed to be called on the same thread, eliminating concurrency concerns and making relaxed memory ordering sufficient for status_combo_ updates.
Applied to files:
src/cc/reader_writer_cntl.cpp
📚 Learning: 2025-10-20T04:30:07.884Z
Learnt from: liunyl
Repo: eloqdata/tx_service PR: 149
File: include/cc/cc_request.h:1876-1927
Timestamp: 2025-10-20T04:30:07.884Z
Learning: ScanNextBatchCc in include/cc/cc_request.h is used only for hash-partition scans; range-partition scans are handled by ScanSliceCc.
Applied to files:
include/cc/reader_writer_cntl.h
🧬 Code graph analysis (1)
src/cc/reader_writer_cntl.cpp (1)
include/cc/reader_writer_cntl.h (1)
ccs_(67-70)
| // If the load precedes the store of writer in AddWriter(), the | ||
| // loaded write request is nullptr. By the time AddWriter() finishes | ||
| // store, the read ref count must be 0 and AddWriter() will return | ||
| // successful. If the load succeeds the store, the last reader and the | ||
| // writer will race to set the write status to ProcessedWriter. Whoever | ||
| // wins will process the write request. | ||
| CcRequestBase *write_req = write_req_.load(std::memory_order_relaxed); | ||
| if (write_req != nullptr) | ||
| uint64_t writer = writer_.load(std::memory_order_relaxed); | ||
| if (IsWriterPtr(writer)) | ||
| { | ||
| cntl = {0, WriteStatus::PendingWriter}; | ||
| if (cntl_block_.compare_exchange_strong( | ||
| cntl, | ||
| {0, WriteStatus::ProcessedWriter}, | ||
| std::memory_order_acq_rel)) | ||
| CcRequestBase *write_req = DecodeWriterPtr(writer); | ||
| if (write_req != nullptr) | ||
| { | ||
| ccs_->Enqueue(write_req); | ||
| cntl = {0, WriteStatus::PendingWriter}; | ||
| if (cntl_block_.compare_exchange_strong( | ||
| cntl, | ||
| {0, WriteStatus::ProcessedWriter}, | ||
| std::memory_order_acq_rel)) | ||
| { | ||
| // Store the writer txn after setting the write status to | ||
| // ProcessedWriter. | ||
| writer_.store(EncodeWriterTxn(write_req->Txn()), | ||
| std::memory_order_relaxed); | ||
| ccs_->Enqueue(write_req); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Use acquire/release on writer_ to prevent stuck writers.
Every load/store of writer_ is memory_order_relaxed. If the final reader races with the AddWriter() store, it can legally see the old 0 value, skip the enqueue path, and return while the control block stays {0, PendingWriter}. Because AddWriter() has already returned WritePending, nothing will promote the request, so the writer remains blocked indefinitely. The C++ memory model explicitly allows this outcome without a happens-before edge between the relaxed operations.
Please promote the hand-off to use release stores and matching acquire loads (FinishReader, both AddWriter branches, FinishWriter, and the txn stores) so the pointer/txn value is visible before the state machine acts on it.
- uint64_t writer = writer_.load(std::memory_order_relaxed);
+ uint64_t writer = writer_.load(std::memory_order_acquire);
...
- writer_.store(EncodeWriterPtr(write_req), std::memory_order_relaxed);
+ writer_.store(EncodeWriterPtr(write_req), std::memory_order_release);
...
- writer_.store(EncodeWriterTxn(write_req->Txn()),
- std::memory_order_relaxed);
+ writer_.store(EncodeWriterTxn(write_req->Txn()),
+ std::memory_order_release);
...
- uint64_t writer = writer_.load(std::memory_order_relaxed);
+ uint64_t writer = writer_.load(std::memory_order_acquire);
...
- writer_.store(0, std::memory_order_relaxed);
+ writer_.store(0, std::memory_order_release);Also applies to: 184-200, 207-235
🤖 Prompt for AI Agents
In src/cc/reader_writer_cntl.cpp around lines 90-115 (also apply same fix to
184-200 and 207-235): the issue is that all loads/stores of writer_ use
memory_order_relaxed which can allow a reader to miss the writer pointer and
leave the control block in PendingWriter indefinitely; change the
synchronization to use release stores and acquire loads so the pointer/txn
becomes visible before state transitions. Concretely: change loads of writer_
that drive state transitions to std::memory_order_acquire, change stores of
writer_ (and txn pointer stores in AddWriter/FinishWriter branches) to
std::memory_order_release, and keep compare_exchange operations with
std::memory_order_acq_rel on success (and std::memory_order_acquire on failure
if two-order parameters are used) so the hand-off has proper happens-before
semantics; apply these exact ordering changes in FinishReader, both AddWriter
branches, FinishWriter, and any txn stores referenced in the noted line ranges.
2 participants
Summary by CodeRabbit