Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow passing timeout and/or AbortSignal to auth.refreshToken() #409

Closed
mnylen opened this issue Sep 13, 2021 · 3 comments
Closed

Allow passing timeout and/or AbortSignal to auth.refreshToken() #409

mnylen opened this issue Sep 13, 2021 · 3 comments
Labels
feature request A feature has been asked for or suggested by the community waiting for customer This issue is waiting for a response from the issue or PR author

Comments

@mnylen
Copy link
Contributor

mnylen commented Sep 13, 2021

Describe the problem you'd like to have solved

If user's phone has a bad network connection, the auth.refreshToken() call might end up taking a very long time, and in the worst case, will never succeed or fail.

To simulate this happening in iOS, you can install Network Link Conditioner and select the 100 % Loss profile.

Describe the ideal solution

Ideally I would like to be able to pass a timeout parameter to auth.refreshToken() call that makes sure the operation completes after the given timeout, no matter what. Ideally the timeout implementation should pass an AbortSignal to the underlying fetch() call and cancel the request when the timeout occurs.

Of course, this functionality should probably be made available to all methods that make network requests, not just auth.refreshToken().

It would also be nice if the caller could pass their own AbortSignal, if they want to implement custom timeout or cancellation logic.

Alternatives and current work-arounds

It is possible to implement timeout now by creating another promise that rejects after given timeout, and then using Promise.race:

var timerId = null;
var timeoutPromise = new Promise((_, reject) => {
  timerId = setTimeout(() => reject(new Error('timeout')), 8500)
});

try {
  var result = await Promise.race([
    client.auth.refreshToken({ ... }),
    timeoutPromise
  ])
} catch (error) {
  if (error.message === "timeout") {
    // handle timeout
  } else {
    // handle other error
  }  
} finally {
  if (timerId) {
    clearTimeout(timerId)
  }
}

However, this solution is only partial: it won't cancel the request. To be able to cancel the fetch request, an AbortSignal is needed and there is currently no way to pass that.

Additional information, if any

@mnylen mnylen added the feature request A feature has been asked for or suggested by the community label Sep 13, 2021
@lbalmaceda
Copy link
Contributor

@mnylen 👋

  1. The simplest and fastest solution would be for us to add a "default timeout" of, say, 10 seconds, that would impact all the network requests made from this library. It could be achieved with an "internal" AbortSignal and a setTimeout function. This needs to be done in the networking client class.

  2. A customizable approach could be exposing a timeout option in networking client that takes the value in seconds/milliseconds; but also expose it in places where this client would be instantiated internally: like the Auth API client, the Management API client, and the WebAuth client. That has a bigger surface area in terms of diff but is still doable.

  3. Changing that parameter in option 2 above to accept not just numbers but also "objects" could make it receive an AbortSignal that will be passed to fetch instead.

Reference: https://qnp.co.id/blog/adding-timout-fetch-api/

Would you like to send a PR with any of the options above?

@lbalmaceda lbalmaceda added the waiting for customer This issue is waiting for a response from the issue or PR author label Sep 29, 2021
@lbalmaceda lbalmaceda removed their assignment Sep 29, 2021
@mnylen
Copy link
Contributor Author

mnylen commented Sep 29, 2021

@lbalmaceda Ideally I'd like it to be customisable in a way that you could pass your own abort signal (someone might want to cancel the request based on other factors than just timeout) for each request (same signal can't be reused between requests), but it definitely is easier to just implement a global timeout of 10 seconds.

I can take a stab at making a PR for this, although it might take a good while before I have the time for it.

Widcket added a commit that referenced this issue Jan 12, 2022
* Implement timeout support to networking Client

* Change test timeout to 2 seconds

Co-authored-by: Rita Zerrizuela <[email protected]>
@mnylen
Copy link
Contributor Author

mnylen commented Jan 26, 2022

This can be closed now that the timeout support was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request A feature has been asked for or suggested by the community waiting for customer This issue is waiting for a response from the issue or PR author
Projects
None yet
Development

No branches or pull requests

2 participants