retryDelayOptions.customBackoff function called for non-retryable errors

[jgehrcke]

SDK version number
v2.730

I noticed that retryDelayOptions.customBackoff() is being called once for non-retryable (e.g., 404-style) errors. The return value is subsequently ignored (no retry is going to happen).

For non-retryable errors there is one call too much which often times is not a problem but of course it can be for side effects triggered by the customBackoff() implementation.

Docs: https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/Config.html#retryDelayOptions-property

That problem was introduced with #2898:

Before #2898:

if (err && err.retryable && retryCount < maxRetries) {
    retryCount++;
    var delay = util.calculateRetryDelay(retryCount, options.retryDelayOptions);
    setTimeout(sendRequest, delay + (err.retryAfter || 0));
}

Note that here calculateRetryDelay() was only ever called when error information was available (err was set) and when err.retryable was true.

After #2898:

var delay = util.calculateRetryDelay(retryCount, options.retryDelayOptions, err);
if (err && err.retryable && retryCount < maxRetries && delay >= 0) {
    retryCount++;
    setTimeout(sendRequest, delay + (err.retryAfter || 0));
}

Note that util.calculateRetryDelay() is always called, w/o inspecting err.retryable.

Notably, it is even getting called when err is not set (which seems to be the case for 404-style API responses!).

However, the actual retry is then only triggered when err && err.retryable -- just like before the patch. That is, in that case the return value of util.calculateRetryDelay() (calling retryDelayOptions.customBackoff()) is ignored.

Proposal:

• Call retryDelayOptions.customBackoff() only when the library internals have determined that err && err.retryable.
• Document that behavior.

Future direction:
If if is up for dispute whether or not a specific error is retryable and custom code should have a word then maybe another hook is required. For example, a hook where calling code can inspect the API response in detail and set err and/or err.retryable. All that happening before retryDelayOptions.customBackoff() gets invoked.

I'd be happy to work on a patch, and in fact might get to doing so still today.

[ajredniwja]

Hey @jgehrcke, thank-you for bringing this up, your initial PR looks good, can you complete all the tests?

[jgehrcke]

Hey! Thanks for picking this up. Are you asking to run tests locally? Can try that tomorrow (is the Travis CI feedback not sufficient?). Or are you asking to add tests?

…

On Wed, Aug 19, 2020, 00:00 Ajwinder Singh ***@***.***> wrote: Hey @jgehrcke <https://github.com/jgehrcke>, thank-you for bringing this up, your initial PR looks good, can you complete all the tests? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3401 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACA3HTM4DWICH3H2YHR2P3SBL2Y7ANCNFSM4QBPBNDQ> .

[ajredniwja]

@jgehrcke I mean all the checklist tests in your PR

[jgehrcke]

I mean all the checklist tests in your PR

@ajredniwja I still have a hard time understanding what "the checklist tests" are. Are you asking me to fill out the checklist in the PR template? I did that initially, commented on each checklist item that I did not check.

I have now also run npm run test and checked that checklist item, and added the outcome to the PR.

Maybe this is still a misunderstanding... .:-). Happy to resolve that.

[ajredniwja]

@jgehrcke You're good, I'll have a look at the PR?

[jgehrcke]

You're good, I'll have a look at the PR?

Thanks and yes, please :-).

[jgehrcke]

@ajredniwja would appreciate if we could move this forward!! Thank you :-) Anything I can still do?

[jgehrcke]

@ajredniwja @AllanFly120 would love for this to not stall. Any help is appreciated. Thank you!

[jgehrcke]

@trivikr is that something you can push forward? Thank you!

[springmeyer]

Another aws-sdk user / premium aws customer who is watching this issue and looking forward to the fix landing (since I use the customBackoff and noticed this PR fixing the inefficiency. Thanks for the work here @jgehrcke!

[ajredniwja]

The PR was merged. Apologies for the delay.

[jgehrcke]

Thank you @ajredniwja and thanks for chiming in @springmeyer.

[peterlundberg]

Have noticed that there is still a problem with customBackoff being called also for errors that are not retryable (such as 404 from S3).

In that case event_listeners will trigger Request.RETRY_CHECK -> service.retryDelays -> calculateRetryDelay -> customBackoff.

Thus one needs a if (error.retryable) {} guard to avoid inefficiencies like calculating the delay or logging the retry attempt. This should at least be documented if true?

Would you prefer re-opening this issue or creating a new one?