proposal: net/http: add context cancelation reason for server handlers

[hummerd]

Proposal Details

While serving HTTP request we have req.Context() that can be propagated down the pipe. This context can be canceled either by standard library - by http.Server itself in case of communication issues or by http.TimeoutHandler. But also it can be canceled by user code.
It would be nice to have ability to clarify the reason why context was canceled (in case it was canceled by standard library). Logged reason will make you 100% sure and should make investigation of such context cancellation errors easier.

I'm ready to make a PR in case this proposal is considered useful.

[mauri870]

You can make use of the Cause apis to do that, context.WithDeadlineCause, context.WithCancelCause, context.WithTimeoutCause and so on, and then look at context.Cause for the cancellation cause.

[hummerd]

You can make use of the Cause apis to do that, context.WithDeadlineCause, context.WithCancelCause, context.WithTimeoutCause and so on, and then look at context.Cause for the cancellation cause.

The idea of the proposal is to add cause for cases when context is canceled within standard library itself. And yes it can be accomplished with context.WithCancelCause and context.WithTimeoutCause.

[kszafran]

This would be very useful. Nowadays, when I see that a request failed due to context canceled, I have no idea happened. It could be a timeout on a SQL query, some internal request, etc. And if the context was canceled, because the client closed the connection (which happens in (*connReader).backgrounRead() -> cr.handleReadError(err), as far as I'm able to tell), I'd like to be able to distinguish it and not log it as an error.

[seankhliao]

cc @neild

[sgielen]

I'd like to work on this. I haven't contributed before, but I think this could be a good first issue and I'm running into it.

My proposal would be to change net/http.conn.cancelCtx into a context.CancelCauseFunc. Any callers now need a cause error, which I believe can be any error. This would be backwards compatible, since the ctx.Err() still returns context.Canceled, but the context.Cause() now newly returns the error from net.http. (Only implementations that were depending on context.Cause() also returning context.Canceled might break. Would this be considered acceptable?

conn.cancelCtx() is called in three places:

• deferred by serve(), where I would use a newly defined net/http.ErrConnectionHandled. I assume here that calling the CancelCauseFunc multiple times is a no-op, and the first cause is kept.
• handleReadError from the background reader, where I would use a newly defined net/http.ErrConnectionClosed if the read error was io.EOF, and the read error otherwise (suggestions welcome?)
• Write() from the checkConnErrorWriter, which I believe is used to check whether we can re-use existing HTTP connections, so I don't think the cause matters, here; the cancelCtx() is only called for cleanup (needs a double-check).

Feedback is welcome. In the meantime, I'll be following the contribution guide.

[gopherbot]

Change https://go.dev/cl/636235 mentions this issue: net/http: add context cancellation reason for server handlers

[sgielen]

Mainly, I believe this proposal requires consensus about the following three net/http contract/API changes:

1. When a server handler context is cancelled by net/http, the Cause was set to the default context.Canceled, but now it is set to a more specific error. (The context Error remains set to context.Canceled, as always.)
2. The newly exposed http.ErrConnectionClosed is used when the client closed the connection between end of request and the server handler return.
3. The newly exposed http.ErrRequestHandlerReturned is used when the server handler returned.

[neild]

I don't think this needs to be a proposal.

I don't see a good reason to expose ErrConnectionClosed or ErrRequestHandlerReturned. I can't think of many, if any, situations when a handler would need to distinguish between these conditions. If all we want to do is provide more information for logging/reporting, then we can do that without exporting any errors.

(If a handler does need to identify when it has returned, that's obviously trivial to add in the handler itself.)

Adding additional human-readable information about the cancellation reason to the context cause seems reasonable to me.

[sgielen]

I don't see a good reason to expose ErrConnectionClosed or ErrRequestHandlerReturned. I can't think of many, if any, situations when a handler would need to distinguish between these conditions. If all we want to do is provide more information for logging/reporting, then we can do that without exporting any errors.

I actually use the exposed ErrConnectionClosed to determine the log level of related log lines, and this was the reason for proposing the CL. In my case, gRPC method failure caused by context cancellation (anywhere in the code) is normally slog.LevelError but if the cancellation was caused by a net/http client request cancellation, then the system is working as intended, so it should be Warn or Info.

if errors.Is(cause, util.ErrConnectionClosed) {
    // Connection was closed by the client, so this Canceled error isn't an error
    level = slog.LevelWarn
}

If the ErrConnectionClosed is not exported I believe I would have to look at the .Error() string to make the distinction between context cancellation caused by net/http or by any other location in the code, isn't it?

@neild would you consider this enough justification to expose (one of) the two Errs, or would you rather suggest another solution? Thanks

[sgielen]

@neild there are two users agreeing with my comment above regarding the export of the Errs, could we use this as community input to my suggestion and move this proposal through for review?