Improved removal of dead network channels #3993
Merged
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
pwojcikdev
force-pushed
the
dead-channels
branch
from
ef895f0 to
d225eb2
Compare
pwojcikdev
force-pushed
the
dead-channels
branch
from
d225eb2 to
618052a
Compare
dsiganos
reviewed
Nov 10, 2022
nano/node/network.cpp
Outdated
| @@ -769,7 +769,7 @@ void nano::network::ongoing_cleanup () | |||
| { | |||
| cleanup (std::chrono::steady_clock::now () - node.network_params.network.cleanup_cutoff ()); | |||
| std::weak_ptr<nano::node> node_w (node.shared ()); | |||
| node.workers.add_timed_task (std::chrono::steady_clock::now () + node.network_params.network.cleanup_period, [node_w] () { | |||
| node.workers.add_timed_task (std::chrono::steady_clock::now () + std::chrono::seconds (1), [node_w] () { | |||
There was a problem hiding this comment.
What is happening to cleanup_period, are we dropping its use?
Isn't checking all sockets every second a bit excessive?
There was a problem hiding this comment.
The cleanup period was set to 60 seconds by default and is still used in multiple different contexts, with interdependencies not being very clear. Here we just iterate all sockets (~1000 max by default) every second to see if any of them are dead. In the grand scheme of things this iteration is a small blip on the profiler chart.
There was a problem hiding this comment.
But 1 second might be a bit excessive, adjusting it to 5 seconds should not negatively impact the fixes in any way.
dsiganos
previously approved these changes
Nov 10, 2022
Use lower value for dev network to speed up timeout unit tests
dsiganos
approved these changes
Nov 10, 2022
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 PR improves eviction of channels when their underlying socket becomes dead (due to graceful shutdown or network error). Before we relied only on timeout to purge those, which was suboptimal, because even when socket was known to be dead we still waited ~5 minutes. That prevented us from quickly reestablishing connections to peers when problems occurred.