From 7fa2b39187b15099a7f1edd74d6dff082d079f2d Mon Sep 17 00:00:00 2001 From: Matheus Pimenta Date: Fri, 2 Sep 2022 15:28:50 +0100 Subject: [PATCH 1/4] bugfix: do not swallow context error when context is done --- internal/gensupport/send.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/gensupport/send.go b/internal/gensupport/send.go index 70a8e01c1b3..1283ba28e2c 100644 --- a/internal/gensupport/send.go +++ b/internal/gensupport/send.go @@ -98,10 +98,11 @@ func sendAndRetry(ctx context.Context, client *http.Client, req *http.Request, r case <-ctx.Done(): // If we got an error, and the context has been canceled, // the context's error is probably more useful. - if err == nil { - err = ctx.Err() + retErr := ctx.Err() + if err != nil { // let's still return a previous error if any + retErr = fmt.Errorf("context error: %v. previous send error: %w", ctx.Err(), err) } - return resp, err + return resp, retErr case <-time.After(pause): } From 78006b1e1c992511fb902b8ff00b95c9920287a9 Mon Sep 17 00:00:00 2001 From: Matheus Pimenta Date: Fri, 2 Sep 2022 15:28:50 +0100 Subject: [PATCH 2/4] bugfix: wrap both ctx and service call errors --- internal/gensupport/send.go | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/internal/gensupport/send.go b/internal/gensupport/send.go index 1283ba28e2c..3f94531d407 100644 --- a/internal/gensupport/send.go +++ b/internal/gensupport/send.go @@ -17,6 +17,27 @@ import ( "github.com/googleapis/gax-go/v2" ) +// Use this error type to return an error which allows introspection of both +// the context error and the error from the service. +type wrappedCallErr struct { + ctxErr error + wrappedErr error +} + +func (e wrappedCallErr) Error() string { + return fmt.Sprintf("retry failed with %v; last error: %v", e.ctxErr, e.wrappedErr) +} + +func (e wrappedCallErr) Unwrap() error { + return e.wrappedErr +} + +// Is allows errors.Is to match the error from the call as well as context +// sentinel errors. +func (e wrappedCallErr) Is(target error) bool { + return errors.Is(e.ctxErr, target) || errors.Is(e.wrappedErr, target) +} + // SendRequest sends a single HTTP request using the given client. // If ctx is non-nil, it calls all hooks, then sends the request with // req.WithContext, then calls any functions returned by the hooks in @@ -43,7 +64,7 @@ func send(ctx context.Context, client *http.Client, req *http.Request) (*http.Re if err != nil { select { case <-ctx.Done(): - err = ctx.Err() + err = wrappedCallErr{ctx.Err(), err} default: } } @@ -98,11 +119,10 @@ func sendAndRetry(ctx context.Context, client *http.Client, req *http.Request, r case <-ctx.Done(): // If we got an error, and the context has been canceled, // the context's error is probably more useful. - retErr := ctx.Err() - if err != nil { // let's still return a previous error if any - retErr = fmt.Errorf("context error: %v. previous send error: %w", ctx.Err(), err) + if err != nil { + return resp, wrappedCallErr{ctx.Err(), err} } - return resp, retErr + return resp, ctx.Err() case <-time.After(pause): } @@ -111,10 +131,10 @@ func sendAndRetry(ctx context.Context, client *http.Client, req *http.Request, r // select is satisfied at the same time, Go will choose one arbitrarily. // That can cause an operation to go through even if the context was // canceled before. - if err == nil { - err = ctx.Err() + if err != nil { + return resp, wrappedCallErr{ctx.Err(), err} } - return resp, err + return resp, ctx.Err() } invocationHeader := fmt.Sprintf("gccl-invocation-id/%s gccl-attempt-count/%d", invocationID, attempts) xGoogHeader := strings.Join([]string{invocationHeader, baseXGoogHeader}, " ") From a247f2d023b6df4e3b6642659683c75d5cfc6e11 Mon Sep 17 00:00:00 2001 From: Matheus Pimenta Date: Wed, 7 Sep 2022 18:00:36 +0100 Subject: [PATCH 3/4] fix comment as suggested by @tritone --- internal/gensupport/send.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/gensupport/send.go b/internal/gensupport/send.go index 3f94531d407..6cfbe8d1e25 100644 --- a/internal/gensupport/send.go +++ b/internal/gensupport/send.go @@ -117,8 +117,8 @@ func sendAndRetry(ctx context.Context, client *http.Client, req *http.Request, r for { select { case <-ctx.Done(): - // If we got an error, and the context has been canceled, - // the context's error is probably more useful. + // If we got an error and the context has been canceled, return an error acknowledging + // both the context cancelation and the service error. if err != nil { return resp, wrappedCallErr{ctx.Err(), err} } From 9a8dddd5b1aedf5626d486259aef6e3df826e61a Mon Sep 17 00:00:00 2001 From: Matheus Pimenta Date: Wed, 7 Sep 2022 18:07:03 +0100 Subject: [PATCH 4/4] hold send() change as suggested by @tritone --- internal/gensupport/send.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/gensupport/send.go b/internal/gensupport/send.go index 6cfbe8d1e25..dd24139b366 100644 --- a/internal/gensupport/send.go +++ b/internal/gensupport/send.go @@ -64,7 +64,7 @@ func send(ctx context.Context, client *http.Client, req *http.Request) (*http.Re if err != nil { select { case <-ctx.Done(): - err = wrappedCallErr{ctx.Err(), err} + err = ctx.Err() default: } }