Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
hummerd opened this issue Nov 30, 2023 · 7 comments · May be fixed by #70850
Open

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

hummerd opened this issue Nov 30, 2023 · 7 comments · May be fixed by #70850
Labels
Milestone

Comments

@hummerd
Copy link

hummerd commented Nov 30, 2023

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.

@gopherbot gopherbot added this to the Proposal milestone Nov 30, 2023
@mauri870
Copy link
Member

mauri870 commented Nov 30, 2023

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
Copy link
Author

hummerd commented Nov 30, 2023

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.

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Dec 2, 2023
@kszafran
Copy link

kszafran commented Jan 2, 2024

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 seankhliao changed the title proposal: net/http: Add context cancelation reason for server handlers proposal: net/http: add context cancelation reason for server handlers Jan 13, 2024
@seankhliao
Copy link
Member

cc @neild

@sgielen
Copy link

sgielen commented Dec 14, 2024

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.

sgielen added a commit to sgielen/go that referenced this issue Dec 15, 2024
When we cancel a HTTP server handler context, set an appropriate
cancel cause. This makes investigation of context cancellation
errors easier.

Fixes golang#64465
sgielen added a commit to sgielen/go that referenced this issue Dec 15, 2024
When we cancel a HTTP server handler context, set an appropriate
cancel cause. This makes investigation of context cancellation
errors easier.

Fixes golang#64465
sgielen added a commit to sgielen/go that referenced this issue Dec 15, 2024
When we cancel a HTTP server handler context, set an appropriate
cancel cause. This makes investigation of context cancellation
errors easier.

Fixes golang#64465
@gopherbot
Copy link
Contributor

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

@sgielen
Copy link

sgielen commented Dec 15, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

Successfully merging a pull request may close this issue.

6 participants