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]

Please delete this. We can't introduce arbitrary sleeps in test code.

[3vilhamster]

Unfortunately, without this code, AfterFunc does not guarantee a function call.
I can try to find a workaround, but I am unsure if I can succeed. Calling runtime.GoSched() is not enough.

[DPJacques]

Hmm, that is a little bit of a code smell. Working with clocks and goroutines make it easy to introduce race conditions, and I want to avoid that. Let me take a look at the problem we are trying to solve again.

PS - Sorry this review took so long. :(

[DPJacques]

I wanted to share my initial thoughts in #92. Obviously the With* functions are not finalized and I just realized afterTime isn't necessary. Regardless, I think my major takeaways are:

1. We need to provide a way for the user that created the context to cancel the context they receive, even if we aren't respecting parent cancellation.
2. I think we can do this with only a goroutine and 2 select statements(?)

I haven't started testing yet, so maybe I haven't hit the same issue as you that seems to require the sleep?

LMK if anything stands out to you. Going to continue working through it over the weekend.

[DPJacques]

Please take a look at #92. As far as I am aware, it has no race conditions and although we do have to sleep at 1 point in 2 different test cases, those cases are run in parallel and that sleep is not in the library's code path.

I'll let that PR hang for comments for ~2 weeks. LMK if that gives you everything you need or if you have any concerns.

[3vilhamster]

I added a comment on usage that is addressed by my PR but is not addressed in #92.

Let me know what you think.

Meanwhile, I'll try to work around the time.Sleep issue.

[DPJacques]

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