service/storage_proxy: capture tr_state by copy in handle_paxos_accept()
#18702
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tchaikov
force-pushed
the
tracing-in-handle_paxos_accept
branch
from
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.
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.
tchaikov
force-pushed
the
tracing-in-handle_paxos_accept
branch
2 times, most recently
from
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>
tchaikov
force-pushed
the
tracing-in-handle_paxos_accept
branch
from
902af1e
to
a429e7b
Compare
|
v2:
@gleb-cloudius hi Gleb, thank you for your review. could you take another look? |
tchaikov
added
backport/5.2
tchaikov
changed the title
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:
|
gleb-cloudius
approved these changes
May 20, 2024
This was referenced May 20, 2024
Draft
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this change is inspired by following warning from clang-tidy
this is not a false positive. as
tr_stateis 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_stateto check if it is empty after preparing the expression,tr_stateis already moved away into the captured variable. soat that moment, the statement of
f = f.finally(...)is neverevaluated, because
tr_stateis always empty by then.so before this change, the trace message is never recorded.
in this change, we address this issue by capturing
tr_statebycopying it. as
tr_stateis 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