Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove std::move() in async_alloc_write. #347

Merged
merged 1 commit into from
Mar 9, 2024

Conversation

xiaoxichen
Copy link
Collaborator

Previously we do move() for rreq which

  1. has no effect on write_with_data path as rreq is captured by value which is a const.
  2. make rreq (intrusive_ptr) to NULL after move and causing panic in handle_error(rreq) later.

Removing the move() to fix the bug, though with a cost of extra atomic_inc/dec

Previously we do move() for rreq which
1. has no effect on write_with_data path as rreq is captured by value
   which is a const.
2. make rreq (intrusive_ptr) to NULL after move and causing panic in
   handle_error(rreq) later.

Removing the move() to fix the bug, though with a cost of extra atomic_inc/dec

Signed-off-by: Xiaoxi Chen <[email protected]>
@xiaoxichen xiaoxichen requested a review from hkadayam March 8, 2024 16:12
@@ -121,7 +121,7 @@ void RaftReplDev::async_alloc_write(sisl::blob const& header, sisl::blob const&
// Write the data
data_service().async_write(rreq->value, rreq->local_blkid).thenValue([this, rreq](auto&& err) {
if (!err) {
auto raft_status = m_state_machine->propose_to_raft(std::move(rreq));
Copy link
Collaborator Author

@xiaoxichen xiaoxichen Mar 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the rreq is a const so actually the move is not happening, that is the reason the handle_error in L125 can pass

@xiaoxichen xiaoxichen requested a review from yamingk March 8, 2024 16:13
@xiaoxichen xiaoxichen merged commit 239ccb5 into eBay:master Mar 9, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants