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: context: add method like WithoutCancel but preserve context deadline #67135

Open
danztran opened this issue May 2, 2024 · 6 comments
Labels
Milestone

Comments

@danztran
Copy link

danztran commented May 2, 2024

Context

The current context.WithoutCancel function supported since Go 1.21 creates a new context that inherits all values from the parent except for cancellation and deadline exceeding. It loses the deadline information set on the parent. This becomes a limitation when we want to maintain a specific timeout even if the parent gets canceled.

Use case

Middleware and interceptor patterns often require preserving request deadlines. A middleware can use context.WithoutCancel to ensure its execution continues regardless of client cancellation. However, if the parent context had a deadline set, it's lost with the current implementation. This can lead to unintended behavior if the request processing takes longer than the intended timeout.

Workaround

To address this issue, I try create a function that produces a context with the same deadline as the parent, even if the parent is cancelled. Here's a workaround to the issue:

func WithoutCancelButDeadline(ctx context.Context) (context.Context, context.CancelFunc) {
  newCtx := context.WithoutCancel(ctx)
  deadline, ok := ctx.Deadline()
  if !ok {
    return context.WithCancel(newCtx)
  }
  return context.WithDeadline(newCtx, deadline)
}

This workaround has drawbacks:

  • It creates a new timer for the new context, instead of reusing the existing one from the parent.
  • It returns a cancellation function that's not actually used.

Proposal new API:

A more efficient approach would be to introduce a new API that directly returns the context without these redundancies. Here are some potential names for this new function (or an alternative with a clearer name):

func WithoutCancelButDeadline(parent context.Context) context.Context
func WithoutCancelPreserveDeadline(parent context.Context) context.Context
func WithDeadlineWithoutCancel(parent context.Context) context.Context
func WithDeadlineOnly(parent context.Context) context.Context
@gopherbot gopherbot added this to the Proposal milestone May 2, 2024
@danztran danztran changed the title proposal: context: WithoutParentCancel proposal: context: add method like WithoutCancel but preserve context deadline May 2, 2024
@ianlancetaylor
Copy link
Contributor

How about context.WithDeadline(context.WithoutCancel(parent), parent.Deadline()) ?

@danztran
Copy link
Author

danztran commented May 2, 2024

@ianlancetaylor

  1. context.WithDeadline(...) creates a totally new timer and cancelFunc, which is fine but both are kinda redundant in my usecase, not optimal though.
  2. parent.Deadline() returns zero time.Time and a isDeadlineSet boolean value. If deadline was not set from parent context, and boolean is not check, then the new context would be timed out immediately.
  3. There's a mistake in your suggestion that causes syntax error, parent.Deadline() returns 2 values (deadline time.Time, ok bool) for context.WithDeadline(parent context.Context, d time.Time)

@ianlancetaylor
Copy link
Contributor

The point is, I think there is already a way to do what you want. If we are going to do a more convenient version, it would help to see some evidence that this is something that various different programs want to do. It's OK if unusual things are a bit harder to do.

@danztran
Copy link
Author

danztran commented May 3, 2024

Context is definitely important throughout in almost everywhere in every projects, most created variables for sure, especially when dealing with for API services. I was just curious if there is any way to make using context even more efficient as possible, to use fewer resources.
But I'm completely agree with your point though, if this is just something specific to our needs, we can find another solution that works for us.

@neild
Copy link
Contributor

neild commented May 4, 2024

As a general point, I'd say that treating context deadlines and cancelation as separate things is always a mistake. Both bound the lifetime of an operation; continuing past the lifetime in some cases but not others likely indicates a design confusion.

Middleware which needs to perform some action after an operation's lifetime has ended probably needs to do so whether the operation ended due to cancelation or due to a timeout.

@danztran
Copy link
Author

danztran commented May 4, 2024

I couldn't argue with that too. In my perspective, all requests should have deadlines. Also in our usecase for some requests, we want to keep processing things even if the context gets cancelled (unexpectedly by client). That way, we wouldn't need to worry about creating new context without cancel but with timeout every time. Things like external service calls in request processing could potentially hang forever if the context is not cancelled and does not time out. If you guys think this is over engineering, please feel free to close.

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

No branches or pull requests

4 participants