cmd/swarm: fix resource leaks in tests (#19443)

* swarm/api: fix file descriptor leak in NewTestSwarmServer

Swarm storage (localstore) was not closed. That resulted a
"too many open files" error if `TestClientUploadDownloadRawEncrypted`
was run with `-count 1000`.

* cmd/swarm: speed up StartNewNodes() by parallelization

Reduce cluster startup time from 13s to 7s.

* swarm/api: disable flaky TestClientUploadDownloadRawEncrypted with -race

* swarm/storage: disable flaky TestLDBStoreCollectGarbage (-race)

With race detection turned on the disabled cases often fail with:
"ldbstore_test.go:535: expected surplus chunk 150 to be missing, but got no error"

* cmd/swarm: fix process leak in TestACT and TestSwarmUp

Each test run we start 3 nodes, but we did not terminate them. So
those 3 nodes continued eating up 1.2GB (3.4GB with -race) after test
completion.

6b6c4d1c27 changed how we start clusters
to speed up tests. The changeset merged together test cases
and introduced a global cluster. But "forgot" about termination.

Let's get rid of "global cluster" so we have a clear owner of
termination (some time sacrifice), while leaving subtests to use the
same cluster.

cmd/swarm/access_test.go

@@ -52,11 +52,12 @@ func TestACT(t *testing.T) {
 		t.Skip()
 	}
 
-	initCluster(t)
+	cluster := newTestCluster(t, clusterSize)
+	defer cluster.Shutdown()
 
 	cases := []struct {
 		name string
-		f    func(t *testing.T)
+		f    func(t *testing.T, cluster *testCluster)
 	}{
 		{"Password", testPassword},
 		{"PK", testPK},
@@ -65,7 +66,9 @@ func TestACT(t *testing.T) {
 	}
 
 	for _, tc := range cases {
-		t.Run(tc.name, tc.f)
+		t.Run(tc.name, func(t *testing.T) {
+			tc.f(t, cluster)
+		})
 	}
 }
 
@@ -74,7 +77,7 @@ func TestACT(t *testing.T) {
 // The parties participating - node (publisher), uploads to second node then disappears. Content which was uploaded
 // is then fetched through 2nd node. since the tested code is not key-aware - we can just
 // fetch from the 2nd node using HTTP BasicAuth
-func testPassword(t *testing.T) {
+func testPassword(t *testing.T, cluster *testCluster) {
 	dataFilename := testutil.TempFileWithContent(t, data)
 	defer os.RemoveAll(dataFilename)
 
@@ -226,7 +229,7 @@ func testPassword(t *testing.T) {
 // The parties participating - node (publisher), uploads to second node (which is also the grantee) then disappears.
 // Content which was uploaded is then fetched through the grantee's http proxy. Since the tested code is private-key aware,
 // the test will fail if the proxy's given private key is not granted on the ACT.
-func testPK(t *testing.T) {
+func testPK(t *testing.T, cluster *testCluster) {
 	dataFilename := testutil.TempFileWithContent(t, data)
 	defer os.RemoveAll(dataFilename)
 
@@ -359,13 +362,13 @@ func testPK(t *testing.T) {
 }
 
 // testACTWithoutBogus tests the creation of the ACT manifest end-to-end, without any bogus entries (i.e. default scenario = 3 nodes 1 unauthorized)
-func testACTWithoutBogus(t *testing.T) {
+func testACTWithoutBogus(t *testing.T, cluster *testCluster) {
-	testACT(t, 0)
+	testACT(t, cluster, 0)
 }
 
 // testACTWithBogus tests the creation of the ACT manifest end-to-end, with 100 bogus entries (i.e. 100 EC keys + default scenario = 3 nodes 1 unauthorized = 103 keys in the ACT manifest)
-func testACTWithBogus(t *testing.T) {
+func testACTWithBogus(t *testing.T, cluster *testCluster) {
-	testACT(t, 100)
+	testACT(t, cluster, 100)
 }
 
 // testACT tests the e2e creation, uploading and downloading of an ACT access control with both EC keys AND password protection
@@ -373,7 +376,7 @@ func testACTWithBogus(t *testing.T) {
 // set and also protects the ACT with a password. the third node should fail decoding the reference as it will not be granted access.
 // the third node then then tries to download using a correct password (and succeeds) then uses a wrong password and fails.
 // the publisher uploads through one of the nodes then disappears.
-func testACT(t *testing.T, bogusEntries int) {
+func testACT(t *testing.T, cluster *testCluster, bogusEntries int) {
 	var uploadThroughNode = cluster.Nodes[0]
 	client := swarmapi.NewClient(uploadThroughNode.URL)
 

cmd/swarm/run_test.go

@@ -59,15 +59,6 @@ func init() {
 
 const clusterSize = 3
 
-var clusteronce sync.Once
-var cluster *testCluster
-
-func initCluster(t *testing.T) {
-	clusteronce.Do(func() {
-		cluster = newTestCluster(t, clusterSize)
-	})
-}
-
 func serverFunc(api *api.API) swarmhttp.TestServer {
 	return swarmhttp.NewServer(api, "")
 }
@@ -165,10 +156,8 @@ outer:
 }
 
 func (c *testCluster) Shutdown() {
-	for _, node := range c.Nodes {
+	c.Stop()
-		node.Shutdown()
+	c.Cleanup()
-	}
-	os.RemoveAll(c.TmpDir)
 }
 
 func (c *testCluster) Stop() {
@@ -179,16 +168,35 @@ func (c *testCluster) Stop() {
 
 func (c *testCluster) StartNewNodes(t *testing.T, size int) {
 	c.Nodes = make([]*testNode, 0, size)
+
+	errors := make(chan error, size)
+	nodes := make(chan *testNode, size)
 	for i := 0; i < size; i++ {
-		dir := filepath.Join(c.TmpDir, fmt.Sprintf("swarm%02d", i))
+		go func(nodeIndex int) {
-		if err := os.Mkdir(dir, 0700); err != nil {
+			dir := filepath.Join(c.TmpDir, fmt.Sprintf("swarm%02d", nodeIndex))
-			t.Fatal(err)
+			if err := os.Mkdir(dir, 0700); err != nil {
+				errors <- err
+				return
+			}
+
+			node := newTestNode(t, dir)
+			node.Name = fmt.Sprintf("swarm%02d", nodeIndex)
+			nodes <- node
+		}(i)
+	}
+
+	for i := 0; i < size; i++ {
+		select {
+		case node := <-nodes:
+			c.Nodes = append(c.Nodes, node)
+		case err := <-errors:
+			t.Error(err)
 		}
+	}
 
-		node := newTestNode(t, dir)
+	if t.Failed() {
-		node.Name = fmt.Sprintf("swarm%02d", i)
+		c.Shutdown()
-
+		t.FailNow()
-		c.Nodes = append(c.Nodes, node)
 	}
 }
 

cmd/swarm/upload_test.go

@@ -46,11 +46,12 @@ func TestSwarmUp(t *testing.T) {
 		t.Skip()
 	}
 
-	initCluster(t)
+	cluster := newTestCluster(t, clusterSize)
+	defer cluster.Shutdown()
 
 	cases := []struct {
 		name string
-		f    func(t *testing.T)
+		f    func(t *testing.T, cluster *testCluster)
 	}{
 		{"NoEncryption", testNoEncryption},
 		{"Encrypted", testEncrypted},
@@ -60,31 +61,33 @@ func TestSwarmUp(t *testing.T) {
 	}
 
 	for _, tc := range cases {
-		t.Run(tc.name, tc.f)
+		t.Run(tc.name, func(t *testing.T) {
+			tc.f(t, cluster)
+		})
 	}
 }
 
 // testNoEncryption tests that running 'swarm up' makes the resulting file
 // available from all nodes via the HTTP API
-func testNoEncryption(t *testing.T) {
+func testNoEncryption(t *testing.T, cluster *testCluster) {
-	testDefault(false, t)
+	testDefault(t, cluster, false)
 }
 
 // testEncrypted tests that running 'swarm up --encrypted' makes the resulting file
 // available from all nodes via the HTTP API
-func testEncrypted(t *testing.T) {
+func testEncrypted(t *testing.T, cluster *testCluster) {
-	testDefault(true, t)
+	testDefault(t, cluster, true)
 }
 
-func testRecursiveNoEncryption(t *testing.T) {
+func testRecursiveNoEncryption(t *testing.T, cluster *testCluster) {
-	testRecursive(false, t)
+	testRecursive(t, cluster, false)
 }
 
-func testRecursiveEncrypted(t *testing.T) {
+func testRecursiveEncrypted(t *testing.T, cluster *testCluster) {
-	testRecursive(true, t)
+	testRecursive(t, cluster, true)
 }
 
-func testDefault(toEncrypt bool, t *testing.T) {
+func testDefault(t *testing.T, cluster *testCluster, toEncrypt bool) {
 	tmpFileName := testutil.TempFileWithContent(t, data)
 	defer os.Remove(tmpFileName)
 
@@ -189,7 +192,7 @@ func testDefault(toEncrypt bool, t *testing.T) {
 	}
 }
 
-func testRecursive(toEncrypt bool, t *testing.T) {
+func testRecursive(t *testing.T, cluster *testCluster, toEncrypt bool) {
 	tmpUploadDir, err := ioutil.TempDir("", "swarm-test")
 	if err != nil {
 		t.Fatal(err)
@@ -279,14 +282,14 @@ func testRecursive(toEncrypt bool, t *testing.T) {
 
 // testDefaultPathAll tests swarm recursive upload with relative and absolute
 // default paths and with encryption.
-func testDefaultPathAll(t *testing.T) {
+func testDefaultPathAll(t *testing.T, cluster *testCluster) {
-	testDefaultPath(false, false, t)
+	testDefaultPath(t, cluster, false, false)
-	testDefaultPath(false, true, t)
+	testDefaultPath(t, cluster, false, true)
-	testDefaultPath(true, false, t)
+	testDefaultPath(t, cluster, true, false)
-	testDefaultPath(true, true, t)
+	testDefaultPath(t, cluster, true, true)
 }
 
-func testDefaultPath(toEncrypt bool, absDefaultPath bool, t *testing.T) {
+func testDefaultPath(t *testing.T, cluster *testCluster, toEncrypt bool, absDefaultPath bool) {
 	tmp, err := ioutil.TempDir("", "swarm-defaultpath-test")
 	if err != nil {
 		t.Fatal(err)

swarm/api/client/client_test.go

@@ -25,6 +25,8 @@ import (
 	"sort"
 	"testing"
 
+	"github.com/ethereum/go-ethereum/swarm/testutil"
+
 	"github.com/ethereum/go-ethereum/swarm/storage"
 	"github.com/ethereum/go-ethereum/swarm/storage/feed/lookup"
 
@@ -43,7 +45,13 @@ func serverFunc(api *api.API) swarmhttp.TestServer {
 func TestClientUploadDownloadRaw(t *testing.T) {
 	testClientUploadDownloadRaw(false, t)
 }
+
 func TestClientUploadDownloadRawEncrypted(t *testing.T) {
+	if testutil.RaceEnabled {
+		t.Skip("flaky with -race on Travis")
+		// See: https://github.com/ethersphere/go-ethereum/issues/1254
+	}
+
 	testClientUploadDownloadRaw(true, t)
 }
 

swarm/api/http/test_server.go

@@ -33,44 +33,45 @@ type TestServer interface {
 }
 
 func NewTestSwarmServer(t *testing.T, serverFunc func(*api.API) TestServer, resolver api.Resolver) *TestSwarmServer {
-	dir, err := ioutil.TempDir("", "swarm-storage-test")
+	swarmDir, err := ioutil.TempDir("", "swarm-storage-test")
 	if err != nil {
 		t.Fatal(err)
 	}
-	storeparams := storage.NewDefaultLocalStoreParams()
+
-	storeparams.DbCapacity = 5000000
+	storeParams := storage.NewDefaultLocalStoreParams()
-	storeparams.CacheCapacity = 5000
+	storeParams.DbCapacity = 5000000
-	storeparams.Init(dir)
+	storeParams.CacheCapacity = 5000
-	localStore, err := storage.NewLocalStore(storeparams, nil)
+	storeParams.Init(swarmDir)
+	localStore, err := storage.NewLocalStore(storeParams, nil)
 	if err != nil {
-		os.RemoveAll(dir)
+		os.RemoveAll(swarmDir)
 		t.Fatal(err)
 	}
 	fileStore := storage.NewFileStore(localStore, storage.NewFileStoreParams())
-
 	// Swarm feeds test setup
 	feedsDir, err := ioutil.TempDir("", "swarm-feeds-test")
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	rhparams := &feed.HandlerParams{}
+	feeds, err := feed.NewTestHandler(feedsDir, &feed.HandlerParams{})
-	rh, err := feed.NewTestHandler(feedsDir, rhparams)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	a := api.NewAPI(fileStore, resolver, rh.Handler, nil)
+	swarmApi := api.NewAPI(fileStore, resolver, feeds.Handler, nil)
-	srv := httptest.NewServer(serverFunc(a))
+	apiServer := httptest.NewServer(serverFunc(swarmApi))
+
 	tss := &TestSwarmServer{
-		Server:    srv,
+		Server:    apiServer,
 		FileStore: fileStore,
-		dir:       dir,
+		dir:       swarmDir,
 		Hasher:    storage.MakeHashFunc(storage.DefaultHash)(),
 		cleanup: func() {
-			srv.Close()
+			apiServer.Close()
-			rh.Close()
+			fileStore.Close()
-			os.RemoveAll(dir)
+			feeds.Close()
+			os.RemoveAll(swarmDir)
 			os.RemoveAll(feedsDir)
 		},
 		CurrentTime: 42,

swarm/storage/ldbstore_test.go

@@ -27,6 +27,8 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/ethereum/go-ethereum/swarm/testutil"
+
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/swarm/chunk"
 	"github.com/ethereum/go-ethereum/swarm/log"
@@ -322,6 +324,12 @@ func TestLDBStoreCollectGarbage(t *testing.T) {
 	initialCap := defaultMaxGCRound / 100
 	cap := initialCap / 2
 	t.Run(fmt.Sprintf("A/%d/%d", cap, cap*4), testLDBStoreCollectGarbage)
+
+	if testutil.RaceEnabled {
+		t.Skip("only the simplest case run as others are flaky with race")
+		// Note: some tests fail consistently and even locally with `-race`
+	}
+
 	t.Run(fmt.Sprintf("B/%d/%d", cap, cap*4), testLDBStoreRemoveThenCollectGarbage)
 
 	// at max round