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 for Issue 4014: simplified ping handling / deferral based on rece… #4015

Closed
wants to merge 1 commit into from
Closed

Fix for Issue 4014: simplified ping handling / deferral based on rece… #4015

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 13 additions & 22 deletions server/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,9 +286,10 @@ type rrTracking struct {

// Struct for PING initiation from the server.
type pinfo struct {
tmr *time.Timer
last time.Time
out int
tmr *time.Timer
lastRxPing time.Time
Copy link
Member

Choose a reason for hiding this comment

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

Can these not be combined? I think they are doing the same which is recording inbound activity only.

lastRxMsg time.Time
out int
}

// outbound holds pending data for a socket.
Expand Down Expand Up @@ -1309,6 +1310,7 @@ func (c *client) readLoop(pre []byte) {
// Activity based on interest changes or data/msgs.
if c.in.msgs > 0 || c.in.subs > 0 {
c.last = last
c.ping.lastRxMsg = last
}

if n >= cap(b) {
Expand Down Expand Up @@ -2196,7 +2198,7 @@ func (c *client) processPing() {

// Record this to suppress us sending one if this
// is within a given time interval for activity.
c.ping.last = time.Now()
c.ping.lastRxPing = time.Now()

// If not a CLIENT, we are done. Also the CONNECT should
// have been received, but make sure it is so before proceeding
Expand Down Expand Up @@ -4555,32 +4557,21 @@ func (c *client) processPingTimer() {

c.Debugf("%s Ping Timer", c.kindString())

var sendPing bool

pingInterval := c.srv.getOpts().PingInterval
if c.kind == GATEWAY {
pingInterval = adjustPingIntervalForGateway(pingInterval)
}
now := time.Now()
needRTT := c.rtt == 0 || now.Sub(c.rttStart) > DEFAULT_RTT_MEASUREMENT_INTERVAL

// Do not delay PINGs for GATEWAY or spoke LEAF connections.
if c.kind == GATEWAY || c.isSpokeLeafNode() {
Copy link
Member

Choose a reason for hiding this comment

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

Doing the write is most important over potential long haul or multi-hope networks. So even if we have received something we may need to write, so prefer this logic remains.

sendPing = true
// If we have had receive activity within the PingInterval then
// there is no need to send a ping. This can be client data
// or if we received a ping from the other side.
if delta := now.Sub(c.ping.lastRxMsg); delta < pingInterval && !needRTT {
c.Debugf("Delaying PING due to receive traffic %v ago", delta.Round(time.Second))
} else if delta := now.Sub(c.ping.lastRxPing); delta < pingInterval && !needRTT {
c.Debugf("Delaying PING due to remote ping %v ago", delta.Round(time.Second))
} else {
// If we have had activity within the PingInterval then
// there is no need to send a ping. This can be client data
// or if we received a ping from the other side.
if delta := now.Sub(c.last); delta < pingInterval && !needRTT {
c.Debugf("Delaying PING due to client activity %v ago", delta.Round(time.Second))
} else if delta := now.Sub(c.ping.last); delta < pingInterval && !needRTT {
c.Debugf("Delaying PING due to remote ping %v ago", delta.Round(time.Second))
} else {
sendPing = true
}
}

if sendPing {
// Check for violation
if c.ping.out+1 > c.srv.getOpts().MaxPingsOut {
c.Debugf("Stale Client Connection - Closing")
Expand Down