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

timer: Add a NewTimer method to Clock #13

Merged
merged 1 commit into from
Dec 18, 2023
Merged

timer: Add a NewTimer method to Clock #13

merged 1 commit into from
Dec 18, 2023

Conversation

dfinkel
Copy link
Contributor

@dfinkel dfinkel commented Dec 8, 2023

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.

@dfinkel dfinkel requested review from arodland and silvenac December 8, 2023 17:06
}
}

// Test that cancels and advances the clock in parallel to guarantee that timer

Choose a reason for hiding this comment

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

maybe i'm just tired but this doc is just not absorbing for me haha. is there a typo maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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{}.

Copy link

@silvenac silvenac left a comment

Choose a reason for hiding this comment

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

lgtm! just a very nitty question

Copy link
Contributor

@sergiosalvatore sergiosalvatore left a comment

Choose a reason for hiding this comment

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

A couple minor nits...

}
}

// RegisteredTimers returns the execution-times of registered timeres.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/timeres/timers/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's definitely a typo.

Fixed.

fc.AwaitAggExtractedChans(1)

if sl := fc.NumSleepers(); sl != 0 {
t.Errorf("unexpected sleeper-count: %d; expected 1", sl)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be "expected 0"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed.
Fixed.

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))
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here -- "expected 0"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

close(testWakeCh)

<-ch

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm -- this test doesn't seem to assert anything...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

t.timers[ft] = wakeTime
}

// returns true if the timer was successfully removed
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe more specifically, "returns true if the timer existed in the first place"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, it may have already been removed.
I've reworded it to returns true if the timer was previously present

// 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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like your wording 🙂
Dropped in verbatim.

// 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.
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

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
@dfinkel dfinkel merged commit 428d684 into master Dec 18, 2023
6 checks passed
@dfinkel dfinkel deleted the timer branch December 18, 2023 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants