From ad8e25cbc9c78c270e89739b644a99c1647fa8ce Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Mon, 21 Nov 2022 13:06:24 +0100 Subject: [PATCH 1/3] multi: don't access loop variables in goroutines This commit makes sure that no loop variables or other temporary variables are accessed directly in a goroutine but are instead passed into the goroutine through a parameter. This makes sure a copy of the value is put on the stack and is not changed while the outside loop continues. --- chainntnfs/bitcoindnotify/bitcoind.go | 12 ++++++++---- chainntnfs/btcdnotify/btcd.go | 6 ++++-- chainntnfs/neutrinonotify/neutrino.go | 6 ++++-- healthcheck/healthcheck.go | 5 +++-- lntest/itest/utils.go | 7 +++---- lnwallet/btcwallet/btcwallet.go | 14 +++++++++----- rpcserver.go | 4 ++-- 7 files changed, 33 insertions(+), 21 deletions(-) diff --git a/chainntnfs/bitcoindnotify/bitcoind.go b/chainntnfs/bitcoindnotify/bitcoind.go index 424291351a..680d7c7d0a 100644 --- a/chainntnfs/bitcoindnotify/bitcoind.go +++ b/chainntnfs/bitcoindnotify/bitcoind.go @@ -235,7 +235,9 @@ out: // // TODO(wilmer): add retry logic if rescan fails? b.wg.Add(1) - go func() { + + //nolint:lll + go func(msg *chainntnfs.HistoricalConfDispatch) { defer b.wg.Done() confDetails, _, err := b.historicalConfDetails( @@ -269,7 +271,7 @@ out: "details of %v: %v", msg.ConfRequest, err) } - }() + }(msg) case *chainntnfs.HistoricalSpendDispatch: // In order to ensure we don't block the caller @@ -278,7 +280,9 @@ out: // // TODO(wilmer): add retry logic if rescan fails? b.wg.Add(1) - go func() { + + //nolint:lll + go func(msg *chainntnfs.HistoricalSpendDispatch) { defer b.wg.Done() spendDetails, err := b.historicalSpendDetails( @@ -320,7 +324,7 @@ out: "details of %v: %v", msg.SpendRequest, err) } - }() + }(msg) case *blockEpochRegistration: chainntnfs.Log.Infof("New block epoch subscription") diff --git a/chainntnfs/btcdnotify/btcd.go b/chainntnfs/btcdnotify/btcd.go index 2ced59754e..5dfb82346a 100644 --- a/chainntnfs/btcdnotify/btcd.go +++ b/chainntnfs/btcdnotify/btcd.go @@ -347,7 +347,9 @@ out: // // TODO(wilmer): add retry logic if rescan fails? b.wg.Add(1) - go func() { + + //nolint:lll + go func(msg *chainntnfs.HistoricalConfDispatch) { defer b.wg.Done() confDetails, _, err := b.historicalConfDetails( @@ -372,7 +374,7 @@ out: if err != nil { chainntnfs.Log.Error(err) } - }() + }(msg) case *blockEpochRegistration: chainntnfs.Log.Infof("New block epoch subscription") diff --git a/chainntnfs/neutrinonotify/neutrino.go b/chainntnfs/neutrinonotify/neutrino.go index 96d5b6f8ae..6f8f9717cb 100644 --- a/chainntnfs/neutrinonotify/neutrino.go +++ b/chainntnfs/neutrinonotify/neutrino.go @@ -432,7 +432,9 @@ func (n *NeutrinoNotifier) notificationDispatcher() { // chain asynchronously to prevent blocking // potentially long rescans. n.wg.Add(1) - go func() { + + //nolint:lll + go func(msg *chainntnfs.HistoricalConfDispatch) { defer n.wg.Done() confDetails, err := n.historicalConfDetails( @@ -457,7 +459,7 @@ func (n *NeutrinoNotifier) notificationDispatcher() { if err != nil { chainntnfs.Log.Error(err) } - }() + }(msg) case *blockEpochRegistration: chainntnfs.Log.Infof("New block epoch subscription") diff --git a/healthcheck/healthcheck.go b/healthcheck/healthcheck.go index 5244eb8885..e314f8d196 100644 --- a/healthcheck/healthcheck.go +++ b/healthcheck/healthcheck.go @@ -71,10 +71,11 @@ func (m *Monitor) Start() error { } m.wg.Add(1) - go func() { + go func(check *Observation) { defer m.wg.Done() + check.monitor(m.cfg.Shutdown, m.quit) - }() + }(check) } return nil diff --git a/lntest/itest/utils.go b/lntest/itest/utils.go index abeac91db2..c2539b73bd 100644 --- a/lntest/itest/utils.go +++ b/lntest/itest/utils.go @@ -78,13 +78,12 @@ func completePaymentRequests(client lnrpc.LightningClient, // Launch all payments simultaneously. results := make(chan error) for _, payReq := range paymentRequests { - payReqCopy := payReq - go func() { - err := send(payReqCopy) + go func(payReq string) { + err := send(payReq) if awaitResponse { results <- err } - }() + }(payReq) } // If awaiting a response, verify that all payments succeeded. diff --git a/lnwallet/btcwallet/btcwallet.go b/lnwallet/btcwallet/btcwallet.go index eca08eb37e..ad32da0cfb 100644 --- a/lnwallet/btcwallet/btcwallet.go +++ b/lnwallet/btcwallet/btcwallet.go @@ -1483,9 +1483,13 @@ out: // Launch a goroutine to re-package and send // notifications for any newly confirmed transactions. - go func() { + //nolint:lll + go func(txNtfn *wallet.TransactionNotifications) { for _, block := range txNtfn.AttachedBlocks { - details, err := minedTransactionsToDetails(currentHeight, block, t.w.ChainParams()) + details, err := minedTransactionsToDetails( + currentHeight, block, + t.w.ChainParams(), + ) if err != nil { continue } @@ -1499,11 +1503,11 @@ out: } } - }() + }(txNtfn) // Launch a goroutine to re-package and send // notifications for any newly unconfirmed transactions. - go func() { + go func(txNtfn *wallet.TransactionNotifications) { for _, tx := range txNtfn.UnminedTransactions { detail, err := unminedTransactionsToDetail( tx, t.w.ChainParams(), @@ -1518,7 +1522,7 @@ out: return } } - }() + }(txNtfn) case <-t.quit: break out } diff --git a/rpcserver.go b/rpcserver.go index 09ba7dae8f..847e9564c0 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -5214,7 +5214,7 @@ sendLoop: // payment so we can continue to serve requests while // this payment is being dispatched. wg.Add(1) - go func() { + go func(payIntent *rpcPaymentIntent) { defer wg.Done() // Attempt to grab a free semaphore slot, using @@ -5292,7 +5292,7 @@ sendLoop: } return } - }() + }(payIntent) } } From 55b53555e93d30ad1c1ef5a841650287f99aea44 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Mon, 21 Nov 2022 13:07:56 +0100 Subject: [PATCH 2/3] multi: improve readability of goroutine defers This commit fixes the readability of some of the defer calls in goroutines by making sure the defer stands out properly. --- discovery/gossiper.go | 2 ++ funding/manager.go | 1 + htlcswitch/switch.go | 1 + lntest/itest/utils.go | 1 + rpcperms/middleware_handler.go | 3 ++- sweep/sweeper.go | 1 + ticker/force.go | 1 + watchtower/wtclient/session_queue.go | 1 + 8 files changed, 10 insertions(+), 1 deletion(-) diff --git a/discovery/gossiper.go b/discovery/gossiper.go index bceb3755c1..bd2d3b605a 100644 --- a/discovery/gossiper.go +++ b/discovery/gossiper.go @@ -1325,6 +1325,7 @@ func (d *AuthenticatedGossiper) networkHandler() { d.wg.Add(1) go func() { defer d.wg.Done() + log.Infof("Broadcasting %v new announcements in %d sub batches", len(announcementBatch), len(splitAnnouncementBatch)) @@ -2947,6 +2948,7 @@ func (d *AuthenticatedGossiper) handleAnnSig(nMsg *networkMsg, d.wg.Add(1) go func() { defer d.wg.Done() + log.Debugf("Received half proof for channel "+ "%v with existing full proof. Sending"+ " full proof to peer=%x", diff --git a/funding/manager.go b/funding/manager.go index d9eeac9a02..b33b18ceda 100644 --- a/funding/manager.go +++ b/funding/manager.go @@ -1969,6 +1969,7 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer, f.wg.Add(1) go func() { defer f.wg.Done() + f.waitForPsbt(psbtIntent, resCtx, pendingChanID) }() diff --git a/htlcswitch/switch.go b/htlcswitch/switch.go index 8b2120d3e8..34997ac82f 100644 --- a/htlcswitch/switch.go +++ b/htlcswitch/switch.go @@ -1742,6 +1742,7 @@ func (s *Switch) htlcForwarder() { wg.Add(1) go func(l ChannelLink) { defer wg.Done() + l.Stop() }(link) } diff --git a/lntest/itest/utils.go b/lntest/itest/utils.go index c2539b73bd..dd03ac40b4 100644 --- a/lntest/itest/utils.go +++ b/lntest/itest/utils.go @@ -405,6 +405,7 @@ func subscribeChannelNotifications(ctxb context.Context, t *harnessTest, chanUpdates := make(chan *lnrpc.ChannelEventUpdate, 20) go func() { defer cancelFunc() + for { select { case <-quit: diff --git a/rpcperms/middleware_handler.go b/rpcperms/middleware_handler.go index 1e9d16b569..b18fc0d16c 100644 --- a/rpcperms/middleware_handler.go +++ b/rpcperms/middleware_handler.go @@ -180,8 +180,9 @@ func (h *MiddlewareHandler) Run() error { // request to the client). h.wg.Add(1) go func() { + defer h.wg.Done() + h.receiveResponses(errChan, responses) - h.wg.Done() }() return h.sendInterceptRequests(errChan, responses) diff --git a/sweep/sweeper.go b/sweep/sweeper.go index d34ee50b9d..5957b51acb 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -1415,6 +1415,7 @@ func (s *UtxoSweeper) waitForSpend(outpoint wire.OutPoint, s.wg.Add(1) go func() { defer s.wg.Done() + select { case spend, ok := <-spendEvent.Spend: if !ok { diff --git a/ticker/force.go b/ticker/force.go index 016e370206..dff99dabdc 100644 --- a/ticker/force.go +++ b/ticker/force.go @@ -39,6 +39,7 @@ func NewForce(interval time.Duration) *Force { m.wg.Add(1) go func() { defer m.wg.Done() + for { select { case t := <-m.ticker: diff --git a/watchtower/wtclient/session_queue.go b/watchtower/wtclient/session_queue.go index 7d98ec86f8..a5c570a71d 100644 --- a/watchtower/wtclient/session_queue.go +++ b/watchtower/wtclient/session_queue.go @@ -689,6 +689,7 @@ func (s *sessionQueueSet) ApplyAndWait(getApply func(*sessionQueue) func()) { wg.Add(1) go func(sq *sessionQueue) { defer wg.Done() + getApply(sq)() }(sessionq) } From ee753923e45854fa494f1cae2410cd6d464fdab5 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Mon, 21 Nov 2022 13:21:49 +0100 Subject: [PATCH 3/3] docs: add release notes --- docs/release-notes/release-notes-0.16.0.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/release-notes/release-notes-0.16.0.md b/docs/release-notes/release-notes-0.16.0.md index 1c560c6858..65e80c7732 100644 --- a/docs/release-notes/release-notes-0.16.0.md +++ b/docs/release-notes/release-notes-0.16.0.md @@ -198,6 +198,9 @@ certain large transactions](https://github.com/lightningnetwork/lnd/pull/7100). * [test: fix loop variables being accessed in closures](https://github.com/lightningnetwork/lnd/pull/7032). + +* [Fix loop and other temporary variables being accessed in + goroutines](https://github.com/lightningnetwork/lnd/pull/7188). ## Watchtowers