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

#409 Implement timeout support to networking Client #423

Merged
merged 4 commits into from
Jan 12, 2022

Conversation

mnylen
Copy link
Contributor

@mnylen mnylen commented Oct 30, 2021

Changes

  • Added timeout option to Auth0 that is passed to networking.Client (defaults to 10 seconds)
  • Requests made with networking.Client timeout after the specified timeout, and any ongoing requests are cancelled using AbortController created for each request.
  • Exports TimeoutError

References

Please include relevant links supporting this change such as a:

Testing

Can be tested in e.g. iOS Simulator using Network Link Condition preferences pane. Setting the packet drop ratio to 100 % will cause the requests to never complete. With this change, TimeoutError will be thrown after attempting the request for 10 seconds.

  • This change adds unit test coverage
  • This change has been tested on the latest version of the platform/language or why not.

Checklist

@mnylen mnylen requested a review from a team as a code owner October 30, 2021 07:23
@Widcket
Copy link
Contributor

Widcket commented Nov 18, 2021

Hi @mnylen, thanks for raising this PR. I haven't been able to look at it yet, but might get to it by the end of the week.

return new Promise(resolve => {
responseTimerId = setTimeout(() => {
resolve(response);
}, 10000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10 secs is a bit much for a test timeout, please lower it to 2 or 3 seconds.

Copy link
Contributor

@Widcket Widcket left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just have a nitpick over the timeout value for the test case. Thank you for the PR, we really appreciate you taking the time for doing this.

@mnylen
Copy link
Contributor Author

mnylen commented Dec 27, 2021

@Widcket thanks for the feedback. I changed the test timeout. Sorry it took a while. Switched companies and not in a project that uses this anymore and don't have the setup locally on my computer anymore either, but hopefully others can find this change useful. :)

@Widcket
Copy link
Contributor

Widcket commented Jan 7, 2022

Thanks for coming back and pushing the change!

@Widcket Widcket merged commit 6a9d257 into auth0:master Jan 12, 2022
@poovamraj poovamraj mentioned this pull request Jan 20, 2022
@poovamraj poovamraj mentioned this pull request Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants