From 668fc7f0392a3c9d44e05721c7d1d3bc5f6adbfa Mon Sep 17 00:00:00 2001
From: James Houlahan <james.houlahan@protonmail.com>
Date: Mon, 27 Apr 2020 13:26:30 +0200
Subject: [PATCH] feat: MinSpeed -> MinBytesPerSecond, check every 3 seconds

---
 Changelog.md             |  1 +
 pkg/config/pmapi_prod.go | 10 +++++-----
 pkg/pmapi/client.go      | 19 ++++++++++++-------
 pkg/pmapi/client_test.go | 10 +++++-----
 4 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/Changelog.md b/Changelog.md
index 4a9b6861..2c13449d 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -32,6 +32,7 @@ Changelog [format](http://keepachangelog.com/en/1.0.0/)
 ### Fixed
 * Use correct binary name when finding location of addcert.scpt
 * GODT-267 Correctly detect if a message is a draft even if does not have DraftLabel
+* GODT-308 reduce minimum read speed threshold to avoid issues with flaky internet
 
 ## [v1.2.6] Donghai - beta (2020-03-31)
 
diff --git a/pkg/config/pmapi_prod.go b/pkg/config/pmapi_prod.go
index aef1793d..4465cfbf 100644
--- a/pkg/config/pmapi_prod.go
+++ b/pkg/config/pmapi_prod.go
@@ -31,11 +31,11 @@ import (
 
 func (c *Config) GetAPIConfig() *pmapi.ClientConfig {
 	return &pmapi.ClientConfig{
-		AppVersion:       strings.Title(c.appName) + "_" + c.version,
-		ClientID:         c.appName,
-		Timeout:          10 * time.Minute, // Overall request timeout (~25MB / 10 mins => ~40kB/s, should be reasonable).
-		FirstReadTimeout: 30 * time.Second, // 30s to match 30s response header timeout.
-		MinSpeed:         1 << 10,          // Enforce minimum download speed of 1kB/s.
+		AppVersion:        strings.Title(c.appName) + "_" + c.version,
+		ClientID:          c.appName,
+		Timeout:           10 * time.Minute, // Overall request timeout (~25MB / 10 mins => ~40kB/s, should be reasonable).
+		FirstReadTimeout:  30 * time.Second, // 30s to match 30s response header timeout.
+		MinBytesPerSecond: 1 << 10,          // Enforce minimum download speed of 1kB/s.
 	}
 }
 
diff --git a/pkg/pmapi/client.go b/pkg/pmapi/client.go
index bbd443bd..5e1e17d2 100644
--- a/pkg/pmapi/client.go
+++ b/pkg/pmapi/client.go
@@ -87,13 +87,13 @@ type ClientConfig struct {
 	Timeout time.Duration
 
 	// FirstReadTimeout specifies the timeout from getting response to the first read of body response.
-	// This timeout is applied only when MinSpeed is used.
+	// This timeout is applied only when MinBytesPerSecond is used.
 	// Default is 5 minutes.
 	FirstReadTimeout time.Duration
 
-	// MinSpeed specifies minimum Bytes per second or the request will be canceled.
+	// MinBytesPerSecond specifies minimum Bytes per second or the request will be canceled.
 	// Zero means no limitation.
-	MinSpeed int64
+	MinBytesPerSecond int64
 }
 
 // Client defines the interface of a PMAPI client.
@@ -316,7 +316,7 @@ func (c *client) doJSONBuffered(req *http.Request, reqBodyBuffer []byte, data in
 	req.Header.Set("Accept", "application/vnd.protonmail.v1+json")
 
 	var cancelRequest context.CancelFunc
-	if c.cm.config.MinSpeed > 0 {
+	if c.cm.config.MinBytesPerSecond > 0 {
 		var ctx context.Context
 		ctx, cancelRequest = context.WithCancel(req.Context())
 		defer func() {
@@ -332,7 +332,7 @@ func (c *client) doJSONBuffered(req *http.Request, reqBodyBuffer []byte, data in
 	defer res.Body.Close() //nolint[errcheck]
 
 	var resBody []byte
-	if c.cm.config.MinSpeed == 0 {
+	if c.cm.config.MinBytesPerSecond == 0 {
 		resBody, err = ioutil.ReadAll(res.Body)
 	} else {
 		resBody, err = c.readAllMinSpeed(res.Body, cancelRequest)
@@ -417,17 +417,22 @@ func (c *client) readAllMinSpeed(data io.Reader, cancelRequest context.CancelFun
 	timer := time.AfterFunc(firstReadTimeout, func() {
 		cancelRequest()
 	})
+
+	// speedCheckSeconds controls how often we check the transfer speed.
+	const speedCheckSeconds = 3
+
 	var buffer bytes.Buffer
 	for {
-		_, err := io.CopyN(&buffer, data, c.cm.config.MinSpeed)
+		_, err := io.CopyN(&buffer, data, c.cm.config.MinBytesPerSecond*speedCheckSeconds)
 		timer.Stop()
-		timer.Reset(1 * time.Second)
+		timer.Reset(speedCheckSeconds * time.Second)
 		if err == io.EOF {
 			break
 		} else if err != nil {
 			return nil, err
 		}
 	}
+
 	return ioutil.ReadAll(&buffer)
 }
 
diff --git a/pkg/pmapi/client_test.go b/pkg/pmapi/client_test.go
index 3cbc94a7..238431a9 100644
--- a/pkg/pmapi/client_test.go
+++ b/pkg/pmapi/client_test.go
@@ -30,10 +30,10 @@ import (
 )
 
 var testClientConfig = &ClientConfig{
-	AppVersion:       "GoPMAPI_1.0.14",
-	ClientID:         "demoapp",
-	FirstReadTimeout: 500 * time.Millisecond,
-	MinSpeed:         256,
+	AppVersion:        "GoPMAPI_1.0.14",
+	ClientID:          "demoapp",
+	FirstReadTimeout:  500 * time.Millisecond,
+	MinBytesPerSecond: 256,
 }
 
 func newTestClient(cm *ClientManager) *client {
@@ -175,7 +175,7 @@ func TestClient_FirstReadTimeout(t *testing.T) {
 
 func TestClient_MinSpeedTimeout(t *testing.T) {
 	finish, c := newTestServerCallbacks(t,
-		routeSlow(2*time.Second),
+		routeSlow(4*time.Second), // 1 second longer than the minimum transfer speed poll time.
 	)
 	defer finish()