luci/common/clock/testclock susceptible to race between .After and context cancellation |
||||
Issue description
+a handful of potentially interested parties
luci/common/clock/testclock has an unexpected (to me) behavior. If you cancel the context that the clock is associated with, timer events that should have fired prior to the cancellation may not actually fire.
Interesting, if I add something like "fmt.Println("foo") inside the testclk.Add loop below, then the test passes (though the number of foos printed fluctuates).
Is this as intended, or not? Am I doing this wrong? Is there some other recommended way to ensure after a call to .Add that all the timers that await dispatch are actually dispatched?
pasting in reproducing code below
=============================================
package repro
import (
"context"
"testing"
"time"
)
import "go.chromium.org/luci/common/clock/testclock"
func TestAfterCancel(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
testclk := testclock.New(testclock.TestRecentTimeLocal)
c := make(chan bool)
// A goroutine that waits 5 seconds using testclock's After and signals true
// (or signals false if context was cancelled)
go func() {
wait := testclk.After(ctx, 5*time.Second)
select {
case <-wait:
c <- true
case <-ctx.Done():
c <- false
}
close(c)
}()
// A goroutine that runs through 100 seconds of testclock time and then
// cancels timer context.
go func() {
defer cancel()
for i := 0; i < 100; i++ {
testclk.Add(1 * time.Second)
}
}()
result := <-c
if result != true {
t.Errorf("Expected %v, actual %v", true, result)
}
}
,
Aug 7
I don't think TestClock is what I'm looking for, ultimately. What I'm looking for is an implementation of the Clock interface that guarantees in-order delivery of timer events. That differs from the intent of TestClock. I propose to create a new implementation of the Clock interface, called "SyncClock" which achieves this. Shouldn't be too hard...
,
Aug 7
I'm also not a fan of the clock.Clock interface, in particular its behavior when a context expires. (it would make more sense to me for channels to be closed then for them to return a "context ended" timer result. I think I will just spin my own library...
,
Aug 8
,
Aug 8
I started on a library for this, but I'm actually questioning whether I even want to proceed along this path for the quota scheduler simulation.
,
Aug 9
i don't see anything unexpected in the behavior of the program in comment 1, nor I am sure it is possible to implement Clock interface such that the program will receive result=true. The goroutine that increments time sends two signals: 1. time reached 5s 2. cancel context via two different channels and the receiving goroutine listens to both channels. The order of receiving from two different channels is arbitrary. It is a race. To sync that, you'd need to wait for the receiving goroutine to exit before canceling the context.
,
Aug 23
|
||||
►
Sign in to add a comment |
||||
Comment 1 by akes...@chromium.org
, Aug 7Behind the scenes, testclk.After is creating a buffered channel via a call to newTimer. // NewTimer returns a new, instantiated timer. func newTimer(ctx context.Context, clk *testClock) *timer { return &timer{ ctx: ctx, clock: clk, tags: clock.Tags(ctx), afterC: make(chan clock.TimerResult, 1), } } I guess there is no deterministic guarantee that a buffered channel will be selected prior to a closed channel. :/