DebouncedChan: When channel is firing continously, only fire once per period #222
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brandur
commented
Feb 22, 2024
| // of +/-2 to allow the channel to be off by two cycles in either direction. | ||
| // By running at `-count 1000` or so I can usually reproduce an | ||
| // off-by-one-or-two cycle. | ||
| expectedNumSignal := int(math.Round(float64(testTime)/float64(cooldown))) + 1 |
There was a problem hiding this comment.
This is the key test change here: notice how we drop the *2 expected invocations.
brandur
force-pushed
the
brandur-fire-only-once-per-period
branch
2 times, most recently
from
af5b789
to
0afda9e
Compare
brandur
force-pushed
the
brandur-fire-only-once-per-period
branch
from
0afda9e
to
a43a1d7
Compare
bgentry
approved these changes
Feb 22, 2024
| @@ -79,25 +83,52 @@ func (d *DebouncedChan) nonBlockingSendOnC() { | |||
| } | |||
| } | |||
|
|
|||
| func (d *DebouncedChan) waitForTimer() { | |||
| // Waits for the timer to be fired, and loops as long as Call invocations comes | |||
There was a problem hiding this comment.
Suggested change
| // Waits for the timer to be fired, and loops as long as Call invocations comes | |
| // Waits for the timer to be fired, and loops as long as Call invocations come |
There was a problem hiding this comment.
Oops, thanks. Fixed!
|
Oh, do you think this is changelog worthy? I would list it as a minor fix in there. |
… period This one's a follow up to #221. `DebouncedChan` previously had a bit of a bug where if `Call` was being invoked continuously, it was firing more often than expected: once for an initial `Call`, and then again when the timer elapsed. Here, we modify the implementation so that under a continuous fire situation, `DebouncedChan` will fire once initially, and then once every time the timer elapses at the end of each period. This reduces the number of emits on `C` from 2N+1 to the more expected N+1 (the +1 being the initial fire). We accomplish this by entering a loop that waits on the timer when receiving an initial `Call`, with the loop continuously resetting and waiting on the timer after each time it fires, sending on `C` each time the period elapses where a `Call` invocation came in. If a period elapses without a new `Call` coming in, the timer stops, the loop returns and `DebouncedChan` returns to its initial state.
brandur
force-pushed
the
brandur-fire-only-once-per-period
branch
from
a43a1d7
to
d593ae1
Compare
Yeah I was having a hard time deciding whether to mention it or not. Added an entry now. |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This one's a follow up to #221.
DebouncedChanpreviously had a bit ofa bug where if
Callwas being invoked continuously, it was firing moreoften than expected: once for an initial
Call, and then again when thetimer elapsed.
Here, we modify the implementation so that under a continuous fire
situation,
DebouncedChanwill fire once initially, and then once everytime the timer elapses at the end of each period. This reduces the
number of emits on
Cfrom 2N+1 to the more expected N+1 (the +1 beingthe initial fire).
We accomplish this by entering a loop that waits on the timer when
receiving an initial
Call, with the loop continuously resetting andwaiting on the timer after each time it fires, sending on
Ceach timethe period elapses where a
Callinvocation came in. If a periodelapses without a new
Callcoming in, the timer stops, the loopreturns and
DebouncedChanreturns to its initial state.