Skip to content

Commit

Permalink
improve disconnect and retry interactions.
Browse files Browse the repository at this point in the history
RTM unconditionally slept for the entire duration
of the backoff, ignoring any disconnect signals
sent by the library or calling applications.

this leads to unnecessary and excessive delays when
invoking Disconnect().

- adds error details to the disconnect events for visibility.
  • Loading branch information
James Lawrence authored and james-lawrence committed Aug 31, 2019
1 parent 972c820 commit 9966997
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 16 deletions.
1 change: 1 addition & 0 deletions websocket_internals.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ type ConnectingEvent struct {
// DisconnectedEvent contains information about how we disconnected
type DisconnectedEvent struct {
Intentional bool
Cause error
}

// LatencyReport contains information about connection latency
Expand Down
32 changes: 16 additions & 16 deletions websocket_managed_conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"time"

"github.com/gorilla/websocket"
"github.com/nlopes/slack/internal/errorsx"
"github.com/nlopes/slack/internal/timex"
)

Expand Down Expand Up @@ -138,21 +139,22 @@ func (rtm *RTM) connect(connectionCount int, useRTMStart bool) (*Info, *websocke
ErrorObj: err,
}}

// check if Disconnect() has been invoked.
// get time we should wait before attempting to connect again
rtm.Debugf("reconnection %d failed: %s reconnecting in %v\n", boff.attempts, err, backoff)

// wait for one of the following to occur,
// backoff duration has elapsed, killChannel is signalled, or
// the rtm finishes disconnecting.
select {
case <-time.After(backoff): // retry after the backoff.
case intentional := <-rtm.killChannel:
if intentional {
rtm.killConnection(intentional)
rtm.killConnection(intentional, ErrRTMDisconnected)
return nil, nil, ErrRTMDisconnected
}
case <-rtm.disconnected:
return nil, nil, ErrRTMDisconnected
default:
}

// get time we should wait before attempting to connect again
rtm.Debugf("reconnection %d failed: %s reconnecting in %v\n", boff.attempts, err, backoff)
time.Sleep(backoff)
}
}

Expand Down Expand Up @@ -205,14 +207,14 @@ func (rtm *RTM) startRTMAndDial(useRTMStart bool) (info *Info, _ *websocket.Conn
//
// This should not be called directly! Instead a boolean value (true for
// intentional, false otherwise) should be sent to the killChannel on the RTM.
func (rtm *RTM) killConnection(intentional bool) (err error) {
func (rtm *RTM) killConnection(intentional bool, cause error) (err error) {
rtm.Debugln("killing connection")

if rtm.conn != nil {
err = rtm.conn.Close()
}

rtm.IncomingEvents <- RTMEvent{"disconnected", &DisconnectedEvent{intentional}}
rtm.IncomingEvents <- RTMEvent{"disconnected", &DisconnectedEvent{Intentional: intentional, Cause: cause}}

if intentional {
rtm.disconnect()
Expand All @@ -233,22 +235,21 @@ func (rtm *RTM) handleEvents() {
select {
// catch "stop" signal on channel close
case intentional := <-rtm.killChannel:
_ = rtm.killConnection(intentional)
_ = rtm.killConnection(intentional, errorsx.String("signaled"))
return
// detect when the connection is dead.
case <-rtm.pingDeadman.C:
rtm.Debugln("deadman switch trigger disconnecting")
_ = rtm.killConnection(false)
_ = rtm.killConnection(false, errorsx.String("deadman switch triggered"))
return
// send pings on ticker interval
case <-ticker.C:
if err := rtm.ping(); err != nil {
_ = rtm.killConnection(false)
_ = rtm.killConnection(false, err)
return
}
case <-rtm.forcePing:
if err := rtm.ping(); err != nil {
_ = rtm.killConnection(false)
_ = rtm.killConnection(false, err)
return
}
// listen for messages that need to be sent
Expand All @@ -258,7 +259,7 @@ func (rtm *RTM) handleEvents() {
case rawEvent := <-rtm.rawEvents:
switch rtm.handleRawEvent(rawEvent) {
case rtmEventTypeGoodbye:
_ = rtm.killConnection(false)
_ = rtm.killConnection(false, errorsx.String("goodbye detected"))
return
default:
}
Expand Down Expand Up @@ -310,7 +311,6 @@ func (rtm *RTM) sendOutgoingMessage(msg OutgoingMessage) {
Message: msg,
ErrorObj: err,
}}
// TODO force ping?
}
}

Expand Down

0 comments on commit 9966997

Please sign in to comment.