-
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
service/storage_proxy: capture tr_state
by copy in handle_paxos_accept()
#18702
Conversation
1c147ee
to
3a818bb
Compare
cc @raphaelsc |
🟢 CI State: SUCCESS✅ - Build Build Details:
|
If it fixes a real bug, please first provide a minimal fix (with a related bug). Later we can evaluate if coroutinization has merits (likely it does). |
Ah, I see it's for a trace message. Still, tracing is important and a missing message could be confusing. |
service/storage_proxy.cc
Outdated
f = f.finally([tr_state, src_ip] { | ||
tracing::trace(tr_state, "paxos_accept: handling is done, sending a response to /{}", src_ip); | ||
}); | ||
tracing::trace(tr_state, "paxos_accept: handling is done, sending a response to /{}", src_ip); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original code used finally
meaning it was trying to trace even in the case of an exception. This new code will not add a trace in case of an exception.
7643f53
to
902af1e
Compare
…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 <kefu.chai@scylladb.com>
902af1e
to
a429e7b
Compare
v2:
@gleb-cloudius hi Gleb, thank you for your review. could you take another look? |
tr_state
by copy in handle_paxos_accept()
🔴 CI State: FAILURE✅ - Build Failed Tests (345/31104):
Build Details:
|
i think it's an infra issue. |
Damn I suspect it could be related to recent change in test.py -- 734c5de @temichus please take look, it could be that we're leaking IP addresses somewhere, perhaps releasing them too early after this change :( |
🟢 CI State: SUCCESS✅ - Build Build Details:
|
this change is inspired by following warning from clang-tidy
this is not a false positive. as
tr_state
is a captured by move forconstructing 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. soat that moment, the statement of
f = f.finally(...)
is neverevaluated, 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
bycopying it. as
tr_state
is backed by alw_shared_ptr
, the overhead isneglectable.
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.
Refs 548767f
Fixes #18725