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

core/zero: fix ticker usage #4969

Merged
merged 2 commits into from
Mar 6, 2024
Merged

core/zero: fix ticker usage #4969

merged 2 commits into from
Mar 6, 2024

Conversation

calebdoxsey
Copy link
Contributor

@calebdoxsey calebdoxsey commented Feb 29, 2024

Summary

In several places we use a time.Ticker based on a backoff.Backoff. We create a new time.Ticker that fires immediately by using a short duration. Unfortunately this causes excessive CPU usage. There are several ways to fix this:

  1. We can use the backoff.NewTicker function, which returns a "Ticker" that behaves as expected by using a time.Timer instead. This approach does not work if we need to call Reset on the Backoff.
  2. We can use the backoff.Retry function with a backoff.WithContext. We then don't need a timer or ticker.
  3. Instead of a time.Ticker we can use a time.Timer set to the backoff interval. Since a time.Timer is allowed to be set to 0 with no downside, this can handle the case when we want to fire immediately. The benefit over a time.After is we can call .Stop to collect resources if another case in the select fires.

Option (3) was used for the retry.Retry function because the logic around resetting is incompatible with (1) and (2).

Related issues

Fixes https://github.com/pomerium/internal/issues/1727

Checklist

  • reference any related issues
  • updated docs
  • updated unit tests
  • updated UPGRADING.md
  • add appropriate tag (improvement / bug / etc)
  • ready for review

@calebdoxsey calebdoxsey requested a review from a team as a code owner February 29, 2024 21:27
@calebdoxsey calebdoxsey added bug Something isn't working backport 0-25-0 labels Feb 29, 2024
@wasaga wasaga self-requested a review February 29, 2024 21:29
@calebdoxsey
Copy link
Contributor Author

I should also mention that a call to Reset on Timer can only be done if the Timer has already fired, which prevents some ways of solving this problem.

@coveralls
Copy link

coveralls commented Feb 29, 2024

Coverage Status

coverage: 56.456% (+0.03%) from 56.426%
when pulling bec9b5f on cdoxsey/fix-ticker
into 0d2705b on main.

internal/zero/leaser/restart.go Outdated Show resolved Hide resolved
@calebdoxsey calebdoxsey deleted the cdoxsey/fix-ticker branch February 29, 2024 23:46
@calebdoxsey calebdoxsey restored the cdoxsey/fix-ticker branch March 1, 2024 04:27
@calebdoxsey
Copy link
Contributor Author

Sorry forgot I wasn't supposed to close PRs. Re-opening.

@calebdoxsey calebdoxsey reopened this Mar 1, 2024
@wasaga wasaga self-requested a review March 1, 2024 16:45
internal/zero/leaser/restart.go Outdated Show resolved Hide resolved
@calebdoxsey calebdoxsey merged commit 67d1362 into main Mar 6, 2024
12 checks passed
@calebdoxsey calebdoxsey deleted the cdoxsey/fix-ticker branch March 6, 2024 21:28
backport-actions-token bot pushed a commit that referenced this pull request Mar 6, 2024
kenjenkins pushed a commit that referenced this pull request Mar 6, 2024
core/zero: fix ticker usage (#4969)

Co-authored-by: Caleb Doxsey <cdoxsey@pomerium.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 0-25-0 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants