Skip to content

Unify timeout behaviour#463

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.

Sign up for GitHub

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

Closed
jcugat wants to merge 5 commits into from
Closed

Unify timeout behaviour #463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ae8fa56
Unify timeout behaviour for top-level API methods
jcugat Oct 8, 2019
aab0b21
Unify timeout behaviour for clients and dispatchers
jcugat Oct 8, 2019
1248837
Use default timeout values in TimeoutConfig init
jcugat Oct 10, 2019
edf56f2
Revert connection change
jcugat Oct 10, 2019
a68c82f
Use keyword arguments in acquire_connection
jcugat Oct 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions httpx/api.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
import typing

from .client import Client
from .config import CertTypes, TimeoutTypes, VerifyTypes
from .config import UNSET, CertTypes, TimeoutTypes, VerifyTypes
from .models import (
AuthTypes,
CookieTypes,
Expand All@@ -26,7 +26,7 @@ def request(
headers: HeaderTypes = None,
cookies: CookieTypes = None,
auth: AuthTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = True,
Expand DownExpand Up@@ -65,7 +65,7 @@ def get(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = True,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
proxies: ProxiesTypes = None,
) -> Response:
Expand DownExpand Up@@ -96,7 +96,7 @@ def options(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = True,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
proxies: ProxiesTypes = None,
) -> Response:
Expand DownExpand Up@@ -127,7 +127,7 @@ def head(
allow_redirects: bool = False, # Note: Differs to usual default.
cert: CertTypes = None,
verify: VerifyTypes = True,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
proxies: ProxiesTypes = None,
) -> Response:
Expand DownExpand Up@@ -161,7 +161,7 @@ def post(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = True,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
proxies: ProxiesTypes = None,
) -> Response:
Expand DownExpand Up@@ -198,7 +198,7 @@ def put(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = True,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
proxies: ProxiesTypes = None,
) -> Response:
Expand DownExpand Up@@ -235,7 +235,7 @@ def patch(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = True,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
proxies: ProxiesTypes = None,
) -> Response:
Expand DownExpand Up@@ -269,7 +269,7 @@ def delete(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = True,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
proxies: ProxiesTypes = None,
) -> Response:
Expand Down
42 changes: 21 additions & 21 deletions httpx/client.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -10,7 +10,7 @@
from .config import (
DEFAULT_MAX_REDIRECTS,
DEFAULT_POOL_LIMITS,
DEFAULT_TIMEOUT_CONFIG,
UNSET,
CertTypes,
HTTPVersionTypes,
PoolLimits,
Expand DownExpand Up@@ -63,7 +63,7 @@ def __init__(
cert: CertTypes = None,
http_versions: HTTPVersionTypes = None,
proxies: ProxiesTypes = None,
timeout: TimeoutTypes = DEFAULT_TIMEOUT_CONFIG,
timeout: TimeoutTypes = UNSET,
pool_limits: PoolLimits = DEFAULT_POOL_LIMITS,
max_redirects: int = DEFAULT_MAX_REDIRECTS,
base_url: URLTypes = None,
Expand DownExpand Up@@ -201,7 +201,7 @@ async def _get_response(
allow_redirects: bool = True,
verify: VerifyTypes = None,
cert: CertTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> AsyncResponse:
if request.url.scheme not in ("http", "https"):
Expand DownExpand Up@@ -350,7 +350,7 @@ async def get(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> AsyncResponse:
return await self.request(
Expand DownExpand Up@@ -380,7 +380,7 @@ async def options(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> AsyncResponse:
return await self.request(
Expand DownExpand Up@@ -410,7 +410,7 @@ async def head(
allow_redirects: bool = False, # NOTE: Differs to usual default.
cert: CertTypes = None,
verify: VerifyTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> AsyncResponse:
return await self.request(
Expand DownExpand Up@@ -443,7 +443,7 @@ async def post(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> AsyncResponse:
return await self.request(
Expand DownExpand Up@@ -479,7 +479,7 @@ async def put(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> AsyncResponse:
return await self.request(
Expand DownExpand Up@@ -515,7 +515,7 @@ async def patch(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> AsyncResponse:
return await self.request(
Expand DownExpand Up@@ -548,7 +548,7 @@ async def delete(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> AsyncResponse:
return await self.request(
Expand DownExpand Up@@ -582,7 +582,7 @@ async def request(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> AsyncResponse:
request = self.build_request(
Expand DownExpand Up@@ -616,7 +616,7 @@ async def send(
allow_redirects: bool = True,
verify: VerifyTypes = None,
cert: CertTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> AsyncResponse:
return await self._get_response(
Expand DownExpand Up@@ -704,7 +704,7 @@ def request(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> Response:
request = self.build_request(
Expand DownExpand Up@@ -737,7 +737,7 @@ def send(
allow_redirects: bool = True,
verify: VerifyTypes = None,
cert: CertTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> Response:
concurrency_backend = self.concurrency_backend
Expand DownExpand Up@@ -794,7 +794,7 @@ def get(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> Response:
return self.request(
Expand DownExpand Up@@ -824,7 +824,7 @@ def options(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> Response:
return self.request(
Expand DownExpand Up@@ -854,7 +854,7 @@ def head(
allow_redirects: bool = False, # NOTE: Differs to usual default.
cert: CertTypes = None,
verify: VerifyTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> Response:
return self.request(
Expand DownExpand Up@@ -887,7 +887,7 @@ def post(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> Response:
return self.request(
Expand DownExpand Up@@ -923,7 +923,7 @@ def put(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> Response:
return self.request(
Expand DownExpand Up@@ -959,7 +959,7 @@ def patch(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> Response:
return self.request(
Expand DownExpand Up@@ -992,7 +992,7 @@ def delete(
allow_redirects: bool = True,
cert: CertTypes = None,
verify: VerifyTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
trust_env: bool = None,
) -> Response:
return self.request(
Expand Down
45 changes: 35 additions & 10 deletions httpx/config.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -8,9 +8,18 @@
from .__version__ import __version__
from .utils import get_ca_bundle_from_env, get_logger


class UnsetType:

@jcugatjcugatOct 8, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@florimondmancaflorimondmancaOct 8, 2019

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.

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. :)

@jcugatjcugatOct 10, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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).

@HanaasagiHanaasagiOct 17, 2019

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.

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

pass


UNSET = UnsetType()

@jcugatjcugatOct 8, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

@florimondmancaflorimondmancaOct 8, 2019

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.

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


CertTypes = typing.Union[str, typing.Tuple[str, str], typing.Tuple[str, str, str]]
VerifyTypes = typing.Union[str, bool, ssl.SSLContext]
TimeoutTypes = typing.Union[float, typing.Tuple[float, float, float], "TimeoutConfig"]
TimeoutTypes = typing.Union[
float, typing.Tuple[float, float, float], "TimeoutConfig", UnsetType
]
HTTPVersionTypes = typing.Union[
str, typing.List[str], typing.Tuple[str], "HTTPVersionConfig"
]
Expand DownExpand Up@@ -224,23 +233,39 @@ def __init__(
self,
timeout: TimeoutTypes = None,
*,
connect_timeout: float = None,
read_timeout: float = None,
write_timeout: float = None,
connect_timeout: typing.Union[float, UnsetType, None] = UNSET,
read_timeout: typing.Union[float, UnsetType, None] = UNSET,
write_timeout: typing.Union[float, UnsetType, None] = UNSET,
):
if timeout is None:
self.connect_timeout = connect_timeout
self.read_timeout = read_timeout
self.write_timeout = write_timeout
self.connect_timeout: typing.Optional[float] = (
connect_timeout
if not isinstance(connect_timeout, UnsetType)
else DEFAULT_TIMEOUT_CONFIG.connect_timeout
)
self.read_timeout: typing.Optional[float] = (
read_timeout
if not isinstance(read_timeout, UnsetType)
else DEFAULT_TIMEOUT_CONFIG.read_timeout
)
self.write_timeout: typing.Optional[float] = (
write_timeout
if not isinstance(write_timeout, UnsetType)
else DEFAULT_TIMEOUT_CONFIG.write_timeout
)
else:
# Specified as a single timeout value
assert connect_timeout is None
assert read_timeout is None
assert write_timeout is None
assert connect_timeout is UNSET
assert read_timeout is UNSET
assert write_timeout is UNSET
if isinstance(timeout, TimeoutConfig):
self.connect_timeout = timeout.connect_timeout
self.read_timeout = timeout.read_timeout
self.write_timeout = timeout.write_timeout
Comment thread
florimondmanca marked this conversation as resolved.
elif isinstance(timeout, UnsetType):

@florimondmancaflorimondmancaOct 8, 2019

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.

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

@jcugatjcugatOct 9, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

@jcugatjcugatOct 10, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

@HanaasagiHanaasagiOct 17, 2019•
edited
Loading

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.

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

self.connect_timeout = DEFAULT_TIMEOUT_CONFIG.connect_timeout
self.read_timeout = DEFAULT_TIMEOUT_CONFIG.read_timeout
self.write_timeout = DEFAULT_TIMEOUT_CONFIG.write_timeout
elif isinstance(timeout, tuple):
self.connect_timeout = timeout[0]
self.read_timeout = timeout[1]
Expand Down
4 changes: 2 additions & 2 deletions httpx/dispatch/asgi.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2,7 +2,7 @@

from ..concurrency.asyncio import AsyncioBackend
from ..concurrency.base import ConcurrencyBackend
from ..config import CertTypes, TimeoutTypes, VerifyTypes
from ..config import UNSET, CertTypes, TimeoutTypes, VerifyTypes
from ..models import AsyncRequest, AsyncResponse
from ..utils import MessageLoggerASGIMiddleware, get_logger
from .base import AsyncDispatcher
Expand DownExpand Up@@ -65,7 +65,7 @@ async def send(
request: AsyncRequest,
verify: VerifyTypes = None,
cert: CertTypes = None,
timeout: TimeoutTypes = None,
timeout: TimeoutTypes = UNSET,
) -> AsyncResponse:

scope = {
Expand Down
Loading