-
Notifications
You must be signed in to change notification settings - Fork 940
Improve usage and testing of delayed operations. #499
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,8 +15,8 @@ | |
*/ | ||
|
||
import * as log from '../util/log'; | ||
import { Deferred } from '../util/promise'; | ||
|
||
import { CancelablePromise } from '../util/promise'; | ||
import { AsyncQueue, TimerId } from '../util/async_queue'; | ||
const LOG_TAG = 'ExponentialBackoff'; | ||
|
||
/** | ||
|
@@ -30,8 +30,17 @@ const LOG_TAG = 'ExponentialBackoff'; | |
*/ | ||
export class ExponentialBackoff { | ||
private currentBaseMs: number; | ||
private timerPromise: CancelablePromise<void> | null = null; | ||
|
||
constructor( | ||
/** | ||
* The AsyncQueue to run backoff operations on. | ||
*/ | ||
private readonly queue: AsyncQueue, | ||
/** | ||
* The ID to use when scheduling backoff operations on the AsyncQueue. | ||
*/ | ||
private readonly timerId: TimerId, | ||
/** | ||
* The initial delay (used as the base delay on the first retry attempt). | ||
* Note that jitter will still be applied, so the actual delay could be as | ||
|
@@ -74,10 +83,13 @@ export class ExponentialBackoff { | |
|
||
/** | ||
* Returns a promise that resolves after currentDelayMs, and increases the | ||
* delay for any subsequent attempts. | ||
* delay for any subsequent attempts. If there was a pending backoff operation | ||
* already, it will be canceled. | ||
*/ | ||
backoffAndWait(): Promise<void> { | ||
const def = new Deferred<void>(); | ||
backoffAndRun(op: () => Promise<void>): void { | ||
if (this.timerPromise !== null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the old API, we had a way to tell the caller that its operation got cancelled. Right now, if I follow this correctly, we will just silently drop the previous invocation/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not 100% sure what you're referring to. Prior to my change we never canceled backoff timers. This actually means that you could end up with multiple outstanding backoff timers. While this doesn't /break/ anything, I'm not sure it was intentional and it means we might reconnect earlier than we should. I did change the return type of this method to void instead of Promise, but that's just because we weren't using it and it matches iOS / Android now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's keep it as is for now, but I do think that it would be useful to be able to figure that when operations get cancelled. If we don't need this now, we can deal with across all platforms we end up needing it. |
||
this.timerPromise.cancel(); | ||
} | ||
|
||
// First schedule using the current base (which may be 0 and should be | ||
// honored as such). | ||
|
@@ -89,9 +101,11 @@ export class ExponentialBackoff { | |
`(base delay: ${this.currentBaseMs} ms)` | ||
); | ||
} | ||
setTimeout(() => { | ||
def.resolve(); | ||
}, delayWithJitterMs); | ||
this.timerPromise = this.queue.enqueueAfterDelay( | ||
this.timerId, | ||
delayWithJitterMs, | ||
op | ||
); | ||
|
||
// Apply backoff factor to determine next delay and ensure it is within | ||
// bounds. | ||
|
@@ -102,8 +116,6 @@ export class ExponentialBackoff { | |
if (this.currentBaseMs > this.maxDelayMs) { | ||
this.currentBaseMs = this.maxDelayMs; | ||
} | ||
|
||
return def.promise; | ||
} | ||
|
||
/** Returns a random value in the range [-currentBaseMs/2, currentBaseMs/2] */ | ||
|
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.
backoffAndRun
lets you schedule any type of operation here, but the ID already got assigned in the constructor. Should we move the assignment of the ID to here as well?Uh oh!
There was an error while loading. Please reload this page.
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.
Eh... The intention is that an ExponentialBackoff object is used for one purpose (restarting the listen stream for instance). It wouldn't make sense to do a different kind of operation each time backoff elapses. So I think it'd be more inline with the intent of ExponentialBackoff to move
op
to the constructor, but I think that makes usage of the class more awkward. So I am inclined to keep it as-is.