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

time: mockable time support #8869

Open
jmhodges opened this issue Oct 5, 2014 · 17 comments
Open

time: mockable time support #8869

jmhodges opened this issue Oct 5, 2014 · 17 comments
Milestone

Comments

@jmhodges
Copy link
Contributor

@jmhodges jmhodges commented Oct 5, 2014

With https://github.com/jmhodges/clock and the broader https://github.com/benbjohnson/clock API,
there's a desire for the timing systems in Go to be fakeable[1]. Using a fake clock
instead of time.Now directly is useful when testing code that stores timestamps, caches
data, etc.

The benbjohnson clock package attempts to make Ticker and Timer calls (as well as their
related AfterFunc and Sleep methods) work against a fake time that can be set and
updated in tests. However, it relies on some micro-sleeps and runtime.Gosched calls that
are obviously going to be flaky. But there is a desire to able to test code that uses
Tickers and Timers, not by adjusting the durations they work for (which can induce flaky
testing), but by adjusting when they think they need to wake up.

To do that, we seem to need more runtime magic to help developers out. In fact, it might
be best if a clock package like these lived in the stdlib so that it could be tied more
carefully and thoughtfully to the scheduler.

 [1] or "mockable", whatever language you prefer. The summary of this issue is to distinguish it from issue #5356.
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 5, 2014

Comment 1:

I'm not entirely sure this is a good idea.  I'm also not sure what it would look like. 
If the goal is simply to provide Ticker and Timer calls for testing, I think that could
be done entirely independently of the time package, along the lines of the old
playground code.  The testing package would keep its own queue of events, and step
forward to the next event without actually waiting.

Labels changed: added repo-main, release-none.

@dvyukov
Copy link
Member

@dvyukov dvyukov commented Oct 5, 2014

Comment 2:

I am not sure playground approach will work in large apps. In playground we advice time
when the process in completely out of other work. But what if we have a goroutine in a
syscall/cgo, should we advice time? Or wait for it to return from syscall/cgo first?
@sougou
Copy link
Contributor

@sougou sougou commented Oct 5, 2014

Comment 3:

The main challenge we faced was related to the original code importing the standard time
package, which is not mockable. This forces the tests to also use actual clock time.
We've mostly managed to work around these. So, it's not a big issue now. But it's still
a nice-to-have.
@sougou
Copy link
Contributor

@sougou sougou commented Oct 30, 2014

Comment 4:

What if the time package accelerated the clock (just for the process) by an order of
magnitude? We can consider restricting this ability to the testing package.
There are still some downsides: it won't work for integration tests, but it may satisfy
the needs of simple in-process tests.
@jmhodges jmhodges added new labels Oct 30, 2014
@bradfitz bradfitz removed the new label Dec 18, 2014
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc removed release-none labels Apr 10, 2015
@jmhodges
Copy link
Contributor Author

@jmhodges jmhodges commented Sep 2, 2015

Any chance of something happening for this?

@jmhodges
Copy link
Contributor Author

@jmhodges jmhodges commented Sep 2, 2015

(I'm okay with hearing no! It's just an open cycle in my head I'm trying to clear out.)

@adg
Copy link
Contributor

@adg adg commented Sep 2, 2015

It's unlikely to happen any time soon.

Dieterbe added a commit to grafana/metrictank that referenced this issue Oct 13, 2016
after lots of experimentation I figured out the mock clock sometimes
simply doesn't properly trigger, so that in Usage.Report() sometimes
nothing is received on the tick channel, despite advancing the fake
clock by more than strictly nessecary (i tried with an extra ms),
despite calling runtime.Goshed() ourselves, and despite sleeping
20 ms with the real clock.

The author of the clock package confirms that due to the way the
runtime schedules goroutines, there's no way around the fake clock
sometimes not working. See
https://gophers.slack.com/archives/general/p1462238960008162

Furthermore, in discussion with the golang developers at
golang/go#8869 it becomes clear that it's
unlikely that we'll have a fakeable clock anytime soon.

Ben Johnson (clock author) suggests in the above mentioned gophers
thread that we could mock out the tick function and pass in a different
function in tests.  However, that changes so much of the time logic
that it becomes pointless to do any time-based testing in this design.

We could also switch to simply test the basics, not time based.
Since the timing code is pretty easy.

However before we go that route, I wanted to try working with the real
clock.  Basically run the usage reporting in real time, but scaled down
to millisecond level instead of second level, to make it finish fairly
quickly.

So now some semantics are changing a bit:
* we allow up to <period> ms for the usage report to be in the state we
need it
* so we now works with steps, which don't happen at exact predictable
  timestamps, rather they have to happen within a timeframe
* checking timestamp would have gotten more complicated, so I just
removed it.  It's easy to reason that if the updates come within the
alotted times, then the timestamps should also be set correctly.
* there's no serious need to explicitly pass around interval settings
  anymore, we just use 1 everywhere.

If it turns out that this approach also triggers false positives
(for example due to circleCI machines being maxed out of CPU and the
reporting unable to happen within the needed time) then we can address
as needed and still switch to the simpler approach.
But that seems very unlikely.  This should work.
Dieterbe added a commit to grafana/metrictank that referenced this issue Oct 13, 2016
after lots of experimentation I figured out the mock clock sometimes
simply doesn't properly trigger, so that in Usage.Report() sometimes
nothing is received on the tick channel, despite advancing the fake
clock by more than strictly nessecary (i tried with an extra ms),
despite calling runtime.Goshed() ourselves, and despite sleeping
20 ms with the real clock.

The author of the clock package confirms that due to the way the
runtime schedules goroutines, there's no way around the fake clock
sometimes not working. See
https://gophers.slack.com/archives/general/p1462238960008162

Furthermore, in discussion with the golang developers at
golang/go#8869 it becomes clear that it's
unlikely that we'll have a fakeable clock anytime soon.

Ben Johnson (clock author) suggests in the above mentioned gophers
thread that we could mock out the tick function and pass in a different
function in tests.  However, that changes so much of the time logic
that it becomes pointless to do any time-based testing in this design.

We could also switch to simply test the basics, not time based.
Since the timing code is pretty easy.

However before we go that route, I wanted to try working with the real
clock.  Basically run the usage reporting in real time, but scaled down
to millisecond level instead of second level, to make it finish fairly
quickly.

So now some semantics are changing a bit:
* we allow up to <period> ms for the usage report to be in the state we
need it
* so we now works with steps, which don't happen at exact predictable
  timestamps, rather they have to happen within a timeframe
* checking timestamp would have gotten more complicated, so I just
removed it.  It's easy to reason that if the updates come within the
alotted times, then the timestamps should also be set correctly.
* there's no serious need to explicitly pass around interval settings
  anymore, we just use 1 everywhere.

If it turns out that this approach also triggers false positives
(for example due to circleCI machines being maxed out of CPU and the
reporting unable to happen within the needed time) then we can address
as needed and still switch to the simpler approach.
But that seems very unlikely.  This should work.
@flimzy
Copy link

@flimzy flimzy commented Jun 8, 2019

One possible step toward this, for the common time.Now() case, would be to add two exported symbols to the time package:

type Clock interface {
    Now() time.Time
}

var DefaultClock = /* an (unexported?) default implementation of Clock */

Then the existing time.Now() function (and possibly others) would become a de facto alias for time.DefaultClock.Now().

Whether the ticker/timer implementations could take advantage of such a Clock interface, or if it would need to be expanded, I don't know.

The primary motivation here, of course, is to provide a standard place to mock out a clock, while honoring the Go compatibility promise. It would, obviously, require altering any existing code that needs mocks, but that's already happening, so this would just provide a defined, supported way to do such mocks.

One other (possible) implication of such a Clock interface, is that it would be possible to alter the default timezone from the system default, by configuring a custom Clock instance. There may be other non-mocking uses as well, which I haven't considered yet.

@tv42
Copy link

@tv42 tv42 commented Jun 8, 2019

Having a caller-replaceable time.DefaultClock sounds both racy and like a step in the wrong direction. If this is about testability, we should live in a world where tests are parallelizable and don't muck with globals. (I'm ok with eating the costs of needing to write code to be testable; that's going to be true anyway, regardless of this yet another reason to write "hermetic" libraries.)

My reading of this issue is to provide better means for such a mock-clock library to simulate the effects of time passing "until next interesting event" in the scheduler.

@flimzy
Copy link

@flimzy flimzy commented Jun 10, 2019

Having a caller-replaceable time.DefaultClock sounds both racy

Perhaps so, but it's a pattern commonly used elsewhere in the Go standard library.

If this is about testability, we should live in a world where tests are parallelizable and don't muck with globals.

Code which relies on this functionality for testing shouldn't mess with globals (the same holds for the other places in the stdlib where this is done), and should instead use dependency injection.

The goal isn't to make it possible to modify time.DefaultClock in tests, but rather to provide a standard way to inject a clock into code that needs to be tested, while allowing legacy code, or code which doesn't need to be tested, to rely on the time.DefaultClock implementation.

I think of this as the same as http.DefaultClient. While it's possible to override http.DefaultClient in tests, I've never seen anyone advocate this. I wouldn't advocate this for time.DefaultClock, either.

@flimzy
Copy link

@flimzy flimzy commented Jun 10, 2019

Note that the suggestion of var DefaultClock could easily be removed, and my proposal would still satisfy my purpose, without adding the "scary" race opportunities. It just wouldn't be as transparent (I suppose) that the package level Now() is the equivalent of using a default implementation of the Clock interface.

@tv42
Copy link

@tv42 tv42 commented Jun 10, 2019

Perhaps so, but it's a pattern commonly used elsewhere in the Go standard library.

If you dig around the issues enough, you will find core devs sometimes expressing regret about those decisions. Just because it was done in the early days doesn't mean it should be the standard going forward.

@flimzy
Copy link

@flimzy flimzy commented Jun 10, 2019

If you dig around the issues enough, you will find core devs sometimes expressing regret about those decisions.

That's fair. I've often wondered about the wisdom of that myself.

But I still don't think that renders my (entire) suggestion untenable. I'd welcome your feedback on the remainder of my proposal. Specifically on the Clock interface idea.

For the sake of discussion, the DefaultClock can be turned into a function which returns a default implementation. This eliminates the user-replaceable, racy aspects.

I feel like this conversation got derailed by this (IMO, minor) aspect of my proposal. I'd rather address the core of it.

@tv42
Copy link

@tv42 tv42 commented Jun 10, 2019

I think the two existing & used libraries mentioned in the first comment are plenty to inform the API design; as far as I understand the challenge here is purely the integration with runtime. See mentions of "flaky".

@jmhodges
Copy link
Contributor Author

@jmhodges jmhodges commented Nov 15, 2019

Yeah, my first paragraph in this ticket was really to set up the latter two that describe why we want and what needs stdlib or runtime support to write better time-dependent tests. Clock interfaces are easy to implement outside of the stdlib and runtime (there's been a proliferation of these beyond just the packages listed now) but the real trick is giving people the ability to test code that involves Tickers and Timers.

There are likely lots of levels of ideas to address that Ticker and Timer problem.

One is to give Tickers and Timers new constructor funcs that take some kind of expanded Clock interface that can inform the Tickers and Timers when to run (instead of having them burn a bunch of CPU checking it in a loop). This would be similar to the addition of Context functions to database/sql.

@jmhodges
Copy link
Contributor Author

@jmhodges jmhodges commented Nov 15, 2019

This ticket pre-dates the Go proposal mechanisms and we might be getting to the time for a formal proposal to resolve this one.

It would be cool to hear ideas different from the rough sketch I gave above. I'm fairly sure there is one.

@maruel
Copy link
Contributor

@maruel maruel commented Apr 4, 2020

When this issue was filed, context.Context was not in the standard library; Context was added in Go 1.7 in 2016.

I like the idea of attaching mocks to the context. It may be considered a non-starter for some people as an abuse of context.

The following assumes this is reasonable:

Here's a precedent based on attaching a Clock interface on a context.Context. This is not racy and permits very well scoped tests:

main.go

package main

import (
        "context"
        "fmt"

        "go.chromium.org/luci/common/clock"
)

func getFormattedTime(ctx context.Context) string {
        clk := clock.Get(ctx)
        return clk.Now().String()
}

func main() {
        fmt.Println(getFormattedTime(context.Background()))
}

main_test.go

package main

import (
        "context"
        "fmt"
        "time"

        "go.chromium.org/luci/common/clock/testclock"
)

func ExampleGetFormattedTime() {
        fixed := time.Date(2006, 1, 2, 15, 4, 5, 6, time.UTC)
        ctx, tm := testclock.UseTime(context.Background(), fixed)
        fmt.Println(getFormattedTime(ctx))

        later := fixed.Add(time.Hour)
        tm.Set(later)
        fmt.Println(getFormattedTime(ctx))

        // Output:
        // 2006-01-02 15:04:05.000000006 +0000 UTC
        // 2006-01-02 16:04:05.000000006 +0000 UTC
}

IMHO this is quite clean. I'm not sure the Clock interface here should be used as-is. I'm not sure it's a good idea to pass a context in to Sleep and NewTimer. Still, I think something inspired by this could be added to the standard library without any regression.

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

Successfully merging a pull request may close this issue.

None yet
10 participants
You can’t perform that action at this time.