Skip to content

Conversation

@isoos
Copy link
Collaborator

@isoos isoos commented Dec 13, 2024

  • Retry ClientException from package:http #8337
  • The RetryEnforcerStorage wrapper (used only in tests) and related classes parse the current stacktrace when a storage-method is called, and throw an assertion error if retry is not in the stack. The detection is rather rudimentary (e.g. needs reentry detection for list + arbitrary processing), but it provides some value already. Further improvements can be done with better parsing (TBD later).
  • Most of the methods are currently ignoring the lack of retry, as it seems a lot to fix everything all at once. The current updates focus on the API exporter admin test (it seems to be covered 100%), enabling the assertion error while I was investigating the test case.
  • I think for some operation it could have been a simple withStorage<R>(Future<R> Function(Storage storage)) {...} helper method that retries internally, but since these operation are not in a transaction, it seems wasted to retry more than one at once.

@isoos isoos requested review from jonasfj and sigurdm December 13, 2024 15:34
}

print('Missing retry detected:\n$st\n');
throw AssertionError('retry is not present in stacktrace: $st');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also detect that we only have a single retry call on the stack?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should allow multiple re-entry in the retry stack, e.g. one for a high-level iteration of items, while the second one may be narrower operations on it.

We could ignore re-entry detection in the retry stack if we could check if the current call is sufficiently close to the latest retry invocation. But I would consider that check to come later, after the low-hanging fixes (e.g. no retry at all) has been done.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should allow multiple re-entry in the retry stack, e.g. one for a high-level iteration of items, while the second one may be narrower operations on it.

Yeah - we would need to have some kind of design here. But nested retries easily leads to n^2 attempts...

But I would consider that check to come later,

SG.

@isoos isoos merged commit 6352f10 into dart-lang:master Dec 16, 2024
32 checks passed
@isoos isoos deleted the retry branch December 16, 2024 09:00
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.

2 participants