-
-
Notifications
You must be signed in to change notification settings - Fork 175
Description
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!