From 9a78e9da7353e642844c1a4ddc3f64bfa066a04b Mon Sep 17 00:00:00 2001
From: carla <kirkcohenc@gmail.com>
Date: Fri, 23 Apr 2021 08:39:39 +0200
Subject: [PATCH] routing/tests: move lock acquisition after state driving
 channels

In our payment lifecycle tests, we have two goroutines that
compete for the lock in our mock control tower: the resumePayment
loop which tries to call RegisterAttempt, and the collectResult
handler which is launched in a goroutine by collectResultAsync
and is responsible for various settle/fail calls.

The order that the lock is acquired by these goroutines is
arbitrary, and can lead to flakes in our tests if the step
that we do not intend to execute first gets the lock (eg,
we want to fail a payment in collectResult, but RegisterAttempt
gets there first). This commit moves contention for this lock
after our mock's various "state driving" channels, so that the
lock will be acquired in the order that the test intends it.
---
 routing/mock_test.go | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/routing/mock_test.go b/routing/mock_test.go
index 9c4f79e5d..ae67075d9 100644
--- a/routing/mock_test.go
+++ b/routing/mock_test.go
@@ -248,13 +248,13 @@ func makeMockControlTower() *mockControlTower {
 func (m *mockControlTower) InitPayment(phash lntypes.Hash,
 	c *channeldb.PaymentCreationInfo) error {
 
-	m.Lock()
-	defer m.Unlock()
-
 	if m.init != nil {
 		m.init <- initArgs{c}
 	}
 
+	m.Lock()
+	defer m.Unlock()
+
 	// Don't allow re-init a successful payment.
 	if _, ok := m.successful[phash]; ok {
 		return channeldb.ErrAlreadyPaid
@@ -279,13 +279,13 @@ func (m *mockControlTower) InitPayment(phash lntypes.Hash,
 func (m *mockControlTower) RegisterAttempt(phash lntypes.Hash,
 	a *channeldb.HTLCAttemptInfo) error {
 
-	m.Lock()
-	defer m.Unlock()
-
 	if m.registerAttempt != nil {
 		m.registerAttempt <- registerAttemptArgs{a}
 	}
 
+	m.Lock()
+	defer m.Unlock()
+
 	// Cannot register attempts for successful or failed payments.
 	if _, ok := m.successful[phash]; ok {
 		return channeldb.ErrPaymentAlreadySucceeded
@@ -312,13 +312,13 @@ func (m *mockControlTower) SettleAttempt(phash lntypes.Hash,
 	pid uint64, settleInfo *channeldb.HTLCSettleInfo) (
 	*channeldb.HTLCAttempt, error) {
 
-	m.Lock()
-	defer m.Unlock()
-
 	if m.settleAttempt != nil {
 		m.settleAttempt <- settleAttemptArgs{settleInfo.Preimage}
 	}
 
+	m.Lock()
+	defer m.Unlock()
+
 	// Only allow setting attempts if the payment is known.
 	p, ok := m.payments[phash]
 	if !ok {
@@ -353,13 +353,13 @@ func (m *mockControlTower) SettleAttempt(phash lntypes.Hash,
 func (m *mockControlTower) FailAttempt(phash lntypes.Hash, pid uint64,
 	failInfo *channeldb.HTLCFailInfo) (*channeldb.HTLCAttempt, error) {
 
-	m.Lock()
-	defer m.Unlock()
-
 	if m.failAttempt != nil {
 		m.failAttempt <- failAttemptArgs{failInfo}
 	}
 
+	m.Lock()
+	defer m.Unlock()
+
 	// Only allow failing attempts if the payment is known.
 	p, ok := m.payments[phash]
 	if !ok {
@@ -437,13 +437,13 @@ func (m *mockControlTower) FetchPayment(phash lntypes.Hash) (
 func (m *mockControlTower) FetchInFlightPayments() (
 	[]*channeldb.InFlightPayment, error) {
 
-	m.Lock()
-	defer m.Unlock()
-
 	if m.fetchInFlight != nil {
 		m.fetchInFlight <- struct{}{}
 	}
 
+	m.Lock()
+	defer m.Unlock()
+
 	// In flight are all payments not successful or failed.
 	var fl []*channeldb.InFlightPayment
 	for hash, p := range m.payments {