Unclear tokenRefreshFunction documentation leads to errors and potential deadlocks

[azertyfun]

Hi!

We are currently looking at implementing a tokenRefreshFunction for our application's login flow (it simply posts the application's JWT token that is exchanged for an AT by the homeserver), but can't make heads or tails of what is expected of SDK users.

• The SDK automatically calls tokenRefreshFunction when a request fails with M_UNKNOWN_TOKEN, before retrying. Is calling MatrixClient.loginRequest to initialize the client advisable or should we let it authenticate in startClient after it receives M_UNKNOWN_TOKEN on /capabilities? I can't find any relevant documentation. Deprecate MatrixClient.login and replace with loginRequest #4632 suggests re-creating the client with the result of loginRequest but I fail to see the point compared to letting tokenRefreshFunction update the credentials?
• Why does loginRequest call this.http.authedRequest? /login is an unauthenticated endpoint. Not only is the Authorization header redundant, but authedRequest causes a nasty deadlock when calling loginRequest from tokenRefreshFunction which only becomes apparent in certain circumstances:
  1. On M_UNKNOWN_TOKEN, the SDK calls this.tokenRefreshPromise ??= this.doTokenRefresh(attempt);
  2. tokenRefreshFunction calls loginRequest. At this point we haven't yet awaited anything so the tokenRefreshPromise is not yet set. loginRequest continues unimpeded
  3. If it succeeds, everything works(!).
  4. However if it fails with M_LIMIT_EXCEEDED (perfectly normal behavior)
     1. The custom tokenRefreshFunction retries loginRequest after waiting the appropriate amount of time
     1. loginRequest calls this.http.authedRequest.
     1. this.http.authedRequest -> this.tokenRefresher.prepareForRequest -> refreshIfNeeded awaits tokenRefreshPromise... this is a deadlock because we are in said promise! (Note that this would have happened on the first call to loginRequest if tokenRefreshFunction had awaited any promise before then, which allows this.tokenRefreshPromise to be set, so this deadlock can also happen without a retry mechanism)

I am unsure if any of this is intended design or not. What seems to work in our case (whether intended or not) is:

• Setting a dummy token in createClient to ensure tokenRefreshFunction is called on the first authed request
• Implementing tokenRefreshFunction with a retry mechanism to take care of rate-limiting or other server-side failures
• Implementing tokenRefreshFunction with calls matrixClient.http.request() instead of the more intuitive matrixClient.loginRequest() to avoid the deadlock explained above

If this is intended design then this should be clearly documented IMO. If not I would appreciate guidance regarding the best practices.