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

backoff: fix flaky tests in backoff cache #1516

Merged
merged 2 commits into from
May 24, 2022
Merged

backoff: fix flaky tests in backoff cache #1516

merged 2 commits into from
May 24, 2022

Conversation

MarcoPolo
Copy link
Collaborator

Fixes #1502 by introducing a mocked clock. Time can be incremented in the mock clock and things check the clocks time via .Now(). If no clock is passed in, the default realClock uses time.Now

This means that these tests are not dependent on actual wall clock times, but our monotonically increasing mock clock.

This PR also fixes a subtle bug that would cause the limit of a sendingChs to be ignored. And a bug that prevented other sendingChs from getting sent data.

delete(c.sendingChs, ch)
break
} else if rem > 0 {
rem--
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is broken, right? This is a copy of an int not a reference so it wouldn't affect the rem value in the map at all.

ch <- sendAi
if rem == 1 {
close(ch)
delete(c.sendingChs, ch)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's no point doing this here, we can clean everything up in one go at the end.

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

In other parts of the code, we use https://github.com/benbjohnson/clock to mock the clock. Can we reuse that package here?

@MarcoPolo
Copy link
Collaborator Author

In other parts of the code, we use https://github.com/benbjohnson/clock to mock the clock. Can we reuse that package here?

Sure thing. Taking a look at that library I noticed they used a pointer receiver though, which goes against my goal of having the real clock be zero sized. It doesn't actually matter I guess but worth pointing out.

Here's some benchmarks:
https://gist.github.com/MarcoPolo/abde3e02d8a6ae49933fe28778b38ffd

The pointer receiver version is 45ns/op vs 38ns/op.

@MarcoPolo
Copy link
Collaborator Author

I can actually keep the real clock as I have it here and use this only in tests.

@marten-seemann marten-seemann changed the title Fix flaky tests in backoff cache backoff: fix flaky tests in backoff cache May 24, 2022
@marten-seemann marten-seemann merged commit bedce22 into master May 24, 2022
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.

flaky TestBackoffDiscoveryMultipleBackoff
2 participants