-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
handle_paxos_accept() fails to record a trace message when done with handling #18725
Labels
Backport candidate
backport/5.4
Issues that should be backported to 5.4 branch once they'll be fixed
Milestone
Comments
tchaikov
added a commit
to tchaikov/scylladb
that referenced
this issue
May 17, 2024
…ept() this change is inspired by following warning from clang-tidy ``` Warning: /home/runner/work/scylladb/scylladb/service/storage_proxy.cc:884:13: warning: 'tr_state' used after it was moved [bugprone-use-after-move] 884 | if (tr_state) { | ^ /home/runner/work/scylladb/scylladb/service/storage_proxy.cc:872:139: note: move occurred here 872 | auto f = get_schema_for_read(proposal.update.schema_version(), src_addr, *timeout).then([&sp = _sp, &sys_ks = _sys_ks, tr_state = std::move(tr_state), | ^ ``` this is not a false positive. as `tr_state` is a captured by move for constructing a variable in the captured list of a lambda which is in turn passed to the expression evaluated to `f`. even the expression itself is not evaluated yet when we reference `tr_state` to check if it is empty after preparing the expression, `tr_state` is already moved away into the captured variable. so at that moment, the statement of `f = f.finally(...)` is never evaluated, because `tr_state` is always empty by then. so before this change, the trace message is never recorded. in this change, we address this issue by capturing `tr_state` by copying it. as `tr_state` is backed by a `lw_shared_ptr`, the overhead is neglectable. after this change, the tracing message is recorded. please note, we could coroutinize this function to improve its readability, but since this is a fix and should be backported, let's start with a minimal fix, and worry about the readability in a follow-up change. Fixes scylladb#18725 Signed-off-by: Kefu Chai <[email protected]>
tchaikov
added a commit
to tchaikov/scylladb
that referenced
this issue
May 17, 2024
…ept() this change is inspired by following warning from clang-tidy ``` Warning: /home/runner/work/scylladb/scylladb/service/storage_proxy.cc:884:13: warning: 'tr_state' used after it was moved [bugprone-use-after-move] 884 | if (tr_state) { | ^ /home/runner/work/scylladb/scylladb/service/storage_proxy.cc:872:139: note: move occurred here 872 | auto f = get_schema_for_read(proposal.update.schema_version(), src_addr, *timeout).then([&sp = _sp, &sys_ks = _sys_ks, tr_state = std::move(tr_state), | ^ ``` this is not a false positive. as `tr_state` is a captured by move for constructing a variable in the captured list of a lambda which is in turn passed to the expression evaluated to `f`. even the expression itself is not evaluated yet when we reference `tr_state` to check if it is empty after preparing the expression, `tr_state` is already moved away into the captured variable. so at that moment, the statement of `f = f.finally(...)` is never evaluated, because `tr_state` is always empty by then. so before this change, the trace message is never recorded. in this change, we address this issue by capturing `tr_state` by copying it. as `tr_state` is backed by a `lw_shared_ptr`, the overhead is neglectable. after this change, the tracing message is recorded. please note, we could coroutinize this function to improve its readability, but since this is a fix and should be backported, let's start with a minimal fix, and worry about the readability in a follow-up change. Fixes scylladb#18725 Signed-off-by: Kefu Chai <[email protected]>
tchaikov
added a commit
to tchaikov/scylladb
that referenced
this issue
May 17, 2024
…ept() this change is inspired by following warning from clang-tidy ``` Warning: /home/runner/work/scylladb/scylladb/service/storage_proxy.cc:884:13: warning: 'tr_state' used after it was moved [bugprone-use-after-move] 884 | if (tr_state) { | ^ /home/runner/work/scylladb/scylladb/service/storage_proxy.cc:872:139: note: move occurred here 872 | auto f = get_schema_for_read(proposal.update.schema_version(), src_addr, *timeout).then([&sp = _sp, &sys_ks = _sys_ks, tr_state = std::move(tr_state), | ^ ``` this is not a false positive. as `tr_state` is a captured by move for constructing a variable in the captured list of a lambda which is in turn passed to the expression evaluated to `f`. even the expression itself is not evaluated yet when we reference `tr_state` to check if it is empty after preparing the expression, `tr_state` is already moved away into the captured variable. so at that moment, the statement of `f = f.finally(...)` is never evaluated, because `tr_state` is always empty by then. so before this change, the trace message is never recorded. in this change, we address this issue by capturing `tr_state` by copying it. as `tr_state` is backed by a `lw_shared_ptr`, the overhead is neglectable. after this change, the tracing message is recorded. the change introduced this issue was 548767f. please note, we could coroutinize this function to improve its readability, but since this is a fix and should be backported, let's start with a minimal fix, and worry about the readability in a follow-up change. Refs 548767f Fixes scylladb#18725 Signed-off-by: Kefu Chai <[email protected]>
1 task
tchaikov
added
backport/5.2
backport/5.4
Issues that should be backported to 5.4 branch once they'll be fixed
labels
May 17, 2024
mergify bot
pushed a commit
that referenced
this issue
May 20, 2024
…ept() this change is inspired by following warning from clang-tidy ``` Warning: /home/runner/work/scylladb/scylladb/service/storage_proxy.cc:884:13: warning: 'tr_state' used after it was moved [bugprone-use-after-move] 884 | if (tr_state) { | ^ /home/runner/work/scylladb/scylladb/service/storage_proxy.cc:872:139: note: move occurred here 872 | auto f = get_schema_for_read(proposal.update.schema_version(), src_addr, *timeout).then([&sp = _sp, &sys_ks = _sys_ks, tr_state = std::move(tr_state), | ^ ``` this is not a false positive. as `tr_state` is a captured by move for constructing a variable in the captured list of a lambda which is in turn passed to the expression evaluated to `f`. even the expression itself is not evaluated yet when we reference `tr_state` to check if it is empty after preparing the expression, `tr_state` is already moved away into the captured variable. so at that moment, the statement of `f = f.finally(...)` is never evaluated, because `tr_state` is always empty by then. so before this change, the trace message is never recorded. in this change, we address this issue by capturing `tr_state` by copying it. as `tr_state` is backed by a `lw_shared_ptr`, the overhead is neglectable. after this change, the tracing message is recorded. the change introduced this issue was 548767f. please note, we could coroutinize this function to improve its readability, but since this is a fix and should be backported, let's start with a minimal fix, and worry about the readability in a follow-up change. Refs 548767f Fixes #18725 Signed-off-by: Kefu Chai <[email protected]> (cherry picked from commit a429e7b) # Conflicts: # service/storage_proxy.cc
Draft
1 task
mergify bot
pushed a commit
that referenced
this issue
May 20, 2024
…ept() this change is inspired by following warning from clang-tidy ``` Warning: /home/runner/work/scylladb/scylladb/service/storage_proxy.cc:884:13: warning: 'tr_state' used after it was moved [bugprone-use-after-move] 884 | if (tr_state) { | ^ /home/runner/work/scylladb/scylladb/service/storage_proxy.cc:872:139: note: move occurred here 872 | auto f = get_schema_for_read(proposal.update.schema_version(), src_addr, *timeout).then([&sp = _sp, &sys_ks = _sys_ks, tr_state = std::move(tr_state), | ^ ``` this is not a false positive. as `tr_state` is a captured by move for constructing a variable in the captured list of a lambda which is in turn passed to the expression evaluated to `f`. even the expression itself is not evaluated yet when we reference `tr_state` to check if it is empty after preparing the expression, `tr_state` is already moved away into the captured variable. so at that moment, the statement of `f = f.finally(...)` is never evaluated, because `tr_state` is always empty by then. so before this change, the trace message is never recorded. in this change, we address this issue by capturing `tr_state` by copying it. as `tr_state` is backed by a `lw_shared_ptr`, the overhead is neglectable. after this change, the tracing message is recorded. the change introduced this issue was 548767f. please note, we could coroutinize this function to improve its readability, but since this is a fix and should be backported, let's start with a minimal fix, and worry about the readability in a follow-up change. Refs 548767f Fixes #18725 Signed-off-by: Kefu Chai <[email protected]> (cherry picked from commit a429e7b)
Closed
1 task
denesb
pushed a commit
that referenced
this issue
May 21, 2024
…ept() this change is inspired by following warning from clang-tidy ``` Warning: /home/runner/work/scylladb/scylladb/service/storage_proxy.cc:884:13: warning: 'tr_state' used after it was moved [bugprone-use-after-move] 884 | if (tr_state) { | ^ /home/runner/work/scylladb/scylladb/service/storage_proxy.cc:872:139: note: move occurred here 872 | auto f = get_schema_for_read(proposal.update.schema_version(), src_addr, *timeout).then([&sp = _sp, &sys_ks = _sys_ks, tr_state = std::move(tr_state), | ^ ``` this is not a false positive. as `tr_state` is a captured by move for constructing a variable in the captured list of a lambda which is in turn passed to the expression evaluated to `f`. even the expression itself is not evaluated yet when we reference `tr_state` to check if it is empty after preparing the expression, `tr_state` is already moved away into the captured variable. so at that moment, the statement of `f = f.finally(...)` is never evaluated, because `tr_state` is always empty by then. so before this change, the trace message is never recorded. in this change, we address this issue by capturing `tr_state` by copying it. as `tr_state` is backed by a `lw_shared_ptr`, the overhead is neglectable. after this change, the tracing message is recorded. the change introduced this issue was 548767f. please note, we could coroutinize this function to improve its readability, but since this is a fix and should be backported, let's start with a minimal fix, and worry about the readability in a follow-up change. Refs 548767f Fixes #18725 Signed-off-by: Kefu Chai <[email protected]> (cherry picked from commit a429e7b) Closes #18763
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Backport candidate
backport/5.4
Issues that should be backported to 5.4 branch once they'll be fixed
clang-tidy warns:
this is not a false positive. as
tr_state
is a captured by move for constructing a variable in the captured list of a lambda which is in turn passed to the expression evaluated tof
.even the expression itself is not evaluated yet when we reference
tr_state
to check if it is empty after preparing the expression,tr_state
is already moved away into the captured variable. so at that moment, the statement off = f.finally(...)
is never evaluated, becausetr_state
is always empty by then.so the trace message in the
finally()
block is never recorded.The text was updated successfully, but these errors were encountered: