From 13d7b59726140ff88ac2135de62ce267b2aad642 Mon Sep 17 00:00:00 2001
From: Ferenc Szabo <ferenc.szabo@ethereum.org>
Date: Thu, 17 Jan 2019 12:06:45 +0100
Subject: [PATCH] swarm/network: fix data race in fetcher_test.go

Problem:
Let's say TestA() and TestB() was started right after each other.
TestA() called `go Fetcher.run(ctx)` (read: searchTimeout), on test
completion the context was cancelled, but the test did not wait for
goroutine termination. Test(B) started by modifying searchTimeout
(write).

Solution: Let's just store searchTimeout on the Fetcher struct and
change the package var (default) to const (thread-safe).

Alternative (almost) solution:
The above could have been solved by (little ugly, more complex)
waitGroups. However that would have not work for TestFetcherFactory.
As fetcherFactory.New()starts a goroutine inside it's body without
providing a way for clear termination. (We did not want to modify the
interface for this one, as we think the unclean termination does not
cause a problem in this case in production.)

fixes ethersphere/go-ethereum#1109
---
 swarm/network/fetcher.go      | 17 +++++++++++------
 swarm/network/fetcher_test.go | 16 +++++-----------
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/swarm/network/fetcher.go b/swarm/network/fetcher.go
index 1a660d7736cb..3043f6475826 100644
--- a/swarm/network/fetcher.go
+++ b/swarm/network/fetcher.go
@@ -26,14 +26,17 @@ import (
 	"github.com/ethereum/go-ethereum/swarm/storage"
 )
 
-var searchTimeout = 1 * time.Second
+const (
+	defaultSearchTimeout = 1 * time.Second
+	// maximum number of forwarded requests (hops), to make sure requests are not
+	// forwarded forever in peer loops
+	maxHopCount uint8 = 20
+)
 
 // Time to consider peer to be skipped.
 // Also used in stream delivery.
 var RequestTimeout = 10 * time.Second
 
-var maxHopCount uint8 = 20 // maximum number of forwarded requests (hops), to make sure requests are not forwarded forever in peer loops
-
 type RequestFunc func(context.Context, *Request) (*enode.ID, chan struct{}, error)
 
 // Fetcher is created when a chunk is not found locally. It starts a request handler loop once and
@@ -47,6 +50,7 @@ type Fetcher struct {
 	addr             storage.Address // the address of the chunk to be fetched
 	offerC           chan *enode.ID  // channel of sources (peer node id strings)
 	requestC         chan uint8      // channel for incoming requests (with the hopCount value in it)
+	searchTimeout    time.Duration
 	skipCheck        bool
 }
 
@@ -118,6 +122,7 @@ func NewFetcher(addr storage.Address, rf RequestFunc, skipCheck bool) *Fetcher {
 		protoRequestFunc: rf,
 		offerC:           make(chan *enode.ID),
 		requestC:         make(chan uint8),
+		searchTimeout:    defaultSearchTimeout,
 		skipCheck:        skipCheck,
 	}
 }
@@ -232,7 +237,7 @@ func (f *Fetcher) run(ctx context.Context, peers *sync.Map) {
 		// if wait channel is not set, set it to a timer
 		if requested {
 			if wait == nil {
-				wait = time.NewTimer(searchTimeout)
+				wait = time.NewTimer(f.searchTimeout)
 				defer wait.Stop()
 				waitC = wait.C
 			} else {
@@ -243,8 +248,8 @@ func (f *Fetcher) run(ctx context.Context, peers *sync.Map) {
 					default:
 					}
 				}
-				// reset the timer to go off after searchTimeout
-				wait.Reset(searchTimeout)
+				// reset the timer to go off after defaultSearchTimeout
+				wait.Reset(f.searchTimeout)
 			}
 		}
 		doRequest = false
diff --git a/swarm/network/fetcher_test.go b/swarm/network/fetcher_test.go
index 3a926f475873..563c6cece793 100644
--- a/swarm/network/fetcher_test.go
+++ b/swarm/network/fetcher_test.go
@@ -284,15 +284,11 @@ func TestFetcherRetryOnTimeout(t *testing.T) {
 	requester := newMockRequester()
 	addr := make([]byte, 32)
 	fetcher := NewFetcher(addr, requester.doRequest, true)
+	// set searchTimeOut to low value so the test is quicker
+	fetcher.searchTimeout = 250 * time.Millisecond
 
 	peersToSkip := &sync.Map{}
 
-	// set searchTimeOut to low value so the test is quicker
-	defer func(t time.Duration) {
-		searchTimeout = t
-	}(searchTimeout)
-	searchTimeout = 250 * time.Millisecond
-
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
@@ -359,11 +355,9 @@ func TestFetcherRequestQuitRetriesRequest(t *testing.T) {
 	addr := make([]byte, 32)
 	fetcher := NewFetcher(addr, requester.doRequest, true)
 
-	// make sure searchTimeout is long so it is sure the request is not retried because of timeout
-	defer func(t time.Duration) {
-		searchTimeout = t
-	}(searchTimeout)
-	searchTimeout = 10 * time.Second
+	// make sure the searchTimeout is long so it is sure the request is not
+	// retried because of timeout
+	fetcher.searchTimeout = 10 * time.Second
 
 	peersToSkip := &sync.Map{}