-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Adjust ping parameters to improve tor stability #8762
Adjust ping parameters to improve tor stability #8762
Conversation
Important Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
I ran this and it does indeed fix the problem I was seeing. Thank you! |
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.
lgtm! 🚢
1f74bd1
to
a766f91
Compare
I'm running this now, and so far so good! Will report back in the morning! |
14c58b8
to
fc6b65f
Compare
fc6b65f
to
5d82b6a
Compare
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.
LGTM 🐠
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.
LGTM ⚡, with a nit (+ the helper function)
// when the PingManager is running. | ||
func (m *PingManager) Stop() error { | ||
// Stop interrupts the goroutines that the PingManager owns. | ||
func (m *PingManager) Stop() { | ||
if m.pingTicker == nil { |
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.
q: Do we still need ths conditional here? Would it ever be hit?
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.
It turns out, yes. Testing this code reveals that if there is a timeout during the init process of the Brontide, then we call Disconnect
before the ping manager is ever started. Since Start
is what initializes the Timer
this will be nil at that point.
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.
Nice work. left some nits and qs
peer/brontide.go
Outdated
@@ -1258,15 +1258,13 @@ func (p *Brontide) Disconnect(reason error) { | |||
|
|||
p.log.Infof(err.Error()) | |||
|
|||
// Stop ping manager before closing TCP connection |
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.
nit: missing full stop
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.
Fwiw, as far as I can tell, the full-stop rule is only important for godocs since it affects the godoc rendering semantics. In-line comments like this aren't rendered and so I don't think it matters, but others may know things that I don't.
5d82b6a
to
1148e57
Compare
Not sure what to make of the |
Appears to be a known flake. |
Change Description
Here we do the following things to increase stability with tor peers:
init
message.Steps to Test
Run this patch with a bunch of tor peers and see if it improves the long term stability of those peers.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.