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.

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

Added retryIfValue and onRetryIfValue to support retry on returned value condition #55

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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: 2 additions & 0 deletions retry/example/main.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import 'dart:async';
import 'dart:io';

import 'package:retry/retry.dart';

Future<void> main() async {
Expand All @@ -34,6 +35,7 @@ Future<void> main() async {
},
// Retry on SocketException or TimeoutException
retryIf: (e) => e is SocketException || e is TimeoutException,
retryIfValue: (v) => v == 500,
);

// Print result from status code
Expand Down
26 changes: 22 additions & 4 deletions retry/lib/retry.dart
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ final _rand = math.Random();
/// () => http.get('https://google.com').timeout(Duration(seconds: 5)),
/// // Retry on SocketException or TimeoutException
/// retryIf: (e) => e is SocketException || e is TimeoutException,
/// retryIfValue: (v) => v == 500,
Copy link
Member

Choose a reason for hiding this comment

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

This example won't work

/// );
/// print(response.body);
/// ```
Expand Down Expand Up @@ -110,9 +111,9 @@ class RetryOptions {
}

/// Call [fn] retrying so long as [retryIf] return `true` for the exception
/// thrown.
/// thrown, or if [retryIfValue] returns `true` for the value returned from [fn].
///
/// At every retry the [onRetry] function will be called (if given). The
/// At every retry the [onRetry] and [onRetryIfValue] functions will be called (if given). The
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be an OR, either onRetry is called or onRetryValue is called?

And don't we need to explain which is called when...

/// function [fn] will be invoked at-most [this.attempts] times.
///
/// If no [retryIf] function is given this will retry any for any [Exception]
Expand All @@ -122,13 +123,22 @@ class RetryOptions {
FutureOr<T> Function() fn, {
FutureOr<bool> Function(Exception) retryIf,
FutureOr<void> Function(Exception) onRetry,
FutureOr<bool> Function(dynamic) retryIfValue,
FutureOr<void> Function(dynamic) onRetryIfValue,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
FutureOr<void> Function(dynamic) onRetryIfValue,
FutureOr<void> Function(T) onRetryValue,

Why the if here? onRetry doesn't have that...

Also why are these dynamic, isn't T a better type? Will that work with void?

}) async {
int attempt = 0;
// ignore: literal_only_boolean_expressions
while (true) {
attempt++; // first invocation is the first attempt
try {
return await fn();
var value = await fn();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var value = await fn();
final value = await fn();

if (retryIfValue == null || !(await retryIfValue(value))) {

Choose a reason for hiding this comment

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

Suggested change
if (retryIfValue == null || !(await retryIfValue(value))) {
if (attempt >= maxAttempts || retryIfValue == null || !(await retryIfValue(value))) {

Shouldn't there also be a maxAttempts check?
Otherwise, the users would have to count by themselves to not exceed the attempts if the value/result repeatedly triggers a retry. (e.g. HTTP 503 for longer than 30 seconds)

return value;
} else {
if (onRetryIfValue != null) {
await onRetryIfValue(value);
}
}
} on Exception catch (e) {
if (attempt >= maxAttempts ||
(retryIf != null && !(await retryIf(e)))) {
Expand Down Expand Up @@ -179,10 +189,18 @@ Future<T> retry<T>(
int maxAttempts = 8,
FutureOr<bool> Function(Exception) retryIf,
FutureOr<void> Function(Exception) onRetry,
FutureOr<bool> Function(dynamic) retryIfValue,
FutureOr<void> Function(dynamic) onRetryIfValue,
}) =>
RetryOptions(
delayFactor: delayFactor,
randomizationFactor: randomizationFactor,
maxDelay: maxDelay,
maxAttempts: maxAttempts,
).retry(fn, retryIf: retryIf, onRetry: onRetry);
).retry(
fn,
retryIf: retryIf,
onRetry: onRetry,
retryIfValue: retryIfValue,
onRetryIfValue: onRetryIfValue,
);
14 changes: 14 additions & 0 deletions retry/test/retry_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -131,5 +131,19 @@ void main() {
await expectLater(f, throwsA(isException));
expect(count, equals(2));
});

test('retry (retryIfValue, success after 3)', () async {
int count = 0;
final r = RetryOptions(
maxAttempts: 5,
maxDelay: Duration(),
);
final f = r.retry(() {
count++;
return count;
}, retryIfValue: (v) => v < 3);
await expectLater(f, completion(3));
expect(count, equals(3));
});
});
}