Open orgads opened 5 months ago
@compulim, @OEvgeny
Thanks for investigating and for the repro. As a user-land solution you could also consider extending the DirectLine
class with own refresh token implementation.
I've analyzed the issue and have a proposal for addressing it (cc @compulim):
The application crashes when the token refresh fails (e.g., with a 403 error). This occurs in the refreshTokenLoop
method, which is called from checkConnection
. As the refresh token loop doesn't interfere with connectionStatus$
subject, the error doesn't get propagated:
I see why this might be done on-purpose. The cause looks like the one we cannot recover direct line from, so trying to crash the runtime to give both: visibility and the ability to restart the whole process seems valid.
We can probably enhance error handling though without changing the public API e.g. to allow replacing the DirctLine
with a new one:
Modify refreshTokenLoop
to catch errors and continue the loop:
private refreshTokenLoop() {
this.tokenRefreshSubscription = Observable.interval(intervalRefreshToken)
.flatMap(_ => this.refreshToken()
.pipe(
catchError(error => {
this.handleFatalError(error);
return Observable.empty();
})
)
)
.subscribe(/* ... */);
}
Update refreshToken
to simplify error handling:
private refreshToken(): Observable<string> {
return this.checkConnection(true)
.flatMap(/* ... */)
.pipe(
catchError(error => {
if (error.status === 403 || error.status === 404) {
return Observable.throw(error);
}
return Observable.throw(error).pipe(
retryWhen(/* ... */)
);
})
);
}
Add handleFatalError
to emit an error through connectionStatus$
:
private handleFatalError(error) {
this.connectionStatus$.error(error);
}
While addressing the issue this seems to:
Users would be able to catch token expiration errors as follows:
directLine.connectionStatus$.subscribe(
status => console.log('Status:', ConnectionStatus[status]),
error => {
console.error('Error:', error);
// Implement recovery logic here
}
);
This proposal could be considered a minor version change as it adds functionality without breaking existing API contracts, but also we do not crash as before, so this may be considered as a breaking change as well. We might be able to work further towards more compatible solution e.g. by checking the observers for error handling.
Any thoughts?
If
tokens/refresh
returns an error, the application crashes, and there is no way to catch and handle it properly.Reproduced by:
If you don't want to wait 15 minutes, reduce the hardcoded value of
lifetimeRefreshToken
:)Output: