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

[core-amqp] Update retry to remove final sleep after all attempts are exhausted #12610

Closed
chradek opened this issue Nov 18, 2020 · 0 comments · Fixed by #12698
Closed

[core-amqp] Update retry to remove final sleep after all attempts are exhausted #12610

chradek opened this issue Nov 18, 2020 · 0 comments · Fixed by #12698
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.

Comments

@chradek
Copy link
Contributor

chradek commented Nov 18, 2020

Reference: #12587 (comment)

Currently the retry function in @azure/core-amqp contains a loop that includes a sleep that matches the user's retryDelayInMs if a retryable error is encountered while invoking the provided operation.

if (lastError && lastError.retryable) {
logger.verbose(
"[%s] Sleeping for %d milliseconds for '%s'.",
config.connectionId,
targetDelayInMs,
config.operationType
);
await delay(
targetDelayInMs,
config.abortSignal,
`The retry operation has been cancelled by the user.`
);
continue;

In the scenario where the final allowed attempt fails with a retryable error, this function will still apply the sleep before throwing the error. In @azure/event-hubs, which uses a default retryDelayInMs of 30000 (30 seconds), this means the user's application has to wait an extra 30 seconds between when the last error is encountered and when it is thrown to the user's code.

Instead, we should add a check to see if the total number of retry attempts have been exhausted, and if they have, skip the sleep.

@chradek chradek added Client This issue points to a problem in the data-plane of the library. Azure.Core labels Nov 18, 2020
@chradek chradek added this to the [2020] December milestone Nov 18, 2020
@ghost ghost closed this as completed in #12698 Dec 3, 2020
ghost pushed a commit that referenced this issue Dec 3, 2020
…e attempts are exhausted (#12698)

Fixes #12610 

This change is pretty straightforward.

### Expected `retry` behavior:
If the operation passed to `retry` fails with a `retryable` error __and__ the maximum number of retries allowed has not been reached, `retry` should 'sleep' between operation attempts.

### Actual `retry` behavior pre-change:
If the operation passed to `retry` fails with a `retryable` error `retry` adds a 'sleep'.

This means that even if the final 'attempt' results in a retryable error being thrown, `retry` still sleeps before yielding the error to the calling code.

### Actual `retry` behavior post-change:
Matches the expected `retry` behavior. There is a 'sleep` only _between_ operation attempts, but not after all attempts have been exhausted.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Projects
None yet
1 participant