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.

Sign up for GitHub

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

Jump to bottom

Fix regression brought by using time.NewTimer() in scale loop #1739

Merged
merged 2 commits into from
Apr 16, 2021
Merged

Fix regression brought by using time.NewTimer() in scale loop #1739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dff727f
fix timer in scale loop
Apr 14, 2021
538d58d
update changelog
Apr 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
## Unreleased

- Add OpenStack Metrics Scaler ([#1382](https://github.com/kedacore/keda/issues/1382))
- Fixed goroutine leaks in usage of timers ([#1704](https://github.com/kedacore/keda/pull/1704))
- Fixed goroutine leaks in usage of timers ([#1704](https://github.com/kedacore/keda/pull/1704) | [#1739](https://github.com/kedacore/keda/pull/1739))

### New

Expand Down
3 changes: 2 additions & 1 deletion pkg/scaling/scale_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,9 @@ func (h *scaleHandler) startScaleLoop(ctx context.Context, withTriggers *kedav1a
pollingInterval := getPollingInterval(withTriggers)
logger.V(1).Info("Watching with pollingInterval", "PollingInterval", pollingInterval)

tmr := time.NewTimer(pollingInterval)
for {
tmr := time.NewTimer(pollingInterval)

Copy link
Contributor

Choose a reason for hiding this comment

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

@zroubalik this is a subtle fix (yet obviously important!) - is it possible somehow to add a test for it? the only apparent way to me would be to pull the timer-related code out into a utility function and then test that. thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

(and if so, I can pick that up in a follow-up PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

I am concerned that pulling that part of code to the utility function would make the code less readable and complex. WDYT?
Anyway more tests are always good, so if you come up with some good way how to test this stuff, I wouldn't be mad :)

Copy link
Contributor

Choose a reason for hiding this comment

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

ha, that was my idea to make it more testable. I don't have others at the moment, but I'll keep my mind open 👍

select {
case <-tmr.C:
h.checkScalers(ctx, scalableObject, scalingMutex)
Expand Down