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.

Sign up for GitHub

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

Fix bug in commit_ret_elems which lead to "live leak" in case of requests batch. #278

Merged
merged 3 commits into from
Jan 26, 2022
Merged

Fix bug in commit_ret_elems which lead to "live leak" in case of requests batch. #278

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b573eec
Merge pull request #1 from eBay/master
alesapin Apr 7, 2021
6f26688
Merge pull request #2 from eBay/master
alesapin Jan 22, 2022
f54c04d
Merge pull request #36 from ClickHouse-Extras/fix_handle_commit_batch
alesapin Jan 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions src/handle_commit.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,14 +300,27 @@ void raft_server::commit_app_log(ulong idx_to_commit,
std::list< ptr<commit_ret_elem> > async_elems;
if (need_to_handle_commit_elem) {
std::unique_lock<std::mutex> cre_lock(commit_ret_elems_lock_);
bool match_found = false;
/// Sometimes user can batch requests to RAFT: for example send 30
/// append entries requests in a single batch. For such request batch
/// user will receive a single response: all was successful or all
/// failed. Obviously we don't need to add info about responses
/// (commit_ret_elems) for 29 requests from batch and need to do it only
/// for 30-th request. precommit_index is exact value which identify ID
/// of the last request from the latest batch. So if we commiting this
/// last request and for some reason it was not added into
/// commit_ret_elems in the handle_cli_req method (logical race
/// condition) we have to add it here. Otherwise we don't need to add
/// anything into commit_ret_elems_, because nobody will wait for the
/// responses of the intermediate requests from requests batch.
bool need_to_check_commit_ret = sm_idx == pc_idx;
Copy link
Contributor

@greensky00 greensky00 Jan 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for this fix. To make sure if I understand this condition correctly, can you confirm the below?

Let's say the current committed index is 10 (for all pre-commit, quick-commit, and sm-commit), and two threads (T1 and T2) are calling handle_cli_req with exactly one log. What will happen is:

  1. T1 calls handle_cli_req first, precommit_index_ becomes 11.
  2. Before T1 creates the corresponding commit_ret_elem, the commit thread wakes up earlier.
  3. Since sm_idx == pc_idx in this case, the commit thread will create it.
  4. However, T2 cannot interfere before step (3), because of the lock held by handle_cli_req_prelock. T2 can append its log only after the commit_ret_elem for 11 is created.

So, if a thread order inversion happens, only one user thread is involved at a time. When the commit thread wakes up, there is no case that there are multiple handle_cli_req calls whose commit_ret_elems are not created yet. Is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I didn't think about such case, but yes handle_cli_req guarded by mutex (or recursive mutex for some reason...) in handle_cli_req_prelock: https://github.com/ClickHouse-Extras/NuRaft/blob/1707a7572aa66ec5d0a2dbe2bf5effa3352e6b2d/src/handle_client_request.cxx#L40-L53.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW I cannot catch "thread inversion" even under heavy load. But according to the code it should be possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, let me merge it.


auto entry = commit_ret_elems_.find(sm_idx);
if (entry != commit_ret_elems_.end()) {
ptr<commit_ret_elem> elem = entry->second;
if (elem->idx_ == sm_idx) {
elem->result_code_ = cmd_result_code::OK;
elem->ret_value_ = ret_value;
match_found = true;
need_to_check_commit_ret = false;
p_dv("notify cb %ld %p", sm_idx, &elem->awaiter_);

switch (ctx_->get_params()->return_method_) {
Expand All @@ -326,7 +339,7 @@ void raft_server::commit_app_log(ulong idx_to_commit,
}
}

if (!match_found) {
if (need_to_check_commit_ret) {
// If not found, commit thread is invoked earlier than user thread.
// Create one here.
ptr<commit_ret_elem> elem = cs_new<commit_ret_elem>();
Expand Down