Problem when trying to use RetryCallback to handle token expiration

[fegabe]

Hello,

first of all congratulations for this awesome project, the code is great and is really flexible to suit all my needs in a pretty complex project I'm working on.

In this project I have to handle refresh token expiration so I checked in the documentation that RetryCallback would be a great solution to catch when an error is because the token expired and refreshing the token before the next retry. When trying this approach, I faced several problems so maybe I'm following a wrong approach and I should be using a different strategy. But since I wasn't able to figure out a better solution I have done some local changes to have it working properly.

I'm more than welcome to send you a PR, but first I wanted to check the approach in this issue.

Problem 1: RetryCallback is called only when it's a network error

Regarding this line: https://github.com/proyecto26/RestClient/blob/develop/src/Proyecto26.RestClient/Helpers/HttpBase.cs#L26, RetryCallback is only called when the error is because a network error, which is not my case since the error code is a 401 (unauthorized).

My proposal is to remove that check in the line at let the user decide inside the callback if this error is relevant or not for his/her use case

Problem 2: Retry delay should happen after calling to RetryCallback

Iniside my RetryCallback I detect if the error is because my session token expired and in that case I refresh it using a different API call. With the current implementation (here https://github.com/proyecto26/RestClient/blob/develop/src/Proyecto26.RestClient/Helpers/HttpBase.cs#L28), the delay is performed before calling to RetryCallback, so after the delay, the RetryCallback performs the API call to refresh the token but at the same time is fired the retry so it won't be yet fixed.

My proposal is to move the line 28 down, to the line 34 for example. This way I can perform the refresh token during the delay and when it resumes the access token will be "fresh".

Problem 3: Be able to modify the request inside the RetryCallback block

Once I perform the refresh token call, I need a way to modify the request before retrying it in order to change the access token for the new one.

My proposal is to make the RequestHelper instance available inside the RetryCallback, e.g. including the instance inside RequestException class. So I can change the proper header before the retry. There are probably more elegant solutions to this but I've found this working for me.

What do you think about these changes? Do you want me to prepare a PR to check those or do you have any other suggestions?

Thanks!

[jdnichollsc]

About Problem 1, that validation was introduced in this PR => #115

[fegabe]

I guess it's true that if the error is a 404 (or any 5xx) there is no point on retrying but then.. how should I deal if I want to have a retry callback for an error like 401 unauthorized? Maybe introducing a new flag in the RequestHelper to let the user decide if want the RetryCallback to be called just for network errors (default case) or for any error?

[jdnichollsc]

Hi Fernando, sorry for the delay, I've been very busy these days, but let me review your comments ASAP 👍

[fegabe]

Great @jdnichollsc, thanks for your time! Meanwhile I've been using the changes that you can see in this PR #142. I'm not sure how you create the release from the source project so maybe I'm missing something else.

[jdnichollsc]

Thanks for your help mate, please check the changes of the new release and let me know! 👍 #196