fix #171: add breaker to context converter to prevent critical bug with http package integration

Author: kamilsk

context.go

@@ -2,47 +2,18 @@ package retry
 
 import "context"
 
-type lite struct {
-	context.Context
-	signal <-chan struct{}
-}
-
-func (ctx lite) Done() <-chan struct{} {
-	return ctx.signal
-}
-
-func (ctx lite) Err() error {
-	select {
-	case <-ctx.signal:
-		return context.Canceled
-	default:
-		return nil
+func convert(breaker Breaker) context.Context {
+	ctx, is := breaker.(context.Context)
+	if !is {
+		ctx = lite{context.Background(), breaker}
 	}
+	return ctx
 }
 
-// equal to go.octolab.org/errors.Unwrap
-func unwrap(err error) error {
-	// compatible with github.com/pkg/errors
-	type causer interface {
-		Cause() error
-	}
-	// compatible with built-in errors since 1.13
-	type wrapper interface {
-		Unwrap() error
-	}
-
-	for err != nil {
-		layer, is := err.(wrapper)
-		if is {
-			err = layer.Unwrap()
-			continue
-		}
-		cause, is := err.(causer)
-		if is {
-			err = cause.Cause()
-			continue
-		}
-		break
-	}
-	return err
+type lite struct {
+	context.Context
+	breaker Breaker
 }
+
+func (ctx lite) Done() <-chan struct{} { return ctx.breaker.Done() }
+func (ctx lite) Err() error            { return ctx.breaker.Err() }

context_test.go

@@ -2,6 +2,7 @@ package retry
 
 import (
 	"context"
+	"reflect"
 	"runtime"
 	"testing"
 	"time"
@@ -13,7 +14,7 @@ func TestContext(t *testing.T) {
 
 		var (
 			sig = make(chan struct{})
-			ctx = context.Context(lite{context.TODO(), sig})
+			ctx = context.Context(lite{context.TODO(), breaker(sig)})
 		)
 		if ctx.Err() != nil {
 			t.Error("invalid state")
@@ -32,7 +33,7 @@ func TestContext(t *testing.T) {
 
 		var (
 			sig = make(chan struct{})
-			ctx = context.Context(lite{context.TODO(), sig})
+			ctx = context.Context(lite{context.TODO(), breaker(sig)})
 		)
 		if ctx.Err() != nil {
 			t.Error("invalid state")
@@ -51,7 +52,7 @@ func TestContext(t *testing.T) {
 
 		var (
 			sig = make(chan struct{})
-			ctx = context.Context(lite{context.TODO(), sig})
+			ctx = context.Context(lite{context.TODO(), breaker(sig)})
 		)
 		if ctx.Err() != nil {
 			t.Error("invalid state")
@@ -70,7 +71,7 @@ func TestContext(t *testing.T) {
 
 		var (
 			sig = make(chan struct{})
-			ctx = context.Context(lite{context.TODO(), sig})
+			ctx = context.Context(lite{context.TODO(), breaker(sig)})
 		)
 		if ctx.Err() != nil {
 			t.Error("invalid state")
@@ -85,6 +86,30 @@ func TestContext(t *testing.T) {
 	})
 }
 
+func TestConvert(t *testing.T) {
+	t.Run("breaker", func(t *testing.T) {
+		br := make(breaker)
+
+		ctx := convert(br)
+		if ctx.Err() != nil {
+			t.Error("invalid state")
+		}
+
+		close(br)
+		if ctx.Err() == nil {
+			t.Error("invalid state")
+		}
+	})
+
+	t.Run("context", func(t *testing.T) {
+		ctx := context.TODO()
+
+		if !reflect.DeepEqual(convert(ctx), ctx) {
+			t.Error("unexpected behavior")
+		}
+	})
+}
+
 // helpers
 
 func stop(timer *time.Timer) {
@@ -116,4 +141,16 @@ func verify(t *testing.T, ctx context.Context, cancel context.CancelFunc, sig ch
 
 type key struct{}
 
+type breaker chan struct{}
+
+func (br breaker) Done() <-chan struct{} { return br }
+func (br breaker) Err() error {
+	select {
+	case <-br:
+		return context.Canceled
+	default:
+		return nil
+	}
+}
+
 var schedule = 10 * time.Duration(runtime.NumCPU()) * time.Millisecond

errors.go

@@ -0,0 +1,39 @@
+package retry
+
+const internal Error = "have no any try"
+
+// Error defines a string-based error without a different root cause.
+type Error string
+
+// Error returns a string representation of an error.
+func (err Error) Error() string { return string(err) }
+
+// Unwrap always returns nil means that an error doesn't have other root cause.
+func (err Error) Unwrap() error { return nil }
+
+// equal to go.octolab.org/errors.Unwrap
+func unwrap(err error) error {
+	// compatible with github.com/pkg/errors
+	type causer interface {
+		Cause() error
+	}
+	// compatible with built-in errors since 1.13
+	type wrapper interface {
+		Unwrap() error
+	}
+
+	for err != nil {
+		layer, is := err.(wrapper)
+		if is {
+			err = layer.Unwrap()
+			continue
+		}
+		cause, is := err.(causer)
+		if is {
+			err = cause.Cause()
+			continue
+		}
+		break
+	}
+	return err
+}

errors_test.go

@@ -0,0 +1,35 @@
+package retry
+
+import (
+	"errors"
+	"reflect"
+	"testing"
+)
+
+func TestError(t *testing.T) {
+	if internal.Error() != string(internal) {
+		t.Error("unexpected behavior")
+	}
+
+	if internal.Unwrap() != nil {
+		t.Error("unexpected behavior")
+	}
+}
+
+func TestUnwrap(t *testing.T) {
+	cause := errors.New("root")
+	core := unwrap(causer{layer{cause}})
+	if !reflect.DeepEqual(core, cause) {
+		t.Error("unexpected behavior")
+	}
+}
+
+// helpers
+
+type causer struct{ error }
+
+func (causer causer) Cause() error { return causer.error }
+
+type layer struct{ error }
+
+func (layer layer) Unwrap() error { return layer.error }

retry.go

@@ -8,53 +8,50 @@ package retry
 import (
 	"context"
 	"fmt"
-
-	"github.com/kamilsk/retry/v5/strategy"
 )
 
 // Action defines a callable function that package retry can handle.
 type Action = func(context.Context) error
 
-// Error defines a string-based error without a different root cause.
-type Error string
-
-// Error returns a string representation of an error.
-func (err Error) Error() string { return string(err) }
-
-// Unwrap always returns nil means that an error doesn't have other root cause.
-func (err Error) Unwrap() error { return nil }
+// A Breaker carries a cancellation signal to interrupt an action execution.
+//
+// It is a subset of the built-in context and github.com/kamilsk/breaker interfaces.
+type Breaker = interface {
+	// Done returns a channel that's closed when a cancellation signal occurred.
+	Done() <-chan struct{}
+	// If Done is not yet closed, Err returns nil.
+	// If Done is closed, Err returns a non-nil error.
+	// After Err returns a non-nil error, successive calls to Err return the same error.
+	Err() error
+}
 
 // How is an alias for batch of Strategies.
 //
 //  how := retry.How{
 //  	strategy.Limit(3),
 //  }
 //
-type How = []func(strategy.Breaker, uint, error) bool
+type How = []func(Breaker, uint, error) bool
 
 // Do takes the action and performs it, repetitively, until successful.
 //
 // Optionally, strategies may be passed that assess whether or not an attempt
 // should be made.
 func Do(
-	breaker strategy.Breaker,
+	breaker Breaker,
 	action func(context.Context) error,
-	strategies ...func(strategy.Breaker, uint, error) bool,
+	strategies ...func(Breaker, uint, error) bool,
 ) error {
 	var (
-		err   error = Error("have no any try")
-		clean error
+		ctx        = convert(breaker)
+		err  error = internal
+		core error
 	)
 
-	ctx, is := breaker.(context.Context)
-	if !is {
-		ctx = lite{context.Background(), breaker.Done()}
-	}
-
 	for attempt, should := uint(0), true; should; attempt++ {
-		clean = unwrap(err)
+		core = unwrap(err)
 		for i, repeat := 0, len(strategies); should && i < repeat; i++ {
-			should = should && strategies[i](breaker, attempt, clean)
+			should = should && strategies[i](breaker, attempt, core)
 		}
 
 		select {
@@ -78,9 +75,9 @@ func Do(
 // Optionally, strategies may be passed that assess whether or not an attempt
 // should be made.
 func Go(
-	breaker strategy.Breaker,
+	breaker Breaker,
 	action func(context.Context) error,
-	strategies ...func(strategy.Breaker, uint, error) bool,
+	strategies ...func(Breaker, uint, error) bool,
 ) error {
 	done := make(chan error, 1)
 

retry_test.go

@@ -2,7 +2,6 @@ package retry_test
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"reflect"
 	"testing"
@@ -100,11 +99,11 @@ func TestGo(t *testing.T) {
 
 // helpers
 
-func breaker() strategy.Breaker {
+func breaker() Breaker {
 	return context.TODO()
 }
 
-func interrupted() strategy.Breaker {
+func interrupted() Breaker {
 	ctx, cancel := context.WithCancel(context.TODO())
 	cancel()
 	return ctx
@@ -127,7 +126,7 @@ func (layer layer) Unwrap() error { return layer.error }
 
 type testCase struct {
 	name       string
-	breaker    strategy.Breaker
+	breaker    Breaker
 	strategies How
 	action     func(context.Context) error
 	expected   expected
@@ -145,8 +144,8 @@ var testCases = []testCase{
 		"failed action call",
 		breaker(),
 		How{strategy.Limit(10)},
-		func(context.Context) error { return layer{causer{errors.New("failure")}} },
-		expected{10, layer{causer{errors.New("failure")}}},
+		func(context.Context) error { return layer{causer{Error("failure")}} },
+		expected{10, layer{causer{Error("failure")}}},
 	},
 	{
 		"action call with interrupted breaker",
@@ -159,7 +158,7 @@ var testCases = []testCase{
 		"have no action call",
 		breaker(),
 		How{strategy.Limit(0)},
-		func(context.Context) error { return layer{causer{errors.New("failure")}} },
+		func(context.Context) error { return layer{causer{Error("failure")}} },
 		expected{0, Error("have no any try")},
 	},
 }