From 7cf056b2c26bcbb1fb31e989945f3b58408788a3 Mon Sep 17 00:00:00 2001
From: Maciej Zimnoch <maciej@scylladb.com>
Date: Thu, 14 Nov 2019 14:14:40 +0100
Subject: [PATCH] accounting: allow MaxCompletedTransfers to be configurable

rclone library users might be intrested in changing default value to
other, or even disabling it. With current version it's impossible which
leads to races when number of uploaded objects exceeds default limit.

Fixes #3732
---
 fs/accounting/stats.go      | 12 ++++---
 fs/accounting/stats_test.go | 66 +++++++++++++++++++++++++------------
 2 files changed, 53 insertions(+), 25 deletions(-)

diff --git a/fs/accounting/stats.go b/fs/accounting/stats.go
index d4a54380e..438c4a554 100644
--- a/fs/accounting/stats.go
+++ b/fs/accounting/stats.go
@@ -13,8 +13,8 @@ import (
 	"github.com/rclone/rclone/fs/rc"
 )
 
-// Maximum number of completed transfers in startedTransfers list
-const maxCompletedTransfers = 100
+// MaxCompletedTransfers specifies maximum number of completed transfers in startedTransfers list
+var MaxCompletedTransfers = 100
 
 // StatsInfo accounts all transfers
 type StatsInfo struct {
@@ -627,11 +627,15 @@ func (s *StatsInfo) RemoveTransfer(transfer *Transfer) {
 	s.mu.Unlock()
 }
 
-// PruneTransfers makes sure there aren't too many old transfers
+// PruneTransfers makes sure there aren't too many old transfers by removing
+// single finished transfer.
 func (s *StatsInfo) PruneTransfers() {
+	if MaxCompletedTransfers < 0 {
+		return
+	}
 	s.mu.Lock()
 	// remove a transfer from the start if we are over quota
-	if len(s.startedTransfers) > maxCompletedTransfers+fs.Config.Transfers {
+	if len(s.startedTransfers) > MaxCompletedTransfers+fs.Config.Transfers {
 		for i, tr := range s.startedTransfers {
 			if tr.IsDone() {
 				s.removeTransfer(tr, i)
diff --git a/fs/accounting/stats_test.go b/fs/accounting/stats_test.go
index 7a5df0d61..2e462cb74 100644
--- a/fs/accounting/stats_test.go
+++ b/fs/accounting/stats_test.go
@@ -382,28 +382,52 @@ func TestTimeRangeDuration(t *testing.T) {
 }
 
 func TestPruneTransfers(t *testing.T) {
-	max := maxCompletedTransfers + fs.Config.Transfers
+	for _, test := range []struct {
+		Name                     string
+		Transfers                int
+		Limit                    int
+		ExpectedStartedTransfers int
+	}{
+		{
+			Name:                     "Limited number of StartedTransfers",
+			Limit:                    100,
+			Transfers:                200,
+			ExpectedStartedTransfers: 100 + fs.Config.Transfers,
+		},
+		{
+			Name:                     "Unlimited number of StartedTransfers",
+			Limit:                    -1,
+			Transfers:                200,
+			ExpectedStartedTransfers: 200,
+		},
+	} {
+		t.Run(test.Name, func(t *testing.T) {
+			prevLimit := MaxCompletedTransfers
+			MaxCompletedTransfers = test.Limit
+			defer func() { MaxCompletedTransfers = prevLimit }()
+
+			s := NewStats()
+			for i := int64(1); i <= int64(test.Transfers); i++ {
+				s.AddTransfer(&Transfer{
+					startedAt:   time.Unix(i, 0),
+					completedAt: time.Unix(i+1, 0),
+				})
+			}
+
+			s.mu.Lock()
+			assert.Equal(t, time.Duration(test.Transfers)*time.Second, s.totalDuration())
+			assert.Equal(t, test.Transfers, len(s.startedTransfers))
+			s.mu.Unlock()
+
+			for i := 0; i < test.Transfers; i++ {
+				s.PruneTransfers()
+			}
+
+			s.mu.Lock()
+			assert.Equal(t, time.Duration(test.Transfers)*time.Second, s.totalDuration())
+			assert.Equal(t, test.ExpectedStartedTransfers, len(s.startedTransfers))
+			s.mu.Unlock()
 
-	s := NewStats()
-	for i := int64(1); i <= int64(max+100); i++ {
-		s.AddTransfer(&Transfer{
-			startedAt:   time.Unix(i, 0),
-			completedAt: time.Unix(i+1, 0),
 		})
 	}
-
-	s.mu.Lock()
-	assert.Equal(t, time.Duration(max+100)*time.Second, s.totalDuration())
-	assert.Equal(t, max+100, len(s.startedTransfers))
-	s.mu.Unlock()
-
-	for i := 0; i < 200; i++ {
-		s.PruneTransfers()
-	}
-
-	s.mu.Lock()
-	assert.Equal(t, time.Duration(max+100)*time.Second, s.totalDuration())
-	assert.Equal(t, max, len(s.startedTransfers))
-	s.mu.Unlock()
-
 }