fix: don't keepalive when the connection is busy #16
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
We wouldn't permanently leak them, but we'd leak them for the write timeout (10s).
* It's a waste of bandwidth. * It causes us to think the connection is dead because the other side is too busy sending us _other_ data to respond to the ping. fixes #15
Stebalien
commented
Mar 13, 2020
| // peer is busy sending us stuff, the pong might get stuck | ||
| // behind a bunch of data. | ||
| if s.keepaliveTimer != nil { | ||
| s.keepaliveTimer.Reset(s.config.KeepAliveInterval) |
There was a problem hiding this comment.
Don't you need to check if the timer has already been read as described in https://golang.org/pkg/time/#Timer.Reset?
There was a problem hiding this comment.
this one gets me every time too -- the API for resetting timers is really poor.
There was a problem hiding this comment.
Not in this case. Reset actually stops it internally.
You have to stop it first and drain the channel if you want to make sure that the previous timer doesn't fire after you reset it but before new timeout elapses. However:
- We don't really care in this case.
- This isn't a normal timer, it's an AfterFunc and doesn't have a channel to drain. There's no way to reliably stop an AfterFunc because the function is called asynchronously. Note: trying to drain a channel of a timer created by
AfterFuncwill just block forever.
raulk
approved these changes
Mar 13, 2020
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.
fixes #15