Introduce context aware helpers for context related testing

[3vilhamster]

Hey folks.

I started using the library and found a missing piece that is very helpful for my case. We use both time related functions and context.Timeout, but we want to be able to orchestrate all operations with a single time source.

I've introduced the logic that was already done by benbjohnson library https://github.com/benbjohnson/clock/blob/master/context.go

While testing context related things, I've also bumped into a problem with AfterFunc, which did not provide a guarantee of call on expiration. I've borrowed a "fix" with a time.Sleep from benbjohnson library. Without it, it is impossible to guarantee the call of the afterfunc.

I've ran tests for a few thousands times, but haven't see any issues.

Let me know if you have any concerns, or maybe you can help with a better way to fix this race.

[DPJacques]

I like the idea. However, my blocking concern is that they overload the clock interface with methods that don't have analogs in https://pkg.go.dev/time, rather they are functions from https://pkg.go.dev/context

I'd support adding these to an unexported struct (maybe clockContext?) to context.go that provides the context.Context interface and is created with a function call clockwork.Context(parent context.Context, c *clockwork.FakeClock) context.Context.

The fast/easy way to do this would be with context.WithoutCancel, but that negates calls to the parent's cancel function.

You might consider trying to implement the unexported struct to intercept context.DeadlineExceeded, thinking that is more robust than the above. However, this would introduce an even more nuanced, unexpected gotcha: that once the parent's deadline passes, which you have overridden, the parent's cancel function no longer works. That would be sure to surprise a caller somewhere, and be a real pain to debug since we are masking the deadline signal.

So.. I think we should offer our own cancel function, since we'll be negating the parent context's, i.e the function signature should be func Context(parent context.Context, c *FakeClock) (context.Context, context.CancelFunc).

[3vilhamster]

hmm. Could you elaborate? The biggest issue between time and context packages is that context package always use realtime, which defeats any controlled testing environment effort: https://cs.opensource.google/go/go/+/refs/tags/go1.23.0:src/context/context.go;l=689 So, implementing context.Context is not enough, we need to have a specific method, which will use real time as context.WithTimeout/context.WithDeadline, and FakeClock version in testing scenarios. It could be:

1. My proposed version

clock.WithTimeout(parent context.Context, timeout time.Duration) (context.Context, context.CancelFunc)
clock.WithDeadline(parent context.Context, deadline time.Time) (context.Context, context.CancelFunc)

2. Separated function

clockwork.WithTimeout(parent context.Context, clock Clock, timeout time.Duration) (context.Context, context.CancelFunc)
clockworkf.WithDeadline(parent context.Context, clock Clock, deadline time.Time) (context.Context, context.CancelFunc)

Both look the same to me, so I can switch from methods to functions.

[DPJacques]

So, implementing context.Context is not enough

I think I may have miscommunicated. Do you agree that something needs to implement context.Context? If your response no I think we have some narrowing to do.

My stance is:

1. Yes we need something to implement context.Context
2. That thing should not be *FakeClock.

My proposal is to make a struct like:

package clockwork

type context struct { // Note this is not exported, because it is always returned as a context.Context.

  // Implements context.Context

  clock *FakeClock // Used to provide context.Context
}

and we'll most certainly need a function to "construct" it, similar to your proposal #2.

---

I'm against your proposal #1 and support your proposal #2 in concept, with 2 clarifications:

1. That the returned context implementation is a struct that uses a *FakeClock
2. That *FakeClock itself is not responsible for how the returned context implementation manages its behavior. This is where I have issues with the currently proposed PR because it uses FakeClock to implement context.Context, violating the single responsibility principle.

[3vilhamster]

Ok.

I'm still not 100% sure if I understood you correctly, but I've updated the PR with the #2 approach.

The most considerable flow of this design changes the behavior from context.WithTimeout and context.WithDeadline from the code that does not use clockwork.

[DPJacques]

See comment in #92, conversation continued in #94.