Skip to content

Add default timeout - #209

Closed
Sytten wants to merge 1 commit into
auth0:masterfrom
Sytten:master
Closed

Sytten wants to merge 1 commit into
auth0:masterfrom
Sytten:master

Conversation

@Sytten

@Sytten Sytten commented Sep 9, 2019

Copy link
Copy Markdown
Contributor

Changes

This PR is an updated version of #148.
I decided for 5s, because 30s seems excessive to me (especially in the context of a sync web server), feel free to disagree.

References

Having no timeout is never a good idea in prod. Auth0 might have issues and we don't want to hang our python server because of that.

Testing

  • This change adds unit test coverage
  • This change has been tested on the latest version

Checklist

@Sytten
Sytten requested a review from a team September 9, 2019 01:56
@Sytten

Sytten commented Sep 9, 2019

Copy link
Copy Markdown
Contributor Author

@lbalmaceda this ready to merge, please don't let it rot :)

@damieng

damieng commented Sep 26, 2019

Copy link
Copy Markdown
Contributor

@joshcanhelp A lot of this addresses things you requested in #148 - can you take a look?

@damieng

damieng commented Sep 26, 2019

Copy link
Copy Markdown
Contributor

Fixes #82

@joshcanhelp joshcanhelp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Sytten - This looks great so far, thank you for putting in the effort on this!

Can you put the default timeout in a constant so that value is only stored in a single place?

@joshcanhelp

Copy link
Copy Markdown
Contributor

@lbalmaceda - Can you give this a quick once-over to make sure we're not missing anything?

@lbalmaceda

Copy link
Copy Markdown
Contributor

@Sytten Thanks for raising this. We have already in our backlog a task to improve the customization of the HTTP client, but in a generic way.

Moving forward, this library should not be receiving a new parameter for each new configuration that the HTTP client requires to be set. Ideally a single options object can hold them all and be extended in the future without the need to change the rest of the method signatures.

What we would like is this configuration object to have for now 2 properties:

  • connect_timeout
  • read_timeout

These would later be set, when present, using this tuple.

Regarding the defaults I don't have a preference. I understand it should be a value close to 10 secs. Don't forget to move it to a constant somewhere it can be referenced from the multiple places you are using it.

Finally, please improve the tests. I rather them set a real world timeout value in seconds vs milliseconds. You should be able to "wait" for it to timeout vs depending on how fast the test is being run. What current tests are missing:

  • Test the base or rest classes, use a value like 5 seconds (different from the default value)
  • Add a test case for when the user doesn't set the timeout value and the default is used instead
  • Add test cases for each of the entity classes (e.g. tenant) that are passing the options down to the base / rest class; ensure the base http client is set up with the correct options.

If you agree with all, we're happy to review the changes again after you submit them.

Cheers

@stale

stale Bot commented Jan 6, 2020

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️

@stale stale Bot added the closed:stale Issue or PR has not seen activity recently label Jan 6, 2020
@Sytten

Sytten commented Jan 7, 2020

Copy link
Copy Markdown
Contributor Author

I will finish this PR later this week

@stale stale Bot removed the closed:stale Issue or PR has not seen activity recently label Jan 7, 2020
@stevehobbsdev

Copy link
Copy Markdown
Contributor

@Sytten Please let us know if this is something that you are still interested in working on, will be happy to review the changes when they come in.

@Sytten

Sytten commented Feb 6, 2020

Copy link
Copy Markdown
Contributor Author

@stevehobbsdev Sorry I didn't have much time to work on open source and multiple projects, if its a priority to you I would say to go ahead and do it, otherwise I will eventually finish it but I don't have a timeline.

@stale

stale Bot commented May 6, 2020

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️

@stale stale Bot added the closed:stale Issue or PR has not seen activity recently label May 6, 2020
@Sytten

Sytten commented May 6, 2020

Copy link
Copy Markdown
Contributor Author

Not stale, but no progress either

@stale stale Bot removed the closed:stale Issue or PR has not seen activity recently label May 6, 2020
@lbalmaceda

Copy link
Copy Markdown
Contributor

Thanks for the update @Sytten. We'll see if we can fit this into our backlog in the coming weeks.

@lbalmaceda lbalmaceda added the feature request A feature has been asked for or suggested by the community label May 6, 2020
@lbalmaceda lbalmaceda mentioned this pull request May 7, 2020
5 of 7 tasks
@lbalmaceda

Copy link
Copy Markdown
Contributor

👋 we're closing this PR in favor of the linked one. We appreciate your contribution.

@lbalmaceda lbalmaceda closed this May 7, 2020
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants