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.

Sign up for GitHub

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

Jump to bottom

Add contexts that use FakeClock rather than the system time. #92

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add contexts that use FakeClock rather than the system time. #92

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0841b94
Initial FakeContext-based context impliementation.
DPJacques Oct 19, 2024
4719e10
Last tweaks of implimentation before testing.
DPJacques Oct 19, 2024
86e2f7f
Add contexts that use FakeClock.
DPJacques Oct 19, 2024
42e854e
Fix typo and test case name.
DPJacques Oct 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 35 additions & 35 deletions clockwork.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,44 +157,53 @@ func (fc *FakeClock) NewTicker(d time.Duration) Ticker {
ft = &fakeTicker{
firer: newFirer(),
d: d,
reset: func(d time.Duration) { fc.set(ft, d) },
stop: func() { fc.stop(ft) },
reset: func(d time.Duration) {
fc.l.Lock()
defer fc.l.Unlock()
fc.setExpirer(ft, d)
},
stop: func() { fc.stop(ft) },
}
fc.set(ft, d)
fc.l.Lock()
defer fc.l.Unlock()
fc.setExpirer(ft, d)
return ft
}

// NewTimer returns a Timer that will fire only after calls to
// fakeClock.Advance() have moved the clock past the given duration.
func (fc *FakeClock) NewTimer(d time.Duration) Timer {
return fc.newTimer(d, nil)
t, _ := fc.newTimer(d, nil)
return t
}

// AfterFunc mimics [time.AfterFunc]; it returns a Timer that will invoke the
// given function only after calls to fakeClock.Advance() have moved the clock
// past the given duration.
func (fc *FakeClock) AfterFunc(d time.Duration, f func()) Timer {
return fc.newTimer(d, f)
t, _ := fc.newTimer(d, f)
return t
}

// newTimer returns a new timer, using an optional afterFunc.
func (fc *FakeClock) newTimer(d time.Duration, afterfunc func()) *fakeTimer {
var ft *fakeTimer
ft = &fakeTimer{
firer: newFirer(),
reset: func(d time.Duration) bool {
fc.l.Lock()
defer fc.l.Unlock()
// fc.l must be held across the calls to stopExpirer & setExpirer.
stopped := fc.stopExpirer(ft)
fc.setExpirer(ft, d)
return stopped
},
stop: func() bool { return fc.stop(ft) },
// newTimer returns a new timer using an optional afterFunc and the time that
// timer expires.
func (fc *FakeClock) newTimer(d time.Duration, afterfunc func()) (*fakeTimer, time.Time) {
ft := newFakeTimer(fc, afterfunc)
fc.l.Lock()
defer fc.l.Unlock()
fc.setExpirer(ft, d)
return ft, ft.expiry()
}

afterFunc: afterfunc,
}
fc.set(ft, d)
// newTimerAtTime is like newTimer, but uses a time instead of a duration.
//
// It is used to ensure FakeClock's lock is held constant through calling
// fc.After(t.Sub(fc.Now())). It should not be exposed externally.
func (fc *FakeClock) newTimerAtTime(t time.Time, afterfunc func()) *fakeTimer {
ft := newFakeTimer(fc, afterfunc)
fc.l.Lock()
defer fc.l.Unlock()
fc.setExpirer(ft, t.Sub(fc.time))
return ft
}

Expand Down Expand Up @@ -289,13 +298,6 @@ func (fc *FakeClock) stopExpirer(e expirer) bool {
return true
}

// set sets an expirer to expire at a future point in time.
func (fc *FakeClock) set(e expirer, d time.Duration) {
fc.l.Lock()
defer fc.l.Unlock()
fc.setExpirer(e, d)
}

// setExpirer sets an expirer to expire at a future point in time.
//
// The caller must hold fc.l.
Expand All @@ -316,16 +318,14 @@ func (fc *FakeClock) setExpirer(e expirer, d time.Duration) {
})

// Notify blockers of our new waiter.
var blocked []*blocker
count := len(fc.waiters)
for _, b := range fc.blockers {
fc.blockers = slices.DeleteFunc(fc.blockers, func(b *blocker) bool {
if b.count <= count {
close(b.ch)
continue
return true
}
blocked = append(blocked, b)
}
fc.blockers = blocked
return false
})
}

// firer is used by fakeTimer and fakeTicker used to help implement expirer.
Expand Down
135 changes: 133 additions & 2 deletions context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@ package clockwork

import (
"context"
"errors"
"sync"
"time"
)

// contextKey is private to this package so we can ensure uniqueness here. This
// type identifies context values provided by this package.
type contextKey string

// keyClock provides a clock for injecting during tests. If absent, a real clock should be used.
// keyClock provides a clock for injecting during tests. If absent, a real clock
// should be used.
var keyClock = contextKey("clock") // clockwork.Clock

// AddToContext creates a derived context that references the specified clock.
Expand All @@ -21,10 +25,137 @@ func AddToContext(ctx context.Context, clock Clock) context.Context {
return context.WithValue(ctx, keyClock, clock)
}

// FromContext extracts a clock from the context. If not present, a real clock is returned.
// FromContext extracts a clock from the context. If not present, a real clock
// is returned.
func FromContext(ctx context.Context) Clock {
if clock, ok := ctx.Value(keyClock).(Clock); ok {
return clock
}
return NewRealClock()
}

// WithDeadline returns a context with a deadline based on a [FakeClock].
//
// The returned context ignores parent cancelation if the parent was cancelled
// with a [context.DeadlineExceeded] error. Any other error returned by the
// parent is treated normally, cancelling the returned context.
//
// If the parent is cancelled with a [context.DeadlineExceeded] error, the only
// way to then cancel the returned context is by calling the returned
// context.CancelFunc.
func WithDeadline(parent context.Context, clock *FakeClock, t time.Time) (context.Context, context.CancelFunc) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey. I'm trying to understand how it supposed to be used.

Case that I have in mind:

func Handler(ctx context.Context) {
   ctx := context.WithTimeout(ctx, 5*time.Second)
   ...
   // do some method with a maximum of 5 seconds timeout.
   select {
      case <-readyChan:
         // finished work
      case ctx.Done():
         // handling timeout
   }
}

That is why in my PR I proposed a generic method that inputs Clock, not *FakeClock.

I wonder which usage you have in mind for these methods.

Copy link
Collaborator Author

@DPJacques DPJacques Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is more of a question on how to inject a FakeClock into a scope.

In your case above, I'd do:

type Handler struct {
  fakeClock *clockwork.FakeClock
}

func (h Handler) Handle(ctx context.Context) {
  if h.fakeClock == nil {
    // Running in prod
    ctx = context.WithTimeout(ctx, 5*time.Second)
  } else {
    // Running in test
    ctx = clockwork.WithTimeout(ctx, h.fakeClock, 5*time.Second)
  }

  ...
  // do some method with a maximum of 5 seconds timeout.
  select {
    case <-readyChan: // finished work 
    case <-ctx.Done(): // handling timeout
  }
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that FakeClock goes to production, which is undesirable. Usually, I pass the Clock interface used in prod and tests. Otherwise, the code path diverges between tests and production.
In your example, h.fakeClock != nil path is impossible to test.

Copy link
Collaborator Author

@DPJacques DPJacques Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that FakeClock goes to production, which is undesirable.

Production code will have a clockwork.Clock interface that switches between a RealClock and FakeClock, so a FakeClock is "in production" in either case, it just hides behind an interface most of the time. Here it would be nil in production, so I wouldn't qualify that as "in production"

Otherwise, the code path diverges between tests and production.

This always happens to some non-zero degree when you have to inject a clock. You have to communicate to your code whether to use a RealClock or a FakeClock somehow.

I don't see a single if-else statement as terribly egregious, The newly created context uses the same code path regardless of how it is initialized.

In your example, h.fakeClock != nil path is impossible to test.

The following would work:

// In test
fc := clockwork.NewFakeClock()
h := Handler{fakeClock: fc}
go func() {
  h.Handle( context.Background())
} ()

// do things with fc.

// in prod - for posterity
h := Handler{}
h.Handle(context.Background())

In general, is your contention that this PR's implementation of clockwork.WithTimeout and clockwork.WithDeadline should take a clockwork.Clock rather than a FakeClock? If so, that is not possible to do without introducing potential race conditions or type inspection to extract the FakeClock (in which case, you should just pass the FakeClock, IMHO).

We could change this so if clockwork.WithTimeout and clockwork.WithDeadline receive a nil clock, they just fall back to context.WithTimeout and context.WithDeadline. Would that help assuage your concerns?

Copy link

@3vilhamster 3vilhamster Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not look like a nice API to me, but that might be just me.
Even typecasting inside the method would be better.

What I usually have is:

Handler{clock Clock}

which is set to a proper one in prod and a fake one in tests. From my experience, it is the best API and separates the library's responsibility from the developer's.

Your proposal will make all library users aware of Clock and Fakeclock, which does not look like a great design.

In my initial proposal, I used a method instead of a function. That will allow separation of implementation without type assertions.

return newFakeClockContext(parent, t, clock.newTimerAtTime(t, nil).Chan())
}

// WithTimeout returns a context with a timeout based on a [FakeClock].
//
// The returned context follows the same behaviors as [WithDeadline].
func WithTimeout(parent context.Context, clock *FakeClock, d time.Duration) (context.Context, context.CancelFunc) {
t, deadline := clock.newTimer(d, nil)
return newFakeClockContext(parent, deadline, t.Chan())
}

// fakeClockContext implements context.Context, using a fake clock for its
// deadline.
//
// It ignores parent cancellation if the parent is cancelled with
// context.DeadlineExceeded.
type fakeClockContext struct {
parent context.Context
deadline time.Time // The user-facing deadline based on the fake clock's time.

// Tracks timeout/deadline cancellation.
timerDone <-chan time.Time

// Tracks manual calls to the cancel function.
cancel func() // Closes cancelCalled wrapped in a sync.Once.
cancelCalled chan struct{}

// The user-facing data from the context.Context interface.
ctxDone chan struct{} // Returned by Done().
err error // nil until ctxDone is ready to be closed.
}

func newFakeClockContext(parent context.Context, deadline time.Time, timer <-chan time.Time) (context.Context, context.CancelFunc) {
cancelCalled := make(chan struct{})
ctx := &fakeClockContext{
parent: parent,
deadline: deadline,
timerDone: timer,
cancelCalled: cancelCalled,
ctxDone: make(chan struct{}),
cancel: sync.OnceFunc(func() {
close(cancelCalled)
}),
}
ready := make(chan struct{}, 1)
go ctx.runCancel(ready)
<-ready // Cancellation goroutine is running.
return ctx, ctx.cancel
}

func (c *fakeClockContext) Deadline() (time.Time, bool) {
return c.deadline, true
}

func (c *fakeClockContext) Done() <-chan struct{} {
return c.ctxDone
}

func (c *fakeClockContext) Err() error {
<-c.Done() // Don't return the error before it is ready.
return c.err
}

func (c *fakeClockContext) Value(key any) any {
return c.parent.Value(key)
}

// runCancel runs the fakeClockContext's cancel goroutine and returns the
// fakeClockContext's cancel function.
//
// fakeClockContext is then cancelled when any of the following occur:
//
// - The fakeClockContext.done channel is closed by its timer.
// - The returned CancelFunc is executed.
// - The fakeClockContext's parent context is cancelled with an error other
// than context.DeadlineExceeded.
func (c *fakeClockContext) runCancel(ready chan struct{}) {
parentDone := c.parent.Done()

// Close ready when done, just in case the ready signal races with other
// branches of our select statement below.
defer close(ready)

var ctxErr error
for ctxErr == nil {
select {
case <-c.timerDone:
ctxErr = context.DeadlineExceeded

case <-c.cancelCalled:
ctxErr = context.Canceled

case <-parentDone:
parentDone = nil // This case statement can only fire once.

if err := c.parent.Err(); !errors.Is(err, context.DeadlineExceeded) {
// The parent context was canceled with some error other than deadline
// exceeded, so we respect it.
ctxErr = err
}
case ready <- struct{}{}:
// Signals the cancellation goroutine has begun, in an attempt to minimize
// race conditions related to goroutine startup time.
ready = nil // This case statement can only fire once.
}
}

c.setError(ctxErr)
return
}

func (c *fakeClockContext) setError(err error) {
c.err = err
close(c.ctxDone)
}
Loading
Loading