sql: make ctxWithStacktrace compatible with gRPC #96483

ecwall · 2023-02-03T03:13:11Z

type ctxWithStacktrace struct was added here #95797 to add a stacktrace
(via errors.WithStack(...) when Context.Err() is called inside
contextutil.RunWithTimeout(...).

This ended up causing an issue if gRPC calls ctxWithStacktrace.Err()
because it is expecting context.DeadlineExceeded to not be wrapped with
errors.WithStack - the wrong status code will be returned in the error
returned here https://github.com/grpc/grpc-go/blob/v1.46.0/rpc_util.go#L833.

To resolve this, store the wrapped exception separately inside
ctxWithStacktrace when ctxWithStacktrace.Err() is called and set this to
TimeoutError.cause inside contextutil.RunWithTimeout(...).

Release note: None

blathers-crl · 2023-02-03T03:13:14Z

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2023-02-03T03:13:19Z

This change is

rafiss · 2023-02-03T16:52:21Z

pkg/util/contextutil/context.go

+	if err == nil {
+		return nil
+	}
+	ctx.mu.Lock()


do you think this would be a good place to use syncutil.Once https://pkg.go.dev/go4.org/syncutil#Once.Do

Once would work if errWithStacktrace were only being accessed from inside Do, but it is also accessed from errWithStacktrace so a mutex is needed that guards both.

ah ok, makes sense

andreimatei

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall, @otan, and @rafiss)

-- commits line 16 at r1:
I know I've suggested this, but I believe it doesn't work when the context expiration is detected by calling Err on a child ctx created by WithCancel (and maybe also by WithDeadline/WithTimeout, I'm not sure) - which I think is the common case when the deadline comes from an rpc call or a sql query or most things, really.
If that's true, I'd reconsider whether having this hack is worth it. Is it ever going to help?
Or, to phrase it as a question - have you verified that this actually shows an interesting stack in at least some cases for the particular deadline that prompted this patch?

pkg/util/contextutil/context.go line 91 at r1 (raw file):

	mu struct {
		syncutil.Mutex
		errWithStacktrace error

maybe capturedErr

pkg/util/contextutil/context.go line 95 at r1 (raw file):

}

func (ctx *contextWithStacktrace) ErrWithStacktrace() error {

Let's find a more suggestive name for this. Maybe CapturedErr?
And put a comment saying something about how it indicates the first detector of the expiration (kinda, since it doesn't deal with Done).
And also say something about how it doesn't work if there's a context.WithCancel under it.

Also, don't export it if the type is not exported.

pkg/util/contextutil/context.go line 95 at r1 (raw file):

}

func (ctx *contextWithStacktrace) ErrWithStacktrace() error {

As written, this can return nil even for an expired ctx. End even for a context that was detected to have expired (through calling Err on a child produced by context.WithCancel. Are you sure that's what you want?
The one caller you have does use Combine(), which deals with a nil, but still.
Consider calling Err().

pkg/util/tracing/grpcinterceptor/grpc_interceptor_test.go line 134 at r1 (raw file):

		},
		{
			name:     "UnaryUnary_ContextTimeout_DeadlineExceeded",

I don't think the new tests are worth it. I think all we need to test is that our ctx implementation respects the contract. I think these "integration tests" will no t make much sense to any reader.

rafiss · 2023-02-03T17:26:30Z

oh oops, missed Andrei's comments. I'll leave the review to him. could you share an example of what the get-user log looks like now?

ecwall

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @otan)

-- commits line 16 at r1: