Skip to content

Commit

Permalink
fix: avoid race conditions
Browse files Browse the repository at this point in the history
These race conditions may seem harmless, and maybe they are unlikely to
cause real trouble, but:

 1. There is a theoretical chance they do. A partially written time
    value in one goroutine may result in an invalid (and very wrong)
    time in another, leading to misbehaving code.
 2. They introduce noise, and make it more difficult to find&fix other
    race conditions.
  • Loading branch information
aykevl authored and devgianlu committed Sep 10, 2024
1 parent efe81ed commit 9dcbd18
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 4 deletions.
10 changes: 8 additions & 2 deletions ap/ap.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ type Accesspoint struct {
recvChans map[PacketType][]chan Packet
recvChansLock sync.RWMutex
lastPongAck time.Time
lastPongAckLock sync.Mutex

// connMu is held for writing when performing reconnection and for reading mainly when accessing welcome
// or sending packets. If it's not held, a valid connection (and APWelcome) is available. Be careful not to deadlock
Expand Down Expand Up @@ -274,7 +275,9 @@ loop:
}
case PacketTypePongAck:
log.Tracef("received accesspoint pong ack")
ap.lastPongAckLock.Lock()
ap.lastPongAck = time.Now()
ap.lastPongAckLock.Unlock()
continue
default:
ap.recvChansLock.RLock()
Expand Down Expand Up @@ -334,8 +337,11 @@ loop:
case <-ap.pongAckTickerStop:
break loop
case <-ticker.C:
if time.Since(ap.lastPongAck) > pongAckInterval {
log.Errorf("did not receive last pong ack from accesspoint, %.0fs passed", time.Since(ap.lastPongAck).Seconds())
ap.lastPongAckLock.Lock()
timePassed := time.Since(ap.lastPongAck)
ap.lastPongAckLock.Unlock()
if timePassed > pongAckInterval {
log.Errorf("did not receive last pong ack from accesspoint, %.0fs passed", timePassed.Seconds())

// closing the connection should make the read on the "recvLoop" fail,
// continue hoping for a new connection
Expand Down
12 changes: 10 additions & 2 deletions dealer/dealer.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type Dealer struct {
recvLoopStop chan struct{}
recvLoopOnce sync.Once
lastPong time.Time
lastPongLock sync.Mutex

// connMu is held for writing when performing reconnection and for reading when accessing the conn.
// If it's not held, a valid connection is available. Be careful not to deadlock anything with this.
Expand Down Expand Up @@ -126,8 +127,11 @@ loop:
case <-d.pingTickerStop:
break loop
case <-ticker.C:
if time.Since(d.lastPong) > pingInterval+timeout {
log.Errorf("did not receive last pong from dealer, %.0fs passed", time.Since(d.lastPong).Seconds())
d.lastPongLock.Lock()
timePassed := time.Since(d.lastPong)
d.lastPongLock.Unlock()
if timePassed > pingInterval+timeout {
log.Errorf("did not receive last pong from dealer, %.0fs passed", timePassed.Seconds())

// closing the connection should make the read on the "recvLoop" fail,
// continue hoping for a new connection
Expand Down Expand Up @@ -200,7 +204,9 @@ loop:
// we never receive ping messages
break
case "pong":
d.lastPongLock.Lock()
d.lastPong = time.Now()
d.lastPongLock.Unlock()
log.Tracef("received dealer pong")
break
default:
Expand Down Expand Up @@ -267,7 +273,9 @@ func (d *Dealer) reconnect() error {
return err
}

d.lastPongLock.Lock()
d.lastPong = time.Now()
d.lastPongLock.Unlock()
// restart the recv loop
go d.recvLoop()

Expand Down

0 comments on commit 9dcbd18

Please sign in to comment.