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

orElse callback when retries are exhausted #167

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

isoos
Copy link
Collaborator

@isoos isoos commented Feb 21, 2022

No description provided.

@isoos isoos requested a review from jonasfj February 21, 2022 16:11
retry/lib/retry.dart Show resolved Hide resolved
@@ -179,10 +184,16 @@ Future<T> retry<T>(
int maxAttempts = 8,
FutureOr<bool> Function(Exception)? retryIf,
FutureOr<void> Function(Exception)? onRetry,
FutureOr<T> Function(Exception)? orElse,
Copy link
Member

Choose a reason for hiding this comment

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

documentation needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.

retry/CHANGELOG.md Show resolved Hide resolved
@slovnicki
Copy link

@isoos @jonasfj How's this progressing?
I found this PR because I needed exactly this feature.

Comment on lines +136 to +142
if (orElse != null) {
return await orElse(e);
}
Copy link
Member

Choose a reason for hiding this comment

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

There are two kinds of exceptions here:

  • (A) Exceptions that trigger a retry -- those where retryIf(e) returns true.
  • (B) Exceptions that don't trigger a retry.

There are two reasons a call to retry(fn, ...) may throw:

  • (i) fn throws an exception of type (B),
  • (ii) fn throws an exception of type (A) enough times that the number of retries have been exhausted.

Do we think it's right to call orElse in both cases (i) and (ii)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, this is an old one that is still open :)

I think we should only have an alternative callback for (ii), and with that in mind, orElse is not the best name for it. How about afterExhausted or onAttemptsExhausted?

Copy link
Member

Choose a reason for hiding this comment

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

I think orElse is an okay name, but we might have to document the behavior -- if we want this at all.

@jonasfj
Copy link
Member

jonasfj commented Oct 26, 2022

@slovnicki, feel free to make a new PR, or comment on this one. I don't think we've resolve the semantics.

side note: I don't think we want breaking changes, hence throwing a RetryExhaustedException is not really a good option.

@drown0315
Copy link

I think to add stackTrace parameter for the orElse method would better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants