Client timeout refactoring

[asvetlov]

There are many different timeout strategies for waiting a response from client:

• uber timeout for total elapsed seconds
• timeout for waiting for available connection from a pool
• connection timeout for waiting a response from a new connection to server
• timeout for getting HTTP headers from response
• timeout for every HTTP body chunk read
• maybe you can add your case

As the solution I propose collapsing all timeouts into the single timeout class.
E.g. Timeout.after(30) is an uber timeout, Timeout.await_pool(5) is a timeout for waiting for connection pool. Timeout.after(30).await_pool(5) composes both.

Do you like the idea? Objections?

[samuelcolvin]

I agree the current situation is confusing, especially for beginners. Maybe keyword arguments would be more canonical than chaining?

…

On 26 Feb 2018 19:08, "Andrew Svetlov" ***@***.***> wrote: There are many different timeout strategies for waiting a response from client: - uber timeout for total elapsed seconds - timeout for waiting for available connection from a pool - connection timeout for waiting a response from a new connection to server - timeout for getting HTTP headers from response - timeout for every HTTP body chunk read - maybe you can add your case As the solution I propose collapsing all timeouts into the single *timeout* class. E.g. Timeout.after(30) is an uber timeout, Timeout.await_pool(5) is a timeout for waiting for connection pool. Timeout.after(30).await_pool(5) composes both. Do you like the idea? Objections? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2768>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD2jGaeXk6Kx62Bsb4JuE8EXW46YAR1oks5tYwBjgaJpZM4STwZi> .

[asvetlov]

Are you talking about keyword arguments in Timeout constructor? I don't know, let's see how things will go.
What I definitely don't want is adding all these settings as keyword arguments to client API. Should be the only timeout which contains all required information internally.

[hubo1016]

When a number is given, It should be converted to uber timeout.

Keyword-arguments for constructor may be better. Though chaining is cool, for the real usage, many applications may use configuration dict to store the settings, so keyword arguments would be more convient.

Timeout object should work like dict, so a request timeout object can be merged into the default settings.

[asvetlov]

No. Disabling a waiting for available connection is preferable in many cases, aiobotocore wants it.
@thehesiod please confirm if I understand your needs properly.
It means that uber timeout is not always applicable. Different timeout types are not always composable, sometimes they are mutually exclusive.
Dict-like idea just doesn't work.

[hubo1016]

Maybe use False or a specified constant for "explicit disable"? Allow inheriting from default is always useful I think.

[asvetlov]

I'm totally missed what default are you talking about and what inheriting means in this context?

[hubo1016]

I think there should be a default timeout settings on the session? And for each request, a different timeout can be specified.

[asvetlov]

Yes. Like now a client session has default timeouts which could be overridden by request.
But I want to shrink these values into opaque timeout object that could be passed both into ClientSession constructor and request methods.

[thehesiod]

another possible timeout:

• DNS resolution timeout (typically tied into "connect" timeout)

I'm not sure if the "send" of the headers blocks, if so I guess that could be a timeout, but more of a "write" timeout.

aiobotocore want's "normal" timeouts, that is timeouts should encapsulate actual network lag. So:

• connect timeout is time to connect to endpoint (includes DNS resolve)
• read timeout is waiting for any data from an endpoint

[pfreixes]

Definitely, this is something that has appeared many times and needs to be tackled sooner than later. My 2cents on that:

• Use kw arguments in the constructor is gonna pollute the ClientSession signature and/or the requests methods, taking into account the current timeouts and future timeouts that might be implemented. I will go for a specific class object.

• How are we gonna deal with backward compatibility?

Will be appreciated a snippet showing the use case.

Regarding the

timeout for waiting for available connection from a pool

We could give a new option that would allow the developer to get notified via an Exception if and only if all connections are busy because the limit of the pool was reached.

[thehesiod]

@pfreixes ya I think there are two categories, those that should be specifiable, and those that are traceable. For now we're only interested in tracing timeout for waiting for available connection from a pool.

[hubo1016]

@asvetlov When we choose a timeout object for the session which contains uber timeout and connect timeout, and then start a request with a timeout object that cancels the uber timeout, usually we still want to keep the connect timeout. So timeout object should be able to merge.

I suggest that the timeout object has following interfaces:

0. timeout object is always immutable. We may consider implement it from NamedTuple.
1. Timeout(uber_timeout = None, *, connect_timeout = None, ...) constructor. Only the uber timeout can be specified as positional arguments, all other arguments are keyword-only. The default values are None. Accepted values are None / False / . None means "use default", False means "force disable", other numbers are timeout values.
2. copy(**kwargs) Copy an existing timeout object and update the timeout values. Equivalent to convert the timeout to a dict, merge the dict with kwargs and use the dict to call Timeout(**args).
3. merge(timeout) Copy an existing timeout object and merge the settings from another timeout object. Only False/ are merged, None values are ignored. Equivalent to .copy(**timeout.todict())
4. todict() Convert the timeout object to a dict containing settings that are not None. Can be used to implement repr and for logging.

NOTICE: copy(**kwargs) is different from merge(Timeout(**kwargs)). Timeout(connect_timeout=1).copy(connect_timeout=None) returns a timeout object with connect_timeout=None, but Timeout(connect_timeout=1).merge(Timeout(connect_timeout=None)) returns a timeout object with connect_timeout=1.

[asvetlov]

I dislike timeout merging and dict exposing, it brings unnecessary complexity.

[asvetlov]

@pfreixes I don't see backward compatibility problems. For transition period a timeout is constructed from old-styled parameters if not provided explicitly.

[asvetlov]

@thehesiod tracing is very reach word with many contexts.
What do you mean be tracing timeout for waiting for available connection from a pool?