transport: fix transparent retries when per-RPC credentials are in use (#4785)

Author: dfawley

internal/transport/http2_client.go

@@ -616,12 +616,22 @@ func (t *http2Client) getCallAuthData(ctx context.Context, audience string, call
 	return callAuthData, nil
 }
 
-// NewStreamError wraps an error and reports additional information.
+// NewStreamError wraps an error and reports additional information.  Typically
+// NewStream errors result in transparent retry, as they mean nothing went onto
+// the wire.  However, there are two notable exceptions:
+//
+// 1. If the stream headers violate the max header list size allowed by the
+//    server.  In this case there is no reason to retry at all, as it is
+//    assumed the RPC would continue to fail on subsequent attempts.
+// 2. If the credentials errored when requesting their headers.  In this case,
+//    it's possible a retry can fix the problem, but indefinitely transparently
+//    retrying is not appropriate as it is likely the credentials, if they can
+//    eventually succeed, would need I/O to do so.
 type NewStreamError struct {
 	Err error
 
-	DoNotRetry  bool
-	PerformedIO bool
+	DoNotRetry            bool
+	DoNotTransparentRetry bool
 }
 
 func (e NewStreamError) Error() string {
@@ -631,24 +641,10 @@ func (e NewStreamError) Error() string {
 // NewStream creates a stream and registers it into the transport as "active"
 // streams.  All non-nil errors returned will be *NewStreamError.
 func (t *http2Client) NewStream(ctx context.Context, callHdr *CallHdr) (_ *Stream, err error) {
-	defer func() {
-		if err != nil {
-			nse, ok := err.(*NewStreamError)
-			if !ok {
-				nse = &NewStreamError{Err: err}
-			}
-			if len(t.perRPCCreds) > 0 || callHdr.Creds != nil {
-				// We may have performed I/O in the per-RPC creds callback, so do not
-				// allow transparent retry.
-				nse.PerformedIO = true
-			}
-			err = nse
-		}
-	}()
 	ctx = peer.NewContext(ctx, t.getPeer())
 	headerFields, err := t.createHeaderFields(ctx, callHdr)
 	if err != nil {
-		return nil, err
+		return nil, &NewStreamError{Err: err, DoNotTransparentRetry: true}
 	}
 	s := t.newStream(ctx, callHdr)
 	cleanup := func(err error) {
@@ -748,7 +744,7 @@ func (t *http2Client) NewStream(ctx context.Context, callHdr *CallHdr) (_ *Strea
 			return true
 		}, hdr)
 		if err != nil {
-			return nil, err
+			return nil, &NewStreamError{Err: err}
 		}
 		if success {
 			break
@@ -759,12 +755,12 @@ func (t *http2Client) NewStream(ctx context.Context, callHdr *CallHdr) (_ *Strea
 		firstTry = false
 		select {
 		case <-ch:
-		case <-s.ctx.Done():
-			return nil, ContextErr(s.ctx.Err())
+		case <-ctx.Done():
+			return nil, &NewStreamError{Err: ContextErr(ctx.Err())}
 		case <-t.goAway:
-			return nil, errStreamDrain
+			return nil, &NewStreamError{Err: errStreamDrain}
 		case <-t.ctx.Done():
-			return nil, ErrConnClosing
+			return nil, &NewStreamError{Err: ErrConnClosing}
 		}
 	}
 	if t.statsHandler != nil {

stream.go

@@ -547,7 +547,7 @@ func (cs *clientStream) shouldRetry(err error) (bool, error) {
 		// In the event of a non-IO operation error from NewStream, we never
 		// attempted to write anything to the wire, so we can retry
 		// indefinitely.
-		if !nse.PerformedIO {
+		if !nse.DoNotTransparentRetry {
 			return true, nil
 		}
 	}