From b5cbeb4ad788f606df1aa2e61853383e99267c2f Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Mon, 15 Jan 2024 13:25:27 -0800 Subject: [PATCH 1/3] peer: make PingManager disconnect call async In this commit, we make all calls to disconnect after a ping/pong violation is detected in the `PingManager` async. We do this to avoid circular waiting that may occur if the disconnect call back ends up waiting on the peer goroutine to be torn down. If this happens, then the peer goroutine will be blocked on the ping manager fully tearing down, which is blocked on the peer disconnect succeeding. This is a similar class of issue we've delt with recently as pertains to the peer and the server: sync all back execution must not lead to a circular waiting loop. Fixes #8379 --- peer/brontide.go | 2 +- peer/ping_manager.go | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/peer/brontide.go b/peer/brontide.go index 1611ff633..a97c1f438 100644 --- a/peer/brontide.go +++ b/peer/brontide.go @@ -577,7 +577,7 @@ func NewBrontide(cfg Config) *Brontide { eStr := "pong response failure for %s: %v " + "-- disconnecting" p.log.Warnf(eStr, p, err) - p.Disconnect(fmt.Errorf(eStr, p, err)) + go p.Disconnect(fmt.Errorf(eStr, p, err)) }, }) diff --git a/peer/ping_manager.go b/peer/ping_manager.go index 3e62cb097..b840d47c4 100644 --- a/peer/ping_manager.go +++ b/peer/ping_manager.go @@ -13,7 +13,6 @@ import ( // PingManagerConfig is a structure containing various parameters that govern // how the PingManager behaves. type PingManagerConfig struct { - // NewPingPayload is a closure that returns the payload to be packaged // in the Ping message. NewPingPayload func() []byte @@ -144,6 +143,7 @@ func (m *PingManager) start() error { // Set up our bookkeeping for the new Ping. if err := m.setPingState(pongSize); err != nil { + m.cfg.OnPongFailure(err) return @@ -157,6 +157,7 @@ func (m *PingManager) start() error { e := errors.New("timeout while waiting for " + "pong response", ) + m.cfg.OnPongFailure(e) return @@ -177,6 +178,7 @@ func (m *PingManager) start() error { e := errors.New("pong response does " + "not match expected size", ) + m.cfg.OnPongFailure(e) return @@ -188,6 +190,7 @@ func (m *PingManager) start() error { rtt := time.Since(*lastPing) m.pingTime.Store(&rtt) } + case <-m.quit: return } From 185119f5c347c01e110f435029b4f2b0dbf403df Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Mon, 15 Jan 2024 13:33:43 -0800 Subject: [PATCH 2/3] peer: refactor main event loop for ping handler The error was never used as the init couldn't return an error, so we do away with that. We also modify the main event loop dispatch to more closely match other areas of the codebase. --- peer/ping_manager.go | 142 +++++++++++++++++++++---------------------- 1 file changed, 69 insertions(+), 73 deletions(-) diff --git a/peer/ping_manager.go b/peer/ping_manager.go index b840d47c4..c456a0581 100644 --- a/peer/ping_manager.go +++ b/peer/ping_manager.go @@ -98,16 +98,20 @@ func NewPingManager(cfg *PingManagerConfig) *PingManager { func (m *PingManager) Start() error { var err error m.started.Do(func() { - err = m.start() + m.pingTicker = time.NewTicker(m.cfg.IntervalDuration) + m.pingTimeout = time.NewTimer(0) + + m.wg.Add(1) + go m.pingHandler() }) return err } -func (m *PingManager) start() error { - m.pingTicker = time.NewTicker(m.cfg.IntervalDuration) - - m.pingTimeout = time.NewTimer(0) +// pingHandler is the main goroutine responsible for enforcing the ping/pong +// protocol. +func (m *PingManager) pingHandler() { + defer m.wg.Done() defer m.pingTimeout.Stop() // Ensure that the pingTimeout channel is empty. @@ -115,89 +119,81 @@ func (m *PingManager) start() error { <-m.pingTimeout.C } - m.wg.Add(1) - go func() { - defer m.wg.Done() - for { - select { - case <-m.pingTicker.C: - // If this occurs it means that the new ping - // cycle has begun while there is still an - // outstanding ping awaiting a pong response. - // This should never occur, but if it does, it - // implies a timeout. - if m.outstandingPongSize >= 0 { - e := errors.New("impossible: new ping" + - "in unclean state", - ) - m.cfg.OnPongFailure(e) + for { + select { + case <-m.pingTicker.C: + // If this occurs it means that the new ping cycle has + // begun while there is still an outstanding ping + // awaiting a pong response. This should never occur, + // but if it does, it implies a timeout. + if m.outstandingPongSize >= 0 { + e := errors.New("impossible: new ping" + + "in unclean state", + ) + m.cfg.OnPongFailure(e) - return - } + return + } - pongSize := m.cfg.NewPongSize() - ping := &lnwire.Ping{ - NumPongBytes: pongSize, - PaddingBytes: m.cfg.NewPingPayload(), - } + pongSize := m.cfg.NewPongSize() + ping := &lnwire.Ping{ + NumPongBytes: pongSize, + PaddingBytes: m.cfg.NewPingPayload(), + } - // Set up our bookkeeping for the new Ping. - if err := m.setPingState(pongSize); err != nil { + // Set up our bookkeeping for the new Ping. + if err := m.setPingState(pongSize); err != nil { + m.cfg.OnPongFailure(err) - m.cfg.OnPongFailure(err) + return + } - return - } + m.cfg.SendPing(ping) - m.cfg.SendPing(ping) + case <-m.pingTimeout.C: + m.resetPingState() - case <-m.pingTimeout.C: - m.resetPingState() + e := errors.New("timeout while waiting for " + + "pong response", + ) - e := errors.New("timeout while waiting for " + - "pong response", + m.cfg.OnPongFailure(e) + + return + + case pong := <-m.pongChan: + pongSize := int32(len(pong.PongBytes)) + + // Save off values we are about to override when we + // call resetPingState. + expected := m.outstandingPongSize + lastPing := m.pingLastSend + + m.resetPingState() + + // If the pong we receive doesn't match the ping we + // sent out, then we fail out. + if pongSize != expected { + e := errors.New("pong response does " + + "not match expected size", ) m.cfg.OnPongFailure(e) return - - case pong := <-m.pongChan: - pongSize := int32(len(pong.PongBytes)) - - // Save off values we are about to override - // when we call resetPingState. - expected := m.outstandingPongSize - lastPing := m.pingLastSend - - m.resetPingState() - - // If the pong we receive doesn't match the - // ping we sent out, then we fail out. - if pongSize != expected { - e := errors.New("pong response does " + - "not match expected size", - ) - - m.cfg.OnPongFailure(e) - - return - } - - // Compute RTT of ping and save that for future - // querying. - if lastPing != nil { - rtt := time.Since(*lastPing) - m.pingTime.Store(&rtt) - } - - case <-m.quit: - return } - } - }() - return nil + // Compute RTT of ping and save that for future + // querying. + if lastPing != nil { + rtt := time.Since(*lastPing) + m.pingTime.Store(&rtt) + } + + case <-m.quit: + return + } + } } // Stop interrupts the goroutines that the PingManager owns. Can only be called From 359f2715414cd478d19250522a29ae3b68f2b15c Mon Sep 17 00:00:00 2001 From: Keagan McClelland Date: Wed, 17 Jan 2024 18:03:35 -0800 Subject: [PATCH 3/3] docs: update release notes --- docs/release-notes/release-notes-0.17.4.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/release-notes/release-notes-0.17.4.md b/docs/release-notes/release-notes-0.17.4.md index c527576b7..25f0f75d6 100644 --- a/docs/release-notes/release-notes-0.17.4.md +++ b/docs/release-notes/release-notes-0.17.4.md @@ -29,6 +29,11 @@ FilterKnownChanIDs](https://github.com/lightningnetwork/lnd/pull/8400) by ensuring the `cacheMu` mutex is acquired before the main database lock. +* [Prevent](https://github.com/lightningnetwork/lnd/pull/8385) ping failures + from [deadlocking](https://github.com/lightningnetwork/lnd/issues/8379) + the peer connection. + + # New Features ## Functional Enhancements ## RPC Additions @@ -50,5 +55,8 @@ ## Tooling and Documentation # Contributors (Alphabetical Order) + * Elle Mouton +* Keagan McClelland +* Olaoluwa Osuntokun * ziggie1984