BlockUntil currently works for blockers like Timers, but not Tickers. Would it be possible to make Tickers a blocker?

Is there an option to make it so that the callbacks from timers are invoked synchronously instead of in a goroutine? Essentially, remove go from here:

My issue with the current method is demonstrated by this workaround https://github.com/temporalio/temporal/blob/15974517a5ea2987a7f9a02afefc423418ba658a/service/matching/liveness_test.go#L44

Because Advance is not synchronous, and we are trying to verify that the callback is not fired, there is no event for us to actually wait on in the test, so we have to resort to a time.Sleep.

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).

I wrote a simple function that executes a certain piece of code at a specified interval.

func ScheduledTask(c clockwork.Clock) {
	ticker := c.NewTicker(1 * time.Hour)
	go func() {
		defer ticker.Stop()

		for range ticker.Chan() {
			fmt.Printf("responsed\n")
		}
	}()
}

I referred to the Readme and wrote a unit test like this.

func TestScheduledTask(t *testing.T) {
	fakeClock := clockwork.NewFakeClock()

	var wg sync.WaitGroup
	wg.Add(1)

	go func() {
		ScheduledTask(fakeClock)
		wg.Done()
	}()
	fakeClock.BlockUntil(1)
	for i := 0; i < 24; i++ {
		fakeClock.Advance(1*time.Hour + 1*time.Second)
		//time.Sleep(1 * time.Millisecond)
	}
	wg.Wait()
}

The program cannot produce the expected output.

=== RUN   TestScheduledTask
--- PASS: TestScheduledTask (0.00s)
PASS

But when I set a delay of one millisecond for each loop, the output of the program is as follows

=== RUN   TestScheduledTask
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
responsed
--- PASS: TestScheduledTask (0.03s)
PASS

Is this a bug or a usage issue?

GitHub workflows run with write-all permissions. This makes repositories vulnerable to supply-chain attacks.

Given clockwork' workflows are for SAST and unit testing, they don't require such broad permissions. I'd therefore like to help the project close this vulnerability.

This can be done in two ways:

1. We can add top-level read-only permissions to all the workflows; and/or
2. You can change the repository settings to change the default permissions from write-all to read-only.

I'll send a PR along with this issue setting the top-level permissions on the workflows.

Or, if you'd rather (or also wish to) change the repository's default settings:

1. Open the repo settings
2. Go to Actions > General
3. Under "Workflow permissions", set them to "Read repository contents and packages permissions"

Disclosure: My name is Pedro and I work with Google and the Open Source Security Foundation (OpenSSF) to improve the supply-chain security of the open-source ecosystem.

Using NewFakeClock() to make a Clock at a specific point in time requires a custom implementation (since fakeClock is internal) or calling Advance while depending on the internal detail that the clock starts in 1900. Is this within the desired project scope?

I previously considered this package for some testing which depended on a specific timestamp for validations.

I found a large number of authors are writing tests that depend on the deterministic time returned by NewFakeClock. I found this out the hard way when I upgraded us to #52, which changed this default.

I believe using a deterministic time when none is specified is an anti-pattern. If a test requires a deterministic time, authors should use NewFackClockAt to make the timestamp requirement explicit. This is similar to how Go's map iterating order is non-deterministic.

I made #55 as a suggestion, which uses a random time from the year 1970 (excluding the Unix epoch time), but I am open to other thoughts/suggestions.

Hi,

	fc := clockwork.NewFakeClock()

	ft := fc.NewTicker(1)
	fc.Advance(3)

	select {
	case x := <-ft.Chan():
		fmt.Println("1-------------", x)
	default:
	}

I am expecting have 3 print in above code, but it only print once, looks like the ticker is only triggered once, how can I trigger multiple times?

I know that the real clock doesn't guarantee that timers will go off exactly on time, but in practice they go off somewhat close to on schedule.

I just created a pull request to add AfterFunc. I noticed when testing it that I can advance the clock well past when a timer is supposed to fire. The timer fires, but it thinks the time is well past when it should be.

So then, the question is: should the clock pause for real time as it advances past timers? I think that for AfterFunc, it should be willing to wait for f() to return or for one real second to pass, whichever comes first.

I don't think any of the other APIs provide any way to know how long to pause.

According to the golang documentation for time.Sleep, when passed a zero or negative duration, the function returns immediately.

FakeClock correctly handles a zero duration, but ends up blocking for negative durations, thus requiring the clock to be advanced by zero to unblock the sleeper (which should never have been blocked to begin with).

to allow a code like this

t := s.clock.NewTicker(s.config.InitialDelay)
var resetOnce sync.Once
defer t.Stop()
for {
	select {
	case <-s.newSlotCh:
		t.Reset(s.config.InitialDelay)
	case <-t.Chan()():
		resetOnce.Do(func() { t.Reset(s.config.PeriodicDelay) })

Hello!

First - thank you for the great package! It has been very helpful.

My coworkers and I wrapped the package and added some context helper methods. Was curious if you would be interested in a PR that would add these helpers to the package.

Can check out the code: https://gist.github.com/connyay/917cb5c6e5d43d08df8bb6a076e5dab9

I can open a PR next week if there is interest.

for this - change clockwork.Clock type from interface to struct or pointer with call system time functions by default (for zero value).

It allow make my own types usable with zero value without wrap time calls in my code.

Hi, thanks for the nice package.

I was trying to fix some flaky tests by converting them to use this. However, the tests use Context.WithTimeout and it looks like the fake clock, even if injected into a context, is not respected by that code?

I can imagine how that might be impossible to fix without core library changes but it might be nice to document it to save people time?

https://tip.golang.org/doc/go1.23#timer-changes

Fast submission just to get this on record:

Test clockwork/clockwork_test: FAILED in 114 out of 1000 in 13.3s at commit 79f9aa2:

TestFakeTicker_DeliveryOrder: fails in 113 out of 1000 runs

third_party/golang/clockwork/ticker_test.go:119: Expected ticker event didn't arrive!

TestFakeTicker_DeliveryOrder: fails 1 out of 1000 runs

third_party/golang/clockwork/ticker_test.go:45: expected tick!
    third_party/golang/clockwork/ticker_test.go:55: wrong tick time, got: 0001-01-01 00:00:00.000000001 +0000 UTC, want: 0001-01-01 00:00:00.000000003 +0000 UTC

These issues are not present in v0.3.0, commit 6a247c3 :

10000 runs: clockwork:clockwork_test PASSED in 18.4s

Hey, it's Pedro and I'm back (see #75) with another security suggestion.

A security policy (SECURITY.md) explains how the project wishes to receive and handle responsible disclosure of potential vulnerabilities. GitHub recommends that projects have one.

The security policy can be found by users who enter the project's "Security" panel. They'll also see references to it in the "New issue" page.

There are two main ways to receive disclosures:

• register an email or website available to receive such reports; and/or
• use GitHub's private vulnerability reporting

If you want to use GitHub's reporting system, it must be activated for the repository:

1. Open the repo's settings
2. Click on Code security & analysis
3. Click "Enable" for "Private vulnerability reporting"

I'll send a PR with a draft policy along with this issue.

As per #82, BlockUntil blocks on the number of active waiters, and for Timers/Tickers this means construction. Stop removes the waiter, and Reset adds it again. In practice I've found BlockUntil is insufficient to test code using Timers and Tickers because it can't distinguish between before a call to Stop, and after a call to Reset.

I'm curious if maintainers would be interested in the proposed addition. I've been using this in my test code implemented as a wrapper around clockwork, but supporting it in the library itself would be preferable. Happy to contribute the change.

Real-ish World Example

Imagine you want to write a test for the following state machine:

// Sends an incrementing counter on the counters channel every 3 seconds.
// If a heartbeat isn't received within 5 seconds, exits with an error.
func runStateMachine(
	ctx context.Context,
	clock clockwork.Clock,
	counters chan int,
	heartbeats chan struct{},
) error {

	counterTimer := clock.NewTimer(3 * time.Second)
	defer counterTimer.Stop()

	heartbeatTimeout := clock.NewTimer(5 * time.Second)
	defer heartbeatTimeout.Stop()

	counter := 1

	for {
		select {
		case <-counterTimer.Chan():
			counterTimer = clock.NewTimer(3 * time.Second)
			counters <- counter
			counter++

		case <-heartbeats:
			heartbeatTimeout.Stop()
			heartbeatTimeout = clock.NewTimer(5 * time.Second)

		case <-heartbeatTimeout.Chan():
			return fmt.Errorf("Timeout at %d", clock.Now().Unix())

		case <-ctx.Done():
			return ctx.Err()
		}
	}
}

You'll end up running into a situation where there's no primitive in the clockwork library to ensure that Advance doesn't race with the resetting of a timer:

func TestRunStateMachine_SendingHeartbeatResetsTimeout(t *testing.T) {
	ctx, cancel := context.WithCancel(context.Background())
	defer cancel()
	fakeClock := clockwork.NewFakeClockAt(time.Unix(0, 0))
	counters := make(chan int)
	heartbeats := make(chan struct{})

	done := make(chan error, 1)
	go func() {
		err := runStateMachine(ctx, fakeClock, counters, heartbeats)
		done <- err
	}()

	// Ensure state machine timers are initialized at time zero before advancing.
	fakeClock.BlockUntil(2)
	fakeClock.Advance(3 * time.Second)
	counter := <-counters
	require.Equal(t, 1, counter)

	// Sending a heartbeat gives us another 5 seconds, allowing for another counter to be
	// produced at 6 seconds in.
	heartbeats <- struct{}{}
	log.Println("Trigger race by removing this print statement (at least on my machine)")

	// PROBLEM: What do we block on here to ensure that the heartbeat timer is reset at time 3
	// rather than time 6? We can't use BlockUntil(2) because the condition is already true
	// before heartbeatTimeout.Stop() is called.

	fakeClock.Advance(3 * time.Second)
	counter = <-counters
	require.Equal(t, 2, counter)

	fakeClock.Advance(2 * time.Second)
	err := <-done
	require.Error(t, err)
	require.Contains(t, err.Error(), "Timeout at 8")
}

Possible Solution: Adjust System Under Test

Sometimes it's possible to write your system under test such that this race is avoided. In the above example, we reset the counterTimer before sending on the counters channel, ensuring that the reset always happens before the reset. However, in-practice I've found situations where it's just not reasonable to contort your system under test to solve for this race condition. In the above example, the heartbeatTimeout reset is similar to a real world situation I ran into, and ultimately solved with the solution proposed below.

Proposed Solution: Add BlockUntilMonotonic or BlockUntilSeenSoFar

What I've found works quite well, is a BlockUntilMonotonic method on FakeClock which blocks on the monotonically increasing number of watchers the clock has ever seen.

In the example above, the usage would look like this:

heartbeats <- struct{}{}
fakeClock.BlockUntilMonotonic(4)
// Unblocks after the new Timer is created.

In-practice, I've found reasoning about the total number of watchers that have been created over the life of the test to be tedious, so I've created a helper method that avoids me having to count.

seen := fakeClock.CurrentMonotonic()
heartbeats <- struct{}{}
fakeClock.BlockUntilMonotonic(seen + 1)
// Unblocks after the new Timer is created.

Implementation

I suspect the implementation will be relatively simple: just keep track of the total number of waiters the FakeClock has ever seen, and use a similar blocker primitive to unblock when enough new waiters are seen.

Naming

I don't have strong feelings about the naming. So far I've liked BlockUntilSeenSoFar or BlockUntilMonotonic the most as far as naming goes.

Hi @jonboulle,

First of all, thanks for this package, I think it's a quite popular one!

I noticed there are a couple open issues and PRs and I wonder if you need help with maintenance. Either as a contributor to this repo, or if you don't want to maintain it anymore, as a managed fork somewhere (eg. https://github.com/gofrs).

Let me know what you think!

Consider the following code:

func TestFakeClock(t *testing.T) {
	c := clockwork.NewFakeClock()

	go func(){
		c.Sleep(time.Second)
	}()
	go func(){
		c.Sleep(time.Second)
	}()

	c.BlockUntil(1)
}

Most of the time this code will pass! But non-deterministically (if the goroutines are scheduled at just the right time), the BlockUntil will block forever.

The problem is that numExpected != numActual (2 != 1), so the BlockUntil call will block forever. I think it should be changed so that it blocks until numExpected <= numActual. Would you accept a PR to change this?

I could also imagine other fixes, like BlockUntil panic-ing if there are more sleepers than numExpected, rather than blocking forever.

Description

fakeClock does not handle negative durations passed to After() correctly.

This looks very similar to #19

Steps to reproduce

func TestNegativeTimer(t *testing.T) {
	fakeClock := clockwork.NewFakeClock()
	timer := time.NewTimer(2 * time.Second)
	defer timer.Stop()
	select {
	case <-fakeClock.After(-1 * time.Second):
	// Success
	case <-timer.C:
		t.Fatalf("Timed out!")
	}
}

Output:

$ go test -v -run TestNegativeTimer
=== RUN   TestNegativeTimer
--- FAIL: TestNegativeTimer (2.00s)
    negativetimer_test.go:18: Timed out!
FAIL
exit status 1

The godoc comments for NewFakeClockAt should refer to that function, rather than NewFakeClock.

Adding go.mod and go.sum is nice, but more important (and simpler) is tagging new version to include "recent" code changes. Without it go take version 0.10 which is quite old.

TL;DR

I discovered a rather nasty little race condition (technically several) in the fakeTicker. It's in the fakeTicker.tick() function and how it references the fake clock.

Steps to reproduce

1. Create fake clock
2. Create fake ticker from fake clock
3. Advance clock so a tick should fire
4. Attempt to pull off of the ticker channel - sometimes doesn't get a tick on the channel

Test to reproduce:

This fails pretty much every time I run this, but you can also throw this in a for loop to test it a bunch of times and you'll be pretty much guaranteed to reproduce it.

func TestFakeTicker_Race(t *testing.T) {
	fc := NewFakeClock()

	tickTime := 1 * time.Millisecond
	ticker := fc.NewTicker(tickTime)
	defer ticker.Stop()

	fc.Advance(tickTime)

	timeout := time.NewTimer(500 * time.Millisecond)
	defer timeout.Stop()

	select {
	case <-ticker.Chan():
		// Pass
	case <-timeout.C:
		t.Fatalf("Ticker didn't detect the clock advance!")
	}
}

Races

I've found two races so far. Both of them are basically the same, just in two different places.

Race 1

The very first thing the tick function does is retrieve the current time from the fake clock for use in the service loop. Unfortunately, when this is actually executed depends on when the ticker goroutine is run by the scheduler. In the test code above, this order of operations can happen:

1. Test thread - Create ticker
2. Test thread - Ticker spawns goroutine, but scheduler hasn't scheduled it yet
3. Test thread - Advances the clock past the tick point
4. Ticker thread - Grabs the current time from the fake clock which has already been advanced. The ticker then does not fire a tick as the clock has not advanced past when it should

This race can be solved by passing in the "now" timestamp to the function when the goroutine starts. A simple two line fix to change fakeTicker.tick to take in tick time.Time and update the fakeClock.NewTicker to call go ft.tick(fc.Now()).

Race 2

During the service loop in fakeTicker.tick the remaining time is calculated based off of a call to ft.clock.Now(). Later on in the loop, ft.clock.After(remaining) is called. Unfortunately, the clock can be advanced between those two calls resulting in a lost tick:

1. Test thread - Create ticker (let's say the duration is 5ms)
2. Test thread - Ticker spawns goroutine
3. Ticker thread - Executes until remaining := tick.Sub(ft.clock.Now()) is called (remaining = 5ms because the clock hasn't advanced yet)
4. Test thread - Advances clock 5ms
5. Ticker thread - remaining is greater than 0, so it doesn't fire a tick. It then falls through to the second select and ultimately calls ft.clock.After(5ms) even though the clock has already advanced to 5ms.

With the current implementation of FakeClock updates are lost if more than one is generated at a time.

If, for example, one has a ticker that fires every five minutes and advances the FakeClock by one hour they will get one tick of five minutes.

This is due to the implementation relying on a lossy channel that is 1-buffered.

My proposal here would be to change this implementation to schedule all needed updates with an unbounded queue.

Simulating time in tests should behave as time should in the actual lifecycle of a program and FakeClock currently does not (during real execution the code has time to consume the messages, they don't get sent all at once)

One other possibility would be to allow users to specify a buffer size for tickers when the fake clock is constructed (it's reasonable to assume who is writing the test might know an approximation of the amount of ticks they expect).

Note: In case you are interested in such a change I can send a PR for it.

Would be great to see clockwork implement more features of the time library.

I noticed that there appears to be a race condition in the fake timer implementation. If Reset gets called multiple times then apparently it can schedule multiple channel events. I will upload a commit demonstrating the issue.