Provide reason string in forced disconnects to allow Zeek to identify backpressure overflow, plus two fixes #436
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
Neverlord
requested changes
Dec 5, 2024
libbroker/broker/internal/peering.hh
Outdated
Comment on lines
67
to
69
| std::string removed_reason() const noexcept { | ||
| return removed_reason_; | ||
| } |
There was a problem hiding this comment.
This cannot be noexcept because it can thow bad_alloc. However, there's no reason to copy the string in the first place:
Suggested change
| std::string removed_reason() const noexcept { | |
| return removed_reason_; | |
| } | |
| const std::string& removed_reason() const noexcept { | |
| return removed_reason_; | |
| } |
libbroker/broker/internal/peering.cc
Outdated
Comment on lines
125
to
128
| void peering::force_disconnect(const std::string& reason) { | ||
| if (!removed_) { | ||
| removed_ = true; | ||
| removed_reason_ = reason; |
There was a problem hiding this comment.
Always forces copies, better:
Suggested change
| void peering::force_disconnect(const std::string& reason) { | |
| if (!removed_) { | |
| removed_ = true; | |
| removed_reason_ = reason; | |
| void peering::force_disconnect(std::string reason) { | |
| if (!removed_) { | |
| removed_ = true; | |
| removed_reason_ = std::move(reason); |
libbroker/broker/internal/peering.cc
Outdated
Comment on lines
97
to
100
| std::string msg{"removed connection to remote peer"}; | ||
| std::string reason = ptr_->removed_reason(); | ||
| if (!reason.empty()) | ||
| msg += " (" + reason + ")"; |
There was a problem hiding this comment.
This creates a lot of unnecessary temporaries (also should use auto).
Suggested change
| std::string msg{"removed connection to remote peer"}; | |
| std::string reason = ptr_->removed_reason(); | |
| if (!reason.empty()) | |
| msg += " (" + reason + ")"; | |
| auto msg = "removed connection to remote peer"s; | |
| if (const auto& reason = ptr_->removed_reason(); !reason.empty()) { | |
| msg += " ("; | |
| msg += reason; | |
| msg += ')'; | |
| } |
libbroker/broker/internal/peering.hh
Outdated
| @@ -38,7 +38,7 @@ public: | |||
|
|
|||
| /// Forces the peering to shut down its connection without performing the BYE | |||
| /// handshake. | |||
| void force_disconnect(); | |||
| void force_disconnect(const std::string& reason = ""); | |||
There was a problem hiding this comment.
Suggested change
| void force_disconnect(const std::string& reason = ""); | |
| void force_disconnect(std::string reason); |
Not a fan of the empty default. There are two more places where we call force_disconnect that would currently use the default:
core_actor_state::finalize_shutdown: can pass something like "shutting down"peering::schedule_bye_timeout: can pass something like "timeout during graceful disconnect"
I think differentiating those with dedicated reasons would make sense as well.
There was a problem hiding this comment.
Yeah I was just trying to keep this minimal. I like your suggestions, will do. Thanks!
ckreibich
force-pushed
the
topic/christian/disconnect-slow-peers
branch
from
ecb9aea to
41bf6ea
Compare
Neverlord
requested changes
Dec 6, 2024
libbroker/broker/internal/peering.cc
Outdated
| @@ -119,18 +126,20 @@ void peering::on_bye_ack() { | |||
| bye_timeout_.dispose(); | |||
| } | |||
|
|
|||
| void peering::force_disconnect() { | |||
| void peering::force_disconnect(const std::string reason) { | |||
There was a problem hiding this comment.
Suggested change
| void peering::force_disconnect(const std::string reason) { | |
| void peering::force_disconnect(std::string reason) { |
Adding const here turns the move below to a copy.
libbroker/broker/internal/peering.hh
Outdated
| @@ -38,7 +38,7 @@ public: | |||
|
|
|||
| /// Forces the peering to shut down its connection without performing the BYE | |||
| /// handshake. | |||
| void force_disconnect(); | |||
| void force_disconnect(const std::string reason); | |||
There was a problem hiding this comment.
This is a sink argument.
Suggested change
| void force_disconnect(const std::string reason); | |
| void force_disconnect(std::string reason); |
There was a problem hiding this comment.
D'oh, these were silly — thank you. (And, you're welcome to commit to my branches in such cases!)
ckreibich
force-pushed
the
topic/christian/disconnect-slow-peers
branch
from
41bf6ea to
94e93c4
Compare
This includes providing a reason for the new backpressure overflow, so Zeek can pick up on them.
When connect_manager::continue_reading() encounters a read_result::stop, it clears the read mask, indicating no more reads should happen, but this flag isn't checked in connector::run_impl()'s while loop. This could lead to infinite 100% CPU utilization in a tight loop. We now simply consult the read mask in addition to the other checks.
ckreibich
force-pushed
the
topic/christian/disconnect-slow-peers
branch
from
94e93c4 to
e2d1d41
Compare
|
Rebased once more just to get it to run with latest CI updates. |
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.
@Neverlord could you let me know if this looks okay to you? The reason-string support should hopefully be straightforward.
More interesting is ecb9aea, which I encountered while testing cluster recovery after forced disconnects: around half of the time the while-loop in
connector::run_impl()ended up in a situation where it kept trying to read but failed, without realizing it had encountered an error. I'm simply adding a check of the requested-event poll mask, which seems to be resolving it.I also noticed that Broker's master currently doesn't build with Zeek. I narrowed this down to a part of 4d9aa3b, which I'm simply reverting here. It doesn't seem to have any impact on clang-tidy, but let me know if I'm missing anything.
Thanks!