Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix goroutine leak: make flushDaemon stoppable #293
Fix goroutine leak: make flushDaemon stoppable #293
Changes from 1 commit
0d5c8b7
7ad18de
b6757a5
31a05ea
ec08484
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/retrun/return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should take a
clock.WithTicker
argument, and if it is passed in asnil
, useclock.RealClock
. And document it in the comment above :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment on why sleep? This is usually a smell...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or inject a fake ticket so you can manually "advance time" rather than sleep. Deterministic tests FTW
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedback. I'm not quite sure what you mean by "fake ticket". Did you mean ticker? I think the problem here is the startup time of the goroutine, not the flush interval, so I don't understand how a fake ticker could help with that.
But I agree that tests should be deterministic if possible and I'd be glad if you could give me some more hints on how to make it deterministic. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant "ticker".
e.g. staging/src/k8s.io/client-go/util/workqueue/rate_limiting_queue_test.go - it uses
testingclock.NewFakeClock
(k8s.io/utils/clock/testing) which has aNewTicker
method.Pass a
k8s.io/utils/clock.WithTicker
tonewFlushDaemon()
(passingnil
means "use time.NewTicker()") and store that in theflushDaemon
struct. Inrun()
you can check if the passed-inclock.WithTicker
is nil. If so, do what this does now (use a real ticker) but if it is not nil, use it instead.Then your test manipulates the fake clock's time to cause ticks when you expect them.
As for the daemon startup - you already call isRunning(), so you know it's alive...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the clock, thanks for the great tip.
In addition, I had to move the creation of the ticker out of the go function and add a channel for communication between the spy function and the test.