add default timeout on requests

[Chovin]

to avoid hanging indefinitely as brought up by #82

I chose 30 seconds because of this comment on the requests repo.

I'd like to try my hand at contributing to auth0 but I'm not quite sure where to go from here in regards to this PR.

On the timeout value,

1. should I add it as a config somehow,
2. put it in as an optional parameter everywhere,
3. or leave it as is?

On the error thrown when timing out,

1. should I catch the error and construct a fake 408 (Request Timeout) response,
2. wrap requests' timeout error and toss that back, updating documentation as needed,
3. or leave it as is?

Any feedback would be greatly appreciated!

[joshcanhelp]

@Chovin - Thanks so much for the contribution here!

I think you're on the right track here with adding this timeout (requests documentation). I also think making this configurable for the developer (a config entry for this SDK) is also a good idea so folks can override that if needed.

For the error handling ... if we're not doing any catching/modifying, I would just pass that back as-is. Since we're not doing that now (defaults to hanging), I wouldn't consider that a breaking change. Also from the docs:

If a request times out, a Timeout exception is raised.

It looks like all errors are caught and wrapped in an Auth0Error here:

https://github.com/auth0/auth0-python/blob/master/auth0/v3/authentication/base.py#L37

... so I would stick with that standard.

I'm curious, though, why a timeout was added to a single mock request. Is that making an HTTP call?

[Chovin]

@joshcanhelp thanks so much for the feedback!

I'll take a look at making it configurable. I couldn't find where this is done currently, but I'll keep looking. I may need to ask for some more help if I'm not able to find it if that's alright 😁

as for the error, I'm pretty sure it doesn't get wrapped into an Auth0Error. I grabbed that AuthenticationBase class and tried it with a very low timeout. From my testing, the post/get throws an error before returning a response. That's ok right? It seems like there might also be other errors also that aren't wrapped by Auth0Error https://stackoverflow.com/a/16511493

for the mock request, it was asserting that post was called with the old list of parameters. It was complaining that now there was a new parameter, timeout. You can see the error here

[joshcanhelp]

I may need to ask for some more help if I'm not able to find it if that's alright

No problem at all 😄 appreciate the effort here. I'll see if I can get some guidance on that for you.

From my testing, the post/get throws an error before returning a response. That's ok right?

Looks like you're right and, in that case, I think it's ok to not just wrap this one. If it's all part of the same set of errors then I think that's OK to pass right through for handling. Right now it just fails "silently" so I don't see this as breaking.

[joshcanhelp]

A few more things to go on here ... the constructor that needs to accept this (with a default fallback) is here:

https://github.com/auth0/auth0-python/blob/master/auth0/v3/management/rest.py#L16

Developers who want to customize it would need to manually require that particular “entity” file e.g. doing Blacklist(domain, token, telemetry, timeout …). OR alternatively, you’d need to modify the Auth0 file to make it pass a timeout value (with a default value):

https://github.com/auth0/auth0-python/blob/master/auth0/v3/management/auth0.py#L21

Hope that helps!

[joshcanhelp]

@Chovin - I still think this would be great to have in this SDK. Do you need anymore guidance here?

[Sytten]

This needs to be merged, we hit the same problem in production where the thread just hanged.

[damieng]

Superceeded by #209