Close ClientStreaming on context cancellation

[emcfarlane]

CloseRequest and CloseResponse must both be called before the span can be ended. Remove onCloseCalled bool as it's easy to derive.

Updates

On CloseResponse we must have stopped the connection and it is now safe to close the span. To handle context cancellation and ensure the span is always closed context.AfterFunc is added with a simpler backport for !go1.21.

[jhump]

CloseRequest and CloseResponse must both be called before the span can be ended.

Is this really true? I think only CloseResponse must be called. If the response side is closed, the request side cannot be open, right? If that's not true, I think there's probably a bug (maybe even a potential deadlock issue) in the underlying connect-go library.

FWIW, I also think that cancelling the context used to create the RPC can be done, instead of calling CloseResponse. But I suppose the only way to catch that, w/out a call to CloseResponse, would be to add a goroutine that awaits ctx.Done() (which might be more resources than we should use for instrumentation...).

[emcfarlane]

@jhump my understanding was CloseRequest and CloseResponse are used as notifiers of the end of input/output and depending on the application use case may be called in any order.

context.AfterFunc would be a nice use case for this cleanup logic.

[jhump]

may be called in any order

This doesn't make sense to me. Once CloseResponse returns, the whole operation is done. I don't see how calling CloseRequest can possibly do anything other than be a no-op at that point. A client cannot half-close only the receive side of an HTTP/2 stream -- it can only half-close the send side.

context.AfterFunc would be a nice use case for this cleanup logic.

Ooh, nice. Yeah that's the way to go. (And a build tag could be used to provide a more heavy-weight implementation for folks not running Go 1.21.)

[emcfarlane]

@jhump updated with a context.AfterFunc impl. I think CloseRequest and CloseResponse will always be called but yep on CloseResponse return we should be closing and don't have to wait for CloseRequest as the ResponseBody close won't be able to continue sending requests.

Related issue around recording metrics on ctx cancel: open-telemetry/opentelemetry-go#4671

[jhump]

Minor nit about sync.Once vs atomic.Bool. Otherwise, LGTM.

[jhump]

@emcfarlane, looks like there are merge conflicts from #153 that must be addressed