Allow the creation of a context.Context that uses a clockwork.Clock

[DPJacques]

This issue was prompted by discussions in PRs #86 & #92. The high level ask is to provide methods to construct a context.Context that uses a clockwork.Clock for deadline determination.

This should include the creation of functions analogous to context.WithDeadline and context.WithTimeout.

Open questions

1. What is the behavior of child Contexts using FakeClock when their parent is cancelled with context.DeadlineExceeded? The options I am currently aware of are:
   1. The child context also canceles with context.DeadlineExceeded, possibly due to a deadline from a real clock used by the parent context.
   2. The child context ignores the parent cancellation from that point forward, possibly missing other cancellation signals from the parent (i.e. manual calls the the parent's context.CancelFunc).
2. What should the interface be? This is somewhat related to the next question.
3. How do we handle context.WithDeadline? Specifically, the following code block has a race condition with concurrent users of clock.

   clock := clockwork.NewFakeClock()
   deadline := someTime // deadline > clock.Now()
   
   // This line races because both Until and After acquire and release the FakeClock's mutex.
   <-clock.After(clock.Until(deadline))
   

   The WithTimeout variant is easy, because time.After is mirrored with clockwork.Clock.After, and thus the resulting channel and backing goroutine can be generated with a single a mutex lock. However there is no equivalent At(t Time) <- chan Time in the time library, and thus we can't add that to the clockwork.Clock interface.

Initial responses to questions / requests for information

1. I am recusing myself on this question because I find both these options sufficiently distasteful that I don't personally intend to use this feature. @3vilhamster, since you were the initial requester, what behavior would you prefer, and ideally, why is it better than the alternative?

2. In Add contexts that use FakeClock rather than the system time. #92, I suggested the interface WithDeadline(parent context.Context, clock *FakeClock, t time.Time) (context.Context, context.CancelFunc) and its timeout analog. I used a FakeClock due to the issue mentioned in Question 3 below. The response was twofold:

   1. A rather strong distaste for referring to *FakeClock vs a Clock interface, which I agree with unless there is no alternative.
   2. The quote "This does not look like a nice API to me."

   Question: With regard to point 2 above, why is this API "not nice"? Was that just because it required the reference to FakeClock? If that were replaced with Clock, would that resolve the issue?

3. I think this will have to be resolved by adding FakeClock.at(t Time) <- chan Time (note that it is not exported) so we can build a timer channel inside a single mutex lock and avoid the race condition. We can then discuss typecasting (yuck) vs exposing FakeClock in the function directly. As I type this, I realize that perhaps typecasting is fine if we just fallback to the standard libraries if the type is not a FakeClock.

[jonathangjertsen]

I'm not sure I understand the race condition in <-clock.After(clock.Until(deadline)) . What are the different behaviors that occur depending on who gets the lock first?

[jonathangjertsen]

Question: With regard to point 2 above, why is this API "not nice"? Was that just because it required the reference to FakeClock? If that were replaced with Clock, would that resolve the issue?

That, and having clockwork.WithTimeout(ctx, clock, timeout) be equivalent to context.WithTimeout(ctx, timeout) when the clock is the real clock, would resolve it for me.

[DPJacques]

I'm not sure I understand the race condition in <-clock.After(clock.Until(deadline)) . What are the different behaviors that occur depending on who gets the lock first?

clock.Until will do a calculation within the mutex lock, and then release the lock

Most of the time, the immediately subsequent call to clock.After is the next thing that acquires the lock, but not always. In some users' test cases, concurrent calls to other methods of the same underlying FakeClock (notably, FakeClock.Advance) can and do race against clock.After, resulting in test flakiness.

You need a way to ensure the lock is acquired and released only once across the combination of clock.Until and clock.After, which you can't do with the exposed receiver functions on clockwork.Clock, it must be a method on FakeClock (unless the stdlib adds func At(t Time) <- chan Time and we can add an analogous function to clockwork.Clock)

[DPJacques]

I think we can overcome questions 2 and 3 if we are good with the following API calls:

• WithDeadline(parent context.Context, clock Clock, t time.Time) (context.Context, context.CancelFunc)
• WithTimeout(parent context.Context, clock Clock, d time.Duration) (context.Context, context.CancelFunc)

Assuming these are acceptable, the only open question is question 1.

[jonathangjertsen]

Regarding type casting, remember that you can also cast to an interface, so you don't necessarily need to check whether the clock is a FakeClock or not - you can just detect the presence of certain methods. errors in stdlib makes heavy use of this feature. Looks pretty janky, but the usage code remains clean and you can always add new types of Clock that opt in to the specialized behaviour https://github.com/golang/go/blob/master/src/errors/wrap.go#L17

So you could have a free function like this (untested)

func At(clock Clock, t time.Time) <- chan time.Time {
	// Call dedicated thread-safe alternative if available
	if c, ok := clock.(interface{ At(t time.Time) <- chan time.Time }); ok {
		return c.At(t)
	}

	// Fall back to näive implementation
	return clock.After(clock.Until(t))
}

[DPJacques]

Oh man, I admit I totally forgot about that. I think I wrote it off because, as you mention, it looks/feels janky. Great call though.

I wanted to touch base on Open Question 1. I am still looking for someone to provide feedback on the lesser of the 2 evils there. If I don't hear strong opinions otherwise I'm going to execute on Option 1, that the child context cancels if/when the parent cancels, just because that is easier to implement. Maybe I can add a custom error to determine if the cancellation was from a fake clock or not. Perhaps that is enough. Actually... as I type that I think that is the cleanest/easiest way forward.

I plan to revisit this over the weekend and ideally get a PR out. Thanks for the feedback @jonathangjertsen

[jonathangjertsen]

Agree with option 1. I expect a child context to cancel when its parent cancels for any reason, and would be surprised if that didn't happen. If the cancellation is caused by a fake clock, all is well. If it's caused by a real clock, that means you're mixing real and fake time in the same program which is not really meaningful and shouldn't necessarily be supported IMO.

[DPJacques]

Please see #92, which I have reopened and implemented the agreements we came to above. Of note:

1. I ended up using a single type assertion. It seemed like it was the most straightforward option.
2. I added a custom error that can be used to validate if the context deadline was hit by a fake clock or a real clock.

I'll leave this open for comment for a week or 2 and then merge it & then address #93. Sorry for the delay. Real life and work life have robbed me of spare headspace/cycles the last few weeks.