Make backoff account for already-elapsed time. #1132
Conversation
|
On the fence on whether this warrants a changelog entry. Backoff is mostly an implementation detail and it'd be hard to describe the user-facing change in behavior very precisely. |
| @@ -31,6 +31,7 @@ const LOG_TAG = 'ExponentialBackoff'; | |||
| export class ExponentialBackoff { | |||
| private currentBaseMs: number; | |||
| private timerPromise: CancelablePromise<void> | null = null; | |||
| private lastAttempt: Date = new Date(0); | |||
There was a problem hiding this comment.
Since the "current time" is not a portably expressed, maybe make this lastAttemptTime stored as a number?
That way we don't have two different ways of reading the current time in the code (currently new Date().getTime() vs new Date()).
There was a problem hiding this comment.
Renamed to lastAttemptTime and switched to epoch milliseconds.
| ); | ||
|
|
||
| // Some time may have already elapsed so account for that. | ||
| const delaySoFarMs = new Date().getTime() - this.lastAttempt.getTime(); |
There was a problem hiding this comment.
This makes me nervous: new Date().getTime() is not guaranteed to be a monotonic time source. At least it's guaranteed to be in UTC so there's no timezone bug here, but large time adjustments backwards could result in negative values for delaySoFarMs. We should probably ensure this is no smaller than zero so the next line doesn't end up with arbitrarily large delays.
If this class accepted a ticker constructor parameter that returned a new timestamp in ms on each call this logic could be tested.
There was a problem hiding this comment.
Fair point. Reworked to ensure it's >= 0. I don't love the way this code looks (doing Math.max(0, ...) twice in a row for different reasons) but I tried rewriting it a couple different ways and it didn't get much better, so I'm just relying on comments.
I almost considered doing the ticker thing and adding tests, but I think we'd need a mockable random number generator too and if I do that I won't get this ported to the other 2 clients before going OOO. So I just tested it manually again. 😛
If you think it's time to bite the bullet and automate tests, I can though.
| ); | ||
|
|
||
| // Some time may have already elapsed so account for that. | ||
| const delaySoFarMs = new Date().getTime() - this.lastAttempt.getTime(); |
|
|
||
| // Some time may have already elapsed so account for that. | ||
|
|
||
| // Math.max(0, ...) to guard against lastAttemptTime being in the future due |
There was a problem hiding this comment.
nit: rather than repeating the function you're calling, just describe what's happening, i.e.
// Guard against lastAttemptTime being in the future due to a clock change.| // to a clock change. | ||
| const delaySoFarMs = Math.max(0, Date.now() - this.lastAttemptTime); | ||
|
|
||
| // Math.max(0, ...) because the desired backoff may already be elapsed. |
There was a problem hiding this comment.
nit: I think the "Some time may have already elapsed" somewhat duplicates this line. Consider axing it.
(also drop "Max.max ... because")
|
|
||
| // Some time may have already elapsed so account for that. | ||
|
|
||
| // Math.max(0, ...) to guard against lastAttemptTime being in the future due |
| // to a clock change. | ||
| const delaySoFarMs = Math.max(0, Date.now() - this.lastAttemptTime); | ||
|
|
||
| // Math.max(0, ...) because the desired backoff may already be elapsed. |
* Make backoff account for already-elapsed time. [Port of firebase/firebase-js-sdk#1132]
* Make C++ exponential backoff account for already-elapsed time. [Port of firebase/firebase-js-sdk#1132]
No description provided.