fix(refresh-session): forceRefreshSession does not reset storageSilentRenewRunning
#1943
Conversation
projects/angular-auth-oidc-client/src/lib/callback/refresh-session.service.ts
Outdated
Show resolved
Hide resolved
projects/angular-auth-oidc-client/src/lib/callback/refresh-session.service.ts
Outdated
Show resolved
Hide resolved
| @@ -143,6 +164,7 @@ export class RefreshSessionService { | |||
| `forceRefreshSession timeout. Attempt #${currentAttempt}` | |||
| ); | |||
|
|
|||
| // Still needed ? | |||
There was a problem hiding this comment.
What's the reasoning behind the new events?
I think it can also be solved by invoking resetSilentRenewRunning within the callback at https://github.com/damienbod/angular-auth-oidc-client/blob/main/projects/angular-auth-oidc-client/src/lib/callback/refresh-session.service.ts#L94. Similar to the current approach at https://github.com/damienbod/angular-auth-oidc-client/blob/main/projects/angular-auth-oidc-client/src/lib/callback/refresh-session.service.ts#L146
There was a problem hiding this comment.
Now that you ask me and after thinking a bit more, as forceRefreshSession returns an Observable, this is indeed useless to create those new events, as we can achieve the same right now.
I will remove the new events.
On why I put resetSilentRenewRunning line 95 in a finalize (tap when I'll remove the events), it's because I prefer to not duplicate logic, that's why I was wondering if the one line 167 was still needed, but I think it's not anymore.
But both are fine for me, as it will fix the issue anyway.
Any "preference"?
There was a problem hiding this comment.
Looking a bit more, I think, if we hit this line https://github.com/damienbod/angular-auth-oidc-client/blob/main/projects/angular-auth-oidc-client/src/lib/callback/refresh-session.service.ts#L138, we will face the same problem, not being able to re-use forceRefreshSession as resetSilentRenewRunning will not be called.
And resetSilentRenewRunning line 138 is only being called when there's the timeout and it retry, if there's no error, resetSilentRenewRunning seems not to be called, meaning we'll also face the problem.
Putting it on this.forceRefreshSession will probably fixes the issue in all cases.
There was a problem hiding this comment.
From my point of view this change looks good 👍
Side note: if you force the refresh token very rapidly (when a request is already pending) then this results in an error "0-angularCodeRefreshTokens - no refresh token found, please login", but I think this behavior was already there and isn't caused by this change.
Hello,
This PR fixes #1942.