Merge pull request #278 from maxmind/retry-http2-internal-err

Retry HTTP2 INTERNAL_ERRORs

CHANGELOG.md

@@ -1,5 +1,10 @@
 # CHANGELOG
 
+## 7.0.0
+
+* `HTTPReader` no longer retries on HTTP errors and therefore
+  `retryFor` was removed from `NewHTTPReader`.
+
 ## 6.1.0 (2024-01-09)
 
 * `geoipupdate` now sets the version in the `User-Agent` header to the

go.mod

@@ -7,6 +7,7 @@ require (
 	github.com/gofrs/flock v0.8.1
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.8.4
+	golang.org/x/net v0.20.0
 	golang.org/x/sync v0.6.0
 )
 
@@ -16,6 +17,7 @@ require (
 	github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	golang.org/x/sys v0.16.0 // indirect
+	golang.org/x/text v0.14.0 // indirect
 	gopkg.in/check.v1 v1.0.0-20200902074654-038fdea0a05b // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )

go.sum

@@ -17,10 +17,14 @@ github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA=
 github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
 github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
 github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
+golang.org/x/net v0.20.0 h1:aCL9BSgETF1k+blQaYUBx9hJ9LOGP3gAVemcZlf1Kpo=
+golang.org/x/net v0.20.0/go.mod h1:z8BVo6PvndSri0LbOE3hAn0apkU+1YvI6E70E9jsnvY=
 golang.org/x/sync v0.6.0 h1:5BMeUDZ7vkXGfEr1x9B4bRcTH4lpkTkpdh0T/J+qjbQ=
 golang.org/x/sync v0.6.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
 golang.org/x/sys v0.16.0 h1:xWw16ngr6ZMtmxDyKyIgsE93KNKz5HKmMa3b8ALHidU=
 golang.org/x/sys v0.16.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
+golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ=
+golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/check.v1 v1.0.0-20200902074654-038fdea0a05b h1:QRR6H1YWRnHb4Y/HeNFCTJLFVxaq6wH4YuVdsUOr75U=
 gopkg.in/check.v1 v1.0.0-20200902074654-038fdea0a05b/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=

pkg/geoipupdate/database/http_reader.go

@@ -14,8 +14,6 @@ import (
 	"strconv"
 	"time"
 
-	"github.com/cenkalti/backoff/v4"
-
 	"github.com/maxmind/geoipupdate/v6/pkg/geoipupdate/internal"
 	"github.com/maxmind/geoipupdate/v6/pkg/geoipupdate/vars"
 )
@@ -33,8 +31,6 @@ type HTTPReader struct {
 	accountID int
 	// licenseKey is used for request auth.
 	licenseKey string
-	// retryFor sets the timeout for when a request can no longuer be retried.
-	retryFor time.Duration
 	// verbose turns on/off debug logs.
 	verbose bool
 }
@@ -46,7 +42,6 @@ func NewHTTPReader(
 	path string,
 	accountID int,
 	licenseKey string,
-	retryFor time.Duration,
 	verbose bool,
 ) Reader {
 	transport := http.DefaultTransport
@@ -60,7 +55,6 @@ func NewHTTPReader(
 		path:       path,
 		accountID:  accountID,
 		licenseKey: licenseKey,
-		retryFor:   retryFor,
 		verbose:    verbose,
 	}
 }
@@ -71,39 +65,7 @@ func NewHTTPReader(
 // It's the responsibility of the Writer to close the io.ReadCloser
 // included in the response after consumption.
 func (r *HTTPReader) Read(ctx context.Context, editionID, hash string) (*ReadResult, error) {
-	var result *ReadResult
-	var err error
-
-	// RetryFor value of 0 means that no retries should be performed.
-	// Max zero retries has to be set to achieve that
-	// because the backoff never stops if MaxElapsedTime is zero.
-	exp := backoff.NewExponentialBackOff()
-	exp.MaxElapsedTime = r.retryFor
-	b := backoff.BackOff(exp)
-	if exp.MaxElapsedTime == 0 {
-		b = backoff.WithMaxRetries(exp, 0)
-	}
-	err = backoff.RetryNotify(
-		func() error {
-			result, err = r.get(ctx, editionID, hash)
-			if err == nil {
-				return nil
-			}
-
-			var httpErr internal.HTTPError
-			if errors.As(err, &httpErr) && httpErr.StatusCode >= 400 && httpErr.StatusCode < 500 {
-				return backoff.Permanent(err)
-			}
-
-			return err
-		},
-		b,
-		func(err error, d time.Duration) {
-			if r.verbose {
-				log.Printf("Couldn't download %s, retrying in %v: %v", editionID, d, err)
-			}
-		},
-	)
+	result, err := r.get(ctx, editionID, hash)
 	if err != nil {
 		return nil, fmt.Errorf("getting update for %s: %w", editionID, err)
 	}

pkg/geoipupdate/database/http_reader_test.go

@@ -121,7 +121,6 @@ func TestHTTPReader(t *testing.T) {
 				server.URL, // fixed, as the server is mocked above.
 				10,         // fixed, as it's not valuable for the purpose of the test.
 				"license",  // fixed, as it's not valuable for the purpose of the test.
-				0,          // zero means no retries.
 				false,      // verbose
 			)
 

pkg/geoipupdate/database/local_file_writer.go

@@ -46,7 +46,7 @@ func NewLocalFileWriter(
 }
 
 // Write writes the result struct returned by a Reader to a database file.
-func (w *LocalFileWriter) Write(result *ReadResult) error {
+func (w *LocalFileWriter) Write(result *ReadResult) (err error) {
 	// exit early if we've got the latest database version.
 	if strings.EqualFold(result.OldHash, result.NewHash) {
 		if w.verbose {
@@ -56,8 +56,11 @@ func (w *LocalFileWriter) Write(result *ReadResult) error {
 	}
 
 	defer func() {
-		if err := result.reader.Close(); err != nil {
-			log.Printf("closing reader for %s: %+v", result.EditionID, err)
+		if closeErr := result.reader.Close(); closeErr != nil {
+			err = errors.Join(
+				err,
+				fmt.Errorf("closing reader for %s: %w", result.EditionID, closeErr),
+			)
 		}
 	}()
 
@@ -69,34 +72,37 @@ func (w *LocalFileWriter) Write(result *ReadResult) error {
 		return fmt.Errorf("setting up database writer for %s: %w", result.EditionID, err)
 	}
 	defer func() {
-		if err := fw.close(); err != nil {
-			log.Printf("closing file writer: %+v", err)
+		if closeErr := fw.close(); closeErr != nil {
+			err = errors.Join(
+				err,
+				fmt.Errorf("closing file writer: %w", closeErr),
+			)
 		}
 	}()
 
-	if err := fw.write(result.reader); err != nil {
+	if err = fw.write(result.reader); err != nil {
 		return fmt.Errorf("writing to the temp file for %s: %w", result.EditionID, err)
 	}
 
 	// make sure the hash of the temp file matches the expected hash.
-	if err := fw.validateHash(result.NewHash); err != nil {
+	if err = fw.validateHash(result.NewHash); err != nil {
 		return fmt.Errorf("validating hash for %s: %w", result.EditionID, err)
 	}
 
 	// move the temoporary database file into it's final location and
 	// sync the directory.
-	if err := fw.syncAndRename(databaseFilePath); err != nil {
+	if err = fw.syncAndRename(databaseFilePath); err != nil {
 		return fmt.Errorf("renaming temp file: %w", err)
 	}
 
 	// sync database directory.
-	if err := syncDir(filepath.Dir(databaseFilePath)); err != nil {
+	if err = syncDir(filepath.Dir(databaseFilePath)); err != nil {
 		return fmt.Errorf("syncing database directory: %w", err)
 	}
 
 	// check if we need to set the file's modified at time
 	if w.preserveFileTime {
-		if err := setModifiedAtTime(databaseFilePath, result.ModifiedAt); err != nil {
+		if err = setModifiedAtTime(databaseFilePath, result.ModifiedAt); err != nil {
 			return err
 		}
 	}

pkg/geoipupdate/geoip_updater.go

@@ -11,6 +11,8 @@ import (
 	"sync"
 	"time"
 
+	"github.com/cenkalti/backoff/v4"
+
 	"github.com/maxmind/geoipupdate/v6/pkg/geoipupdate/database"
 	"github.com/maxmind/geoipupdate/v6/pkg/geoipupdate/internal"
 )
@@ -32,7 +34,6 @@ func NewClient(config *Config) *Client {
 			config.URL,
 			config.AccountID,
 			config.LicenseKey,
-			config.RetryFor,
 			config.Verbose,
 		), nil
 	}
@@ -85,20 +86,11 @@ func (c *Client) Run(ctx context.Context) error {
 	for _, editionID := range c.config.EditionIDs {
 		editionID := editionID
 		processFunc := func(ctx context.Context) error {
-			editionHash, err := writer.GetHash(editionID)
+			edition, err := c.downloadEdition(ctx, editionID, reader, writer)
 			if err != nil {
 				return err
 			}
 
-			edition, err := reader.Read(ctx, editionID, editionHash)
-			if err != nil {
-				return err
-			}
-
-			if err := writer.Write(edition); err != nil {
-				return err
-			}
-
 			edition.CheckedAt = time.Now().In(time.UTC)
 
 			mu.Lock()
@@ -126,3 +118,60 @@ func (c *Client) Run(ctx context.Context) error {
 
 	return nil
 }
+
+// downloadEdition downloads the file with retries.
+func (c *Client) downloadEdition(
+	ctx context.Context,
+	editionID string,
+	r database.Reader,
+	w database.Writer,
+) (*database.ReadResult, error) {
+	editionHash, err := w.GetHash(editionID)
+	if err != nil {
+		return nil, err
+	}
+
+	// RetryFor value of 0 means that no retries should be performed.
+	// Max zero retries has to be set to achieve that
+	// because the backoff never stops if MaxElapsedTime is zero.
+	exp := backoff.NewExponentialBackOff()
+	exp.MaxElapsedTime = c.config.RetryFor
+	b := backoff.BackOff(exp)
+	if exp.MaxElapsedTime == 0 {
+		b = backoff.WithMaxRetries(exp, 0)
+	}
+
+	var edition *database.ReadResult
+	err = backoff.RetryNotify(
+		func() error {
+			if edition, err = r.Read(ctx, editionID, editionHash); err != nil {
+				if internal.IsPermanentError(err) {
+					return backoff.Permanent(err)
+				}
+
+				return err
+			}
+
+			if err = w.Write(edition); err != nil {
+				if internal.IsPermanentError(err) {
+					return backoff.Permanent(err)
+				}
+
+				return err
+			}
+
+			return nil
+		},
+		b,
+		func(err error, d time.Duration) {
+			if c.config.Verbose {
+				log.Printf("Couldn't download %s, retrying in %v: %v", editionID, d, err)
+			}
+		},
+	)
+	if err != nil {
+		return nil, err
+	}
+
+	return edition, nil
+}

pkg/geoipupdate/geoip_updater_test.go

@@ -10,8 +10,10 @@ import (
 	"testing"
 	"time"
 
-	"github.com/maxmind/geoipupdate/v6/pkg/geoipupdate/database"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/net/http2"
+
+	"github.com/maxmind/geoipupdate/v6/pkg/geoipupdate/database"
 )
 
 // TestClientOutput makes sure that the client outputs the result of it's
@@ -77,6 +79,21 @@ func TestClientOutput(t *testing.T) {
 			t.Errorf("database %s was not updated", outputDatabases[i].EditionID)
 		}
 	}
+
+	streamErr := http2.StreamError{
+		Code: http2.ErrCodeInternal,
+	}
+	c.getWriter = func() (database.Writer, error) {
+		w := mockWriter{
+			WriteFunc: func(_ *database.ReadResult) error {
+				return streamErr
+			},
+		}
+
+		return &w, nil
+	}
+	err = c.Run(context.Background())
+	require.ErrorIs(t, err, streamErr)
 }
 
 type mockReader struct {
@@ -93,10 +110,18 @@ func (mr *mockReader) Read(_ context.Context, _, _ string) (*database.ReadResult
 	return &res, nil
 }
 
-type mockWriter struct{}
+type mockWriter struct {
+	WriteFunc func(*database.ReadResult) error
+}
+
+func (w *mockWriter) Write(r *database.ReadResult) error {
+	if w.WriteFunc != nil {
+		return w.WriteFunc(r)
+	}
 
-func (w *mockWriter) Write(_ *database.ReadResult) error { return nil }
-func (w mockWriter) GetHash(_ string) (string, error)    { return "", nil }
+	return nil
+}
+func (w mockWriter) GetHash(_ string) (string, error) { return "", nil }
 
 func afterOrEqual(t1, t2 time.Time) bool {
 	return t1.After(t2) || t1.Equal(t2)

pkg/geoipupdate/internal/errors.go

@@ -2,6 +2,7 @@
 package internal
 
 import (
+	"errors"
 	"fmt"
 )
 
@@ -14,3 +15,13 @@ type HTTPError struct {
 func (h HTTPError) Error() string {
 	return fmt.Sprintf("received HTTP status code: %d: %s", h.StatusCode, h.Body)
 }
+
+// IsPermanentError returns true if the error is non-retriable.
+func IsPermanentError(err error) bool {
+	var httpErr HTTPError
+	if errors.As(err, &httpErr) && httpErr.StatusCode >= 400 && httpErr.StatusCode < 500 {
+		return true
+	}
+
+	return false
+}