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

Improve docs for BlockUntil and waiters #82

Open
jpittis opened this issue Feb 25, 2024 · 1 comment
Open

Improve docs for BlockUntil and waiters #82

jpittis opened this issue Feb 25, 2024 · 1 comment

Comments

@jpittis
Copy link

jpittis commented Feb 25, 2024

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:

  • Add a few sentences defining waiters with respect to the different operation on Timers and Tickers.
  • Add a warning/note that BlockUntil doesn't consider calls to Chan, and only reacts to construction/Stop/Reset.
  • Add example code documenting the use of BlockUntil for each operation (e.g., Sleep/Await, NewTimer, NewTicker, Stop, and Reset).
  • Discuss the implications of the BlockUntil behavior.
    • Timers are ideally constructed in the test's main thread to ensure potential future Advance calls don't race with construction (but that BlockUntil can be used to wait for the construction of Timers and Tickers).
    • BlockUntil is not sufficient to test Timers (e.g., unable to distinguish between before the Stop call and after the Reset call).
@DPJacques
Copy link
Collaborator

DPJacques commented Jun 3, 2024

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.

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

No branches or pull requests

2 participants