Implement exponential backoff for backend errors

[vmsh0]

Hi,

I'm opening this pull request with an initial proof-of-concept implementation of exponential backoff for backend interactions and uploads. In my evaluation, the feature might be useful as shown by #2399 and https://forum.duplicati.com/t/scheduled-backup-failure-google-drive-403-forbidden/13160.

I've experienced similar issues myself (I have an ~8TB backup which has been going since the beginning of December, more details in #2399), which I mitigated by increasing the number of retries and the timeout, but the solution is sub-optimal, because /all/ retries are of course penalized by the increased delay.

The implementation I'm presenting here is very simple and straightforward. Please forgive my unfamiliarity with the project style an conventions. I'm hoping someone from the project could guide me towards producing further iterations of this PR and reaching a solution suitable for inclusion.

[warwickmm]

Thanks @vmsh0. Our style conventions have evolved and are not consistent throughout the solution. However, you did the right thing by being consistent with the style of the surrounding code.

[vmsh0]

I have pushed a new version of the patch, addressing the previous comments, as well as introducing a clamp to the exponential backoff exponent. I think 10 is a sane default, but should we add a parameter to allow changing it?

[warwickmm]

I think 10 is a sane default, but should we add a parameter to allow changing it?

I think 10 is a reasonable default. To avoid having too many options, we can leave this as-is until someone makes the case for this to be customizable.

[vmsh0]

I implemented the suggested changes, and rebased my patch on your latest master.

(please forgive the mess in the PR log, I had accidentally force-pushed the wrong local branch to my remote master)

[warwickmm]

Looks good to me. Thanks @vmsh0!

[duplicatibot]

This pull request has been mentioned on Duplicati. There might be relevant details there:

https://forum.duplicati.com/t/failed-the-remote-server-returned-an-error-403-forbidden-google-drive/1623/49

[duplicatibot]

This pull request has been mentioned on Duplicati. There might be relevant details there:

https://forum.duplicati.com/t/is-there-a-work-arround-for-google-drive-403/14148/3

[duplicatibot]

This pull request has been mentioned on Duplicati. There might be relevant details there:

https://forum.duplicati.com/t/is-there-a-work-arround-for-google-drive-403/14148/25

[Paul-5]

Hi @vmsh0!, warwickmm just let me know about this fix after I implemented something similar but it was specifically for Google Services & Drive. A couple of things I've observed about this fix. It appears that the first thing it does in DoWithRetry is delay and then run the operation, unless I'm misreading the code it should be the other way round with the delay at the end of the loop, possibly within the exception handler.

The other thing I've noticed is that the fix will retry for any exception, whether or not it's a retriable exception. I realise that each backend has its own way of alerting errors, particularly retriable errors. I was thinking that a base class for backends could implement a virtual method maybe called "IsRetriableException(Exception ex)" that returns true in the base class. When overridden in a backend handler it could interrogate the exception and determine whether the exception was truly retriable for that backend. If the exception is retriable then the code would enter the retry delay loops otherwise it would simply rethrow the exception.

Would you be happy for me make a few modifications to this fix accordingly?

[vmsh0]

Hi @Paul-5,

Thank you for your feedback.

It appears that the first thing it does in DoWithRetry is delay and then run the operation, unless I'm misreading the code it should be the other way round with the delay at the end of the loop, possibly within the exception handler.

That is not correct. The conditional block at line 320 skips the delay for the first iteration.

The other thing I've noticed is that the fix will retry for any exception, whether or not it's a retriable exception. I realise that each backend has its own way of alerting errors, particularly retriable errors. I was thinking that a base class for backends could implement a virtual method maybe called "IsRetriableException(Exception ex)" that returns true in the base class. When overridden in a backend handler it could interrogate the exception and determine whether the exception was truly retriable for that backend. If the exception is retriable then the code would enter the retry delay loops otherwise it would simply rethrow the exception.

I would suggest you to discuss it with any of the more active Duplicati developers. I have only made this one fix, and gave a small contribution to a discussion related to a completely different issue, and as such I'm afraid I have nothing interesting to say! Fwiw, it does sound like a sensible idea.

Would you be happy for me make a few modifications to this fix accordingly?

Please note that my PR only changes the way the duration of the delays is computed. It does not change or address any of the delay logic, which from what I can see has been in place for ~3 years now. As such, I think it might be a better idea to open a separate pull request for your proposed improvements regarding retriable exceptions (possibly after discussing it on the forum!) rather than contributing to this one.

[Paul-5]

That is not correct. The conditional block at line 320 skips the delay for the first iteration.

Ah, yes, thank you, I see it now.

As such, I think it might be a better idea to open a separate pull request for your proposed improvements regarding retriable exceptions (possibly after discussing it on the forum!) rather than contributing to this one.

Will do, thank you. I just want to kick around the idea before implementing as it affects backend handlers, the majority of which I have not looked at... I've only played with Google Drive.

Thank you for your help and for your fix.

[duplicatibot]

This pull request has been mentioned on Duplicati. There might be relevant details there:

https://forum.duplicati.com/t/error-while-running-mindestens-ein-fehler-ist-aufgetreten-at-least-one-error-has-occured/14924/9