jonboulle / clockwork

a fake clock for golang
Apache License 2.0
656 stars 58 forks source link

Improve docs for BlockUntil and waiters #82

Open jpittis opened 7 months ago

jpittis commented 7 months ago

The docs don't sufficiently explain the behavior of BlockUntil, at least when it comes to using Timers and Tickers.

I've found the behavior of BlockUntil for After and Sleep to be intuitive due to their blocking nature.

I'd be pleased to take a stab at contributing these improvements, but I wanted to check-in first to get a read on the attitude of the maintainers towards both the behavior of BlockUntil, and general documentation philosophy.

Existing Documentation

Here are the existing references to BlockUntil in the docs:

// BlockUntil blocks until the FakeClock has the given number of waiters.

Users can call BlockUntil to block until the clock has an expected number of waiters.

Here are the existing references to waiters in the docs:

// Advance advances the FakeClock to a new point in time, ensuring any existing // waiters are notified appropriately before returning.

FakeClock maintains a list of "waiters," which consists of all callers waiting on the underlying clock (i.e. Tickers and Timers including callers of Sleep or After).

Example of Undocumented Behavior

When I was recently using BlockUntil with Timer, I discovered that creating a NewTimer immediately created a waiter, rather than say, the call to the Chan. When I went to the docs to understand more, it was hardly discussed.

Here's some example code that I initially found unexpected, which I'd like the documentation to explain:

fakeClock := clockwork.NewFakeClock()
timer := fakeClock.NewTimer(5 * time.Second)
fakeClock.BlockUntil(1) // does not block because NewTimer created a waiter

Similarly, explaining how Stop deregisters a waiter, and Reset reregisters it again.

Again, for Tickers I haven't tested it, but my understanding is that a Ticker behave similarly wrt to Stop and Reset.

Suggested Improvements

Doing all of the following may be overkill, but here are some ideas:

DPJacques commented 4 months ago

Despite being a maintainer, I don't particularly love BlockUntilContext, because in concurrent code it can be racey. I have not found a great way to use it in my own complex code.

That said, "Add a warning/note that BlockUntil doesn't consider calls to Chan, and only reacts to construction/Stop/Reset." is intentional per the comment on FakeClock. Chan() can't be considered a waiter, consider: If a single Timer has 2 callers of Chan() is only 1 of the 2 channels will receive the time on expiration, therefore there is only 1 waiter regardless of the number of callers of Chan().

I'm supportive of updating/improving the documentation around BlockUnitl, Stop(), and Reset(), but I don't think we need to warn that Chan() is not waiter, because it cannot be.