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.}

This change is 

We have two problems here.
@yuzefovich as the reviewer of #103866 I'd like you to double check what I write below.

1. Goroutine saving - are you sure?

allows for the same functionality without the additional goroutine cost.

This is not true in general. Have you opened the black box? Look at the go source code, this propagateCancel spawns a goroutine to do the job:

             atomic.AddInt32(&goroutines, +1)
             go func() {
                     select {
                     case <-parent.Done():
                             child.cancel(false, parent.Err())
                     case <-child.Done():
                     }
             }()

2. Possible goroutine leak!

ctxutil.WhenDone (added by dfe4ef8)

I double checked this and I don't like what I am seeing.

If you look at the go source code for this internal propagateCancel" function, you see that it spawns a goroutine. That goroutine only ever terminates if either the child or parent context is canceled.

So what if they don't get canceled at all? Like, if our code paths pass a context.Background or similar?

At the very least with our code base, when someone spawns a goroutine they know they have to listen on the stopper ShouldQuiesce channel. This is also encouraged in the API of our retry package.

This new ctxutil.WhenDone does not encourage that. Worse yet, it terminates immediately (i.e. it does not block execution in the caller) which means there is no synchronization on termination of that implicit goroutine.

Recommendation: remove ctxutil altogether

If the above is confirmed, I would recommend removing the current impl of ctxutil altogether.

If we really need this functionality, we need to build a patch to the go runtime to do what we want (we have a patch infrastructure now! let's use it!) while ensuring that stray goroutines don't leak.

Thanks for looking into this @knz, but I think your concerns can be assuaged. WhenDone() can only take a context with Done() != nil, i.e. a cancellable context derived from WithCancel or WithTimeout:

If you look at the go source code for this internal propagateCancel" function, you see that it spawns a goroutine.

Only if the parent context isn't cancellable. We've already established that it must be, so it takes this code path here, which doesn't spawn a goroutine:

https://github.com/golang/go/blob/5fe3f0a265c90a9c0346403742c6cafeb154503b/src/context/context.go#L473-L487

So what if they don't get canceled at all? Like, if our code paths pass a context.Background or similar?

We've established that the context must be cancellable, so the caller is responsible for avoiding goroutine leaks by calling the associated cancel function.

I think the only concern here would be if we use a custom context implementation that overrides Done(), in which case parentCancelCtx won't be able to resolve the parent cancelCtx and attach to it.

That said, I'm not super-thrilled about accessing Go internals like this. This stuff is internal for a reason.

I'll give this a closer review.

FWIW, I also verified that this gets rid of the superfluous goroutine.

@knz -- what @erikgrinaker said.

Have you opened the black box?
Obviously, I have in order to implement ctxutil package. If you do some shenanigans and override context, then
the default code path is taken that does spin up goroutine. But, we don't do that; and in the common case of
context.WithTimeout/WithCancel, the code does not spawn anything.

If we really need this functionality, we need to build a patch to the go runtime to do what we want (we have a patch infrastructure now! let's use it!) while ensuring that stray goroutines don't leak.

Goroutines do not leak with this change. The behavior is no different from the default implementation of context
which spins up go routine if the implementation is anything other than the 2 "approved" cancelers.
The last time context.go changed was the first time go was released. I think a ~20 year lifetime is not that terrible, and of course, we could try patching things locally.

re @erikgrinaker

That said, I'm not super-thrilled about accessing Go internals like this. This stuff is internal for a reason.
Me neither; Having said that, a) this particular implementation does not change frequently -- first time in 1.20; b) we do occasionally access those internal implementations ("git grep linkname") -- it's not done frequently, nor in a wonton way.
If there was another way to do it, I would.

I have searched quite extensively on this topic; there were proposals to extend context.go somehow; there was always a pushback.

We have two problems here.
@yuzefovich as the reviewer of #103866 I'd like you to double check what I write below.

@knz -- I don't think there are two problems here.
The reason why the change reviewed by @yuzefovich was backed out is that while
this implementation is faster than checking for ->Done() (~8x), it is not 128 (or even 1024) times faster since
that's how frequent the Done checks are in vectorized engine.

I am friendly to the change now. You have successfully changed my opinion.

@erikgrinaker @knz
Please take another look. I expanded test coverage in ctxutil package; Added utility function to check
if we can directly propagate context, and added a check which triggers panic under test.

bors r+

Build succeeded:

• Bazel Essential CI (Cockroach)