timer: Add a NewTimer method to Clock #13
Conversation
fake/fake_clock_test.go
Outdated
| } | ||
| } | ||
|
|
||
| // Test that cancels and advances the clock in parallel to guarantee that timer |
There was a problem hiding this comment.
maybe i'm just tired but this doc is just not absorbing for me haha. is there a typo maybe?
There was a problem hiding this comment.
oh, yeah, that was missing a couple words. (it's canceling the timer, not the clock)
I've also simplified the test a touch by using a channel of type chan struct{}.
fake/fake_clock.go
Outdated
| } | ||
| } | ||
|
|
||
| // RegisteredTimers returns the execution-times of registered timeres. |
There was a problem hiding this comment.
nit: s/timeres/timers/
There was a problem hiding this comment.
Yep, that's definitely a typo.
Fixed.
fake/fake_clock_test.go
Outdated
| fc.AwaitAggExtractedChans(1) | ||
|
|
||
| if sl := fc.NumSleepers(); sl != 0 { | ||
| t.Errorf("unexpected sleeper-count: %d; expected 1", sl) |
There was a problem hiding this comment.
I think this should be "expected 0"
fake/fake_clock_test.go
Outdated
| t.Errorf("unexpected sleeper-count: %d; expected 1", sl) | ||
| } | ||
| if sl := fc.Sleepers(); len(sl) != 0 { | ||
| t.Errorf("unexpected sleeper-count: %d; expected 1", len(sl)) |
There was a problem hiding this comment.
Also here -- "expected 0"
| close(testWakeCh) | ||
|
|
||
| <-ch | ||
|
|
There was a problem hiding this comment.
Hmm -- this test doesn't seem to assert anything...
There was a problem hiding this comment.
That's right 🙂
It's relying on either concurrent map mutation panics or the race-detector to fail the test if something unsafe is going on.
Since it's specifically there to exercise the synchronization, there are two intermediate states, and one final state (which is empty).
fake/fake_timer.go
Outdated
| t.timers[ft] = wakeTime | ||
| } | ||
|
|
||
| // returns true if the timer was successfully removed |
There was a problem hiding this comment.
Maybe more specifically, "returns true if the timer existed in the first place"
There was a problem hiding this comment.
well, it may have already been removed.
I've reworded it to returns true if the timer was previously present
fake/fake_clock.go
Outdated
| // AwaitAggExtractedChans waits the aggregate number of calls to Ch() on | ||
| // timers to equal or exceed its argument. | ||
| // To be be most useful, uses of the channel should directly call `.Ch()` on | ||
| // the timers and dereferencing the channel pointer. |
There was a problem hiding this comment.
This seems a bit unclear to me. Maybe more like:
For this method to be most useful, users of timers should not store the value of .Ch(). Instead, call .Ch(), dereference the pointer, and attempt a receive immediately, as in case <-*timer.Ch().
There was a problem hiding this comment.
I like your wording 🙂
Dropped in verbatim.
fake/fake_clock.go
Outdated
| // NumAggExtractedChans returns the aggregate number of calls to Ch() on | ||
| // timers. | ||
| // To be be most useful, uses of the channel should directly call `.Ch()` on | ||
| // the timers and dereferencing the channel pointer. |
The select statement/block is a major strength of Go, but it provides a few challenges for the fake clock. In particular, we need a way to know when the channel has been extracted, and ideally when it's in use. To support tracking how many timers have their channels extracted, we return a pointer to a channel that lives on an anonymous struct and leverage runtime.SetFinalizer to decrement a reference-count when it would get GC'd (likely because the select block was exited). However, finalizers may be run a bit earlier than one would otherwise expect (the documentation for [runtime.SetFinalizer] indicates instruction/statement-level granularity on usage -- hence the existence of [runtime.KeepAlive]) Due to the laxness of the guarantees from runtime.SetFinalizer in the absense of caller-help with runtime.KeepaAlive calls, we don't expose the finalizer-based accounting, and leave those unexported. We can decide later whether to remove the finalizer-based accounting. I'd like to get some mileage with it before deciding whether it would even be useful. On the bright side, the call to get the channel gives us a signal as to when we're in the select block, and facilitates the AwaitAggExtractedChans, and NumAggExtractedChans methods. [runtime.SetFinalizer]: https://pkg.go.dev/runtime#SetFinalizer [runtime.SetFinalizer]: https://pkg.go.dev/runtime#KeepAlive
The select statement/block is a major strength of Go, but it provides a few challenges for the fake clock. In particular, we need a way to know when the channel has been extracted, and ideally when it's in use.
To support tracking how many timers have their channels extracted, we return a pointer to a channel that lives on an anonymous struct and leverage runtime.SetFinalizer to decrement a reference-count when it would get GC'd (likely because the select block was exited).
However, finalizers may be run a bit earlier than one would otherwise expect (the documentation for runtime.SetFinalizer indicates instruction/statement-level granularity on usage -- hence the existence of runtime.KeepAlive)
Due to the laxness of the guarantees from runtime.SetFinalizer in the absense of caller-help with runtime.KeepaAlive calls, we don't expose the finalizer-based accounting, and leave those unexported.
We can decide later whether to remove the finalizer-based accounting. I'd like to get some mileage with it before deciding whether it would even be useful.
On the bright side, the call to get the channel gives us a signal as to when we're in the select block, and facilitates the AwaitAggExtractedChans, and NumAggExtractedChans methods.