copy: remove max number of ErrResets

jedevc · jedevc · commit a8004007a2ab · 2024-03-04T12:59:44.000Z
If a writer continually asks to be reset then it should always succeed -
it should be the responsibility of the underlying content.Writer to
stop producing ErrReset after some amount of time and to instead return
the underlying issue - which pushWriter already does today, using the
doWithRetries function.

doWithRetries already has a separate cap for retries of 6 requests (5
retries after the original failure), and it seems like this would be
previously overridden by content.Copy's max number of 5 attempts, hiding
the original error.

Signed-off-by: Justin Chadwell &lt;me@jedevc.com&gt;
diff --git a/content/helpers.go b/content/helpers.go
@@ -31,9 +31,6 @@ import (
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 )
 
-// maxResets is the no.of times the Copy() method can tolerate a reset of the body
-const maxResets = 5
-
 var ErrReset = errors.New("writer has been reset")
 
 var bufPool = sync.Pool{
@@ -160,7 +157,7 @@ func Copy(ctx context.Context, cw Writer, or io.Reader, size int64, expected dig
 		}
 	}
 
-	for i := 0; i < maxResets; i++ {
+	for i := 0; ; i++ {
 		if i >= 1 {
 			log.G(ctx).WithField("digest", expected).Debugf("retrying copy due to reset")
 		}
@@ -201,9 +198,6 @@ func Copy(ctx context.Context, cw Writer, or io.Reader, size int64, expected dig
 		}
 		return nil
 	}
-
-	log.G(ctx).WithField("digest", expected).Errorf("failed to copy after %d retries", maxResets)
-	return fmt.Errorf("failed to copy after %d retries", maxResets)
 }
 
 // CopyReaderAt copies to a writer from a given reader at for the given
diff --git a/content/helpers_test.go b/content/helpers_test.go
@@ -20,7 +20,7 @@ import (
 	"bytes"
 	"context"
 	_ "crypto/sha256" // required by go-digest
-	"fmt"
+	"errors"
 	"io"
 	"strings"
 	"testing"
@@ -42,7 +42,7 @@ func TestCopy(t *testing.T) {
 	cf1 := func(buf *bytes.Buffer, st Status) commitFunction {
 		i := 0
 		return func() error {
-			// function resets the first time
+			// function resets the first time, but then succeeds after
 			if i == 0 {
 				// this is the case where, the pipewriter to which the data was being written has
 				// changed. which means we need to clear the buffer
@@ -55,11 +55,28 @@ func TestCopy(t *testing.T) {
 		}
 	}
 
+	cf2err := errors.New("commit failed")
 	cf2 := func(buf *bytes.Buffer, st Status) commitFunction {
 		i := 0
 		return func() error {
-			// function resets for more than the maxReset value
-			if i < maxResets+1 {
+			// function resets a lot of times, and eventually fails
+			if i < 10 {
+				// this is the case where, the pipewriter to which the data was being written has
+				// changed. which means we need to clear the buffer
+				i++
+				buf.Reset()
+				st.Offset = 0
+				return ErrReset
+			}
+			return cf2err
+		}
+	}
+
+	cf3 := func(buf *bytes.Buffer, st Status) commitFunction {
+		i := 0
+		return func() error {
+			// function resets a lot of times, and eventually succeeds
+			if i < 10 {
 				// this is the case where, the pipewriter to which the data was being written has
 				// changed. which means we need to clear the buffer
 				i++
@@ -73,8 +90,10 @@ func TestCopy(t *testing.T) {
 
 	s1 := Status{}
 	s2 := Status{}
+	s3 := Status{}
 	b1 := bytes.Buffer{}
 	b2 := bytes.Buffer{}
+	b3 := bytes.Buffer{}
 
 	var testcases = []struct {
 		name        string
@@ -130,15 +149,25 @@ func TestCopy(t *testing.T) {
 			expected: "content to copy",
 		},
 		{
-			name:   "write fails more than maxReset times due to reset",
+			name:   "write fails after lots of resets",
 			source: newCopySource("content to copy"),
 			writer: fakeWriter{
 				Buffer:     &b2,
 				status:     s2,
 				commitFunc: cf2(&b2, s2),
 			},
 			expected:    "",
-			expectedErr: fmt.Errorf("failed to copy after %d retries", maxResets),
+			expectedErr: cf2err,
+		},
+		{
+			name:   "write succeeds after lots of resets",
+			source: newCopySource("content to copy"),
+			writer: fakeWriter{
+				Buffer:     &b3,
+				status:     s3,
+				commitFunc: cf3(&b3, s3),
+			},
+			expected: "content to copy",
 		},
 	}
 
@@ -153,7 +182,7 @@ func TestCopy(t *testing.T) {
 
 			// if an error is expected then further comparisons are not required
 			if testcase.expectedErr != nil {
-				assert.Equal(t, testcase.expectedErr, err)
+				assert.ErrorIs(t, err, testcase.expectedErr)
 				return
 			}
 

Original file line number	Diff line number	Diff line change
`@@ -31,9 +31,6 @@ import (`
`31`	`31`	`ocispec "github.com/opencontainers/image-spec/specs-go/v1"`
`32`	`32`	`)`
`33`	`33`
`34`		`-// maxResets is the no.of times the Copy() method can tolerate a reset of the body`
`35`		`-const maxResets = 5`
`36`		`-`
`37`	`34`	`var ErrReset = errors.New("writer has been reset")`
`38`	`35`
`39`	`36`	`var bufPool = sync.Pool{`
`@@ -160,7 +157,7 @@ func Copy(ctx context.Context, cw Writer, or io.Reader, size int64, expected dig`
`160`	`157`	`}`
`161`	`158`	`}`
`162`	`159`
`163`		`- for i := 0; i < maxResets; i++ {`
	`160`	`+ for i := 0; ; i++ {`
`164`	`161`	`if i >= 1 {`
`165`	`162`	`log.G(ctx).WithField("digest", expected).Debugf("retrying copy due to reset")`
`166`	`163`	`}`
`@@ -201,9 +198,6 @@ func Copy(ctx context.Context, cw Writer, or io.Reader, size int64, expected dig`
`201`	`198`	`}`
`202`	`199`	`return nil`
`203`	`200`	`}`
`204`		`-`
`205`		`- log.G(ctx).WithField("digest", expected).Errorf("failed to copy after %d retries", maxResets)`
`206`		`- return fmt.Errorf("failed to copy after %d retries", maxResets)`
`207`	`201`	`}`
`208`	`202`
`209`	`203`	`// CopyReaderAt copies to a writer from a given reader at for the given`