Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: don't keepalive when the connection is busy #16

Merged
merged 3 commits into from
Mar 13, 2020
Merged

fix: don't keepalive when the connection is busy #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
888369a
chore: avoid leaking ping timers
Stebalien Mar 13, 2020
bddc294
fix: don't keepalive when the connection is busy
Stebalien Mar 13, 2020
922d36f
ci: configure travis
Stebalien Mar 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 29 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
os:
- linux

language: go

go:
- 1.14.x

env:
global:
- GOTFLAGS="-race"
- BUILD_DEPTYPE=gomod


# disable travis install
install:
- true

script:
- bash <(curl -s https://raw.githubusercontent.com/ipfs/ci-helpers/master/travis-ci/run-standard-tests.sh)


cache:
directories:
- $GOPATH/pkg/mod
- $HOME/.cache/go-build

notifications:
email: false
14 changes: 12 additions & 2 deletions session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,9 +304,11 @@ func (s *Session) Ping() (time.Duration, error) {

// Wait for a response
start := time.Now()
timer := time.NewTimer(s.config.ConnectionWriteTimeout)
defer timer.Stop()
select {
case <-ch:
case <-time.After(s.config.ConnectionWriteTimeout):
case <-timer.C:
s.pingLock.Lock()
delete(s.pings, id) // Ignore it if a response comes later.
s.pingLock.Unlock()
Expand All @@ -316,7 +318,7 @@ func (s *Session) Ping() (time.Duration, error) {
}

// Compute the RTT
return time.Now().Sub(start), nil
return time.Since(start), nil
}

// startKeepalive starts the keepalive process.
Expand Down Expand Up @@ -506,6 +508,14 @@ func (s *Session) recvLoop() error {
return err
}

// Reset the keepalive timer every time we receive data.
// There's no reason to keepalive if we're active. Worse, if the
// 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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the bugfix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need to check if the timer has already been read as described in https://golang.org/pkg/time/#Timer.Reset?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one gets me every time too -- the API for resetting timers is really poor.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. We don't really care in this case.
  2. 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 AfterFunc will just block forever.

}

// Verify the version
if hdr.Version() != protoVersion {
s.logger.Printf("[ERR] yamux: Invalid protocol version: %d", hdr.Version())
Expand Down