Unify timeout behaviour

[jcugat]

Fixes #433

Took a first stab at it. There are a few tests missing, but wanted to push it early to start getting some feedback.

[jcugat]

Had to put this here instead of the model package due to a circular dependency.

[florimondmanca]

config.py is probably a nicer home for this constant anyway. :)

[jcugat]

Unfortunately can't use this pattern

UnsetType=NewType('UnsetType', object)
UNSET=UnsetType(object())

because NewType does not allow isinstance() on it, and it was needed otherwise mypy was complaining in TimeoutConfig.

[florimondmanca]

Yup, well done here.

From Python 3.8, typing.Literal would allow to solve this quite elegantly:

importtypingUNSET=object()
MyCustomType=typing.Union[bool, typing.Literal[UNSET]]

Not sure that there'll be a backport to 3.6 and 3.7 unfortunately, but just thought it was interesting to share. :)

[jcugat]

It's available in the typing_extensions package https://github.com/python/typing/tree/master/typing_extensions

But I guess it's not worth adding a new dependency just for this (since we have the class workaround anyway).

[Hanaasagi]

@florimondmanca I think typing.Literal is not valid in this usage. UNSET is not a Literal.

[jcugat]

Slight change from the previous code, now we use the provided timeout for acquiring the connection too.

[florimondmanca]

Looking good!

Need to accept that we'll now be having UNSETs floating around everywhere, but overall I think this is on a good path.

I left a couple of comments and suggestions. Would be good to have others' opinions too. :)

[florimondmanca]

Is there any reason (maybe type hints-related?) we can't use elif timeout is UNSET here?

[jcugat]

I tried that first but mypy was complaining, I will reproduce the actual error once I get home, maybe there is something I missed.

[jcugat]

This is the mypy error:

httpx/config.py:262: error: Incompatible types in assignment (expression has type "Union[float, UnsetType]", variable has type "Optional[float]")
httpx/config.py:263: error: Incompatible types in assignment (expression has type "Union[float, UnsetType]", variable has type "Optional[float]")
httpx/config.py:264: error: Incompatible types in assignment (expression has type "Union[float, UnsetType]", variable has type "Optional[float]")

The weird thing is that it complains about these lines:

else:
self.connect_timeout = timeout
self.read_timeout = timeout
self.write_timeout = timeout

[Hanaasagi]

emm, this is because you declare the type of timeout is TimeoutTypes(in __init__ method). It's a Union which includes UnsetType. And you declare the type of self.connect_timeout is typing.Optional[float] in line 241. So if you use elif timeout is UNSET, any instance of UnsetType will enter the else branch.

Ref: https://www.python.org/dev/peps/pep-0484/#support-for-singleton-types-in-unions

[florimondmanca]

So, maybe this is an odd behavior. Shouldn't we expect accessing the timeout config at any point in the chain to return a definite TimeoutConfig object? cc @tomchristie

[florimondmanca]

Ideally, to fully test this we should make a request to the /slow_response endpoint of the server with a long delay (e.g. 100ms), surround the call with a "raise a timeout after x seconds" (where e.g. x = 50ms) instruction, and ensure that the timeout error is raised.

The "raise timeout after x seconds" instruction depends on the underlying async library, but we can add an util in tests/concurrency.py (single-dispatch stuff not listed here):

importasyncioasyncdeftimeout_after(delay, coroutine):
try:
awaitasyncio.wait(coroutine, timeout=delay)
exceptasyncio.TimeoutError:
raiseTimeoutErrorimporttrioasyncdeftimeout_after(delay, coroutine):
asyncwithtrio.move_on_after(delay) ascancel_scope:
awaitcoroutineifcancel_scope.cancelled_caught:
raiseTimeoutError

And use it like this:

asyncdeftest_async_client_timeout_disabled(server, backend):
url=server.url.replace_with(path="/slow_response/100")
asyncwithhttpx.AsyncClient(backend=backend, timeout=None) asclient:
assertclient.dispatch.timeoutisNonewithpytest.raises(TimeoutError):
awaittimeout_after(50e-3, client.get(url))

Or would this be too much?

[jcugat]

I was struggling a bit to change the default timeout of the client (didn't want to have a test that took the full 5s of the default timeout), but I really like this approach. I'll add a few tests with this pattern!

[lovelydinosaur]

Having seen this going all the way through the stack like this I'm a whole lot more cautious about it.
Am less sure now that it's definatly what we want to do.

[jcugat]

@tomchristie your concern is that now we have an UNSET value that is exposed in client.dispatch.timeout (instead of actual timeout values), or the amount of code changes it required?
I'm trying to understand your point of view so I can think about alternatives.

[jcugat]

Is there something else I can do to help push this forward?

[florimondmanca]

@jcugat I remember we discussed in #433 about the option of using timeout=False to disable timeouts. I explore this possibility in #490, feel free to take a look over there. :)

[lovelydinosaur]

Thanks for having put a bunch of time into this!

It's bit out of sync now, but a good basis for figure out what we need.
Here's what I think...

• Drop DEFAULT_TIMEOUT_CONFIG from config.py.
• Instead declare a DEFAULT_TIMEOUT = 5 in client.py (Closer to the source where it's used is better.)
• Declare an UNSET = object() in client.py
• Use timeout = DEFAULT_TIMEOUT on the Client.__init__, and on the base API methods.
• Use timeout = UNSET on Client.request, Client.get & pals, and Client.send()
• Don't pass timeout=... to the connection pool on __init__
• .send() should use timeout = self.timeout if timeout is UNSET else timeout to select either the client timeout, or the explicitly passed override timeout.

  httpx/httpx/client.py

  Line 345 in 3f68601

  trust_env=self.trust_enviftrust_envisNoneelsetrust_env

• Everywhere else (eg. the dispatch API) takes an optional timeout: TimeoutConfig() = None argument, and just uses timeout = TimeoutConfig() if timeout is None else timeout. It won't need to select between self.timeout and timeout anymore, because that decision has been taken at a client level instead.

[lovelydinosaur]

Superseeded by #592 - Thanks so much for your time on this, it's been really helpful in getting it squared away!