Skip to content

backoff: reset on successful event poll #564

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

Closed
wants to merge 2 commits into from
Closed

backoff: reset on successful event poll #564

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
34f1a4d
backoff: add reset method to reset to minimal wait
chrisirhc Mar 13, 2024
246b7b3
store: reset backoff on success
chrisirhc Mar 13, 2024
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
6 changes: 6 additions & 0 deletions lib/api/backoff.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,10 @@ class BackoffMachine {

_waitsCompleted++;
}

/// Reset the machine to its initial state with minimal wait.
void reset() {
_startTime = null;
_waitsCompleted = 0;
}
}
4 changes: 3 additions & 1 deletion lib/model/store.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,6 @@ class UpdateMachine {
assert(debugLog('Transient error polling event queue for $store: $e\n'
'Backing off, then will retry…'));
// TODO tell user if transient polling errors persist
// TODO reset to short backoff eventually
await backoffMachine.wait();
assert(debugLog('… Backoff wait complete, retrying poll.'));
continue;
Expand All @@ -716,6 +715,9 @@ class UpdateMachine {
if (events.isNotEmpty) {
lastEventId = events.last.id;
}

// On success, reset the backoff.
backoffMachine.reset();
}
}

Expand Down
13 changes: 13 additions & 0 deletions test/api/backoff_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,17 @@ void main() {
check(maxFromAllTrials).isGreaterThan(expectedMax * 0.75);
}
});

test('BackoffMachine resets duration', () async {
final backoffMachine = BackoffMachine();
await measureWait(backoffMachine.wait());
final duration2 = await measureWait(backoffMachine.wait());
check(backoffMachine.waitsCompleted).equals(2);

backoffMachine.reset();
check(backoffMachine.waitsCompleted).equals(0);
final durationReset1 = await measureWait(backoffMachine.wait());
check(durationReset1).isLessThan(duration2);
Comment on lines +63 to +64
Copy link
Member

Choose a reason for hiding this comment

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

This test will flake — it'll fail about 25% of the time. For example:

$ time flutter test test/api/backoff_test.dart 
00:01 +1 -1: BackoffMachine.reset [E]                                            
  Expected: a Duration that:
    is less than <0:00:00.023591>
  Actual: <0:00:00.049613>
  Which: is not less than <0:00:00.023591>
  package:checks/src/checks.dart 85:9             check.<fn>
  package:checks/src/checks.dart 708:12           _TestContext.expect
  package:checks/src/extensions/core.dart 167:13  ComparableChecks.isLessThan
  test/api/backoff_test.dart 64:27                main.<fn>

The trouble is that the behavior of wait is randomized, and the range of possible durations for the second wait (or even the Nth wait for large N) overlaps with the range for the first wait.

That randomization is why the existing test for that method, above, runs 100 trials and then makes a fairly loose check on the results; the parameters are chosen, using some math that's in the comments, to ensure the flake rate is below one in a billion.

(I picked that threshold so that even in a large codebase, even with lots of randomized test cases using that threshold, the total flake rate due to randomization would be negligible compared to other sources of flakiness like races, other bugs, and infra issues.)

We totally could write a working test for this reset method by using the same strategy. But maybe the complexity of that is a good reason to skip the reset method and go with the other approach you mentioned at #564 (comment) , of just resetting a BackoffMachine? local to null.

check(backoffMachine.waitsCompleted).equals(1);
});
}