Skip to content
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.

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

Issue warning on unclosed AsyncClient. #1197

69 changes: 53 additions & 16 deletions httpx/_client.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import functools
import typing
import warnings
from types import TracebackType

import httpcore
Expand Down Expand Up @@ -79,6 +80,14 @@ def __init__(
self.max_redirects = max_redirects
self._trust_env = trust_env
self._netrc = NetRCInfo()
self._is_closed = True

@property
def is_closed(self) -> bool:
"""
Check if the client being closed
"""
return self._is_closed

@property
def trust_env(self) -> bool:
Expand Down Expand Up @@ -696,6 +705,8 @@ def send(

[0]: /advanced/#request-instances
"""
self._is_closed = False

timeout = self.timeout if isinstance(timeout, UnsetType) else Timeout(timeout)

auth = self._build_request_auth(request, auth)
Expand Down Expand Up @@ -1029,16 +1040,20 @@ def close(self) -> None:
"""
Close transport and proxies.
"""
self._transport.close()
for proxy in self._proxies.values():
if proxy is not None:
proxy.close()
if not self.is_closed:
self._is_closed = True

self._transport.close()
for proxy in self._proxies.values():
if proxy is not None:
proxy.close()

def __enter__(self) -> "Client":
self._transport.__enter__()
for proxy in self._proxies.values():
if proxy is not None:
proxy.__enter__()
self._is_closed = False
return self

def __exit__(
Expand All @@ -1047,10 +1062,16 @@ def __exit__(
exc_value: BaseException = None,
traceback: TracebackType = None,
) -> None:
self._transport.__exit__(exc_type, exc_value, traceback)
for proxy in self._proxies.values():
if proxy is not None:
proxy.__exit__(exc_type, exc_value, traceback)
if not self.is_closed:
self._is_closed = True

self._transport.__exit__(exc_type, exc_value, traceback)
for proxy in self._proxies.values():
if proxy is not None:
proxy.__exit__(exc_type, exc_value, traceback)

def __del__(self) -> None:
self.close()


class AsyncClient(BaseClient):
Expand Down Expand Up @@ -1305,6 +1326,8 @@ async def send(

[0]: /advanced/#request-instances
"""
self._is_closed = False

timeout = self.timeout if isinstance(timeout, UnsetType) else Timeout(timeout)

auth = self._build_request_auth(request, auth)
Expand Down Expand Up @@ -1640,16 +1663,20 @@ async def aclose(self) -> None:
"""
Close transport and proxies.
"""
await self._transport.aclose()
for proxy in self._proxies.values():
if proxy is not None:
await proxy.aclose()
if not self.is_closed:
self._is_closed = True

await self._transport.aclose()
for proxy in self._proxies.values():
if proxy is not None:
await proxy.aclose()

async def __aenter__(self) -> "AsyncClient":
await self._transport.__aenter__()
for proxy in self._proxies.values():
if proxy is not None:
await proxy.__aenter__()
self._is_closed = False
return self

async def __aexit__(
Expand All @@ -1658,10 +1685,20 @@ async def __aexit__(
exc_value: BaseException = None,
traceback: TracebackType = None,
) -> None:
await self._transport.__aexit__(exc_type, exc_value, traceback)
for proxy in self._proxies.values():
if proxy is not None:
await proxy.__aexit__(exc_type, exc_value, traceback)
if not self.is_closed:
self._is_closed = True
await self._transport.__aexit__(exc_type, exc_value, traceback)
for proxy in self._proxies.values():
if proxy is not None:
await proxy.__aexit__(exc_type, exc_value, traceback)

def __del__(self) -> None:
if not self.is_closed:
warnings.warn(
f"Unclosed {self!r}. "
"See https://www.python-httpx.org/async/#opening-and-closing-clients "
"for details."
)
Copy link
Member

Choose a reason for hiding this comment

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

Right, I'm going to suggest that we use UserWarning for this case, for better visibility.

We'll eventually end up tracking individual connections with proper a ResourceWarning against each of those, but this is a good case for a higher-level warning I think.

Also, let's make sure to use a more concise warning format.

Perhaps...

warnings.warn("Unclosed {self!r}. See https://www.python-httpx.org/async/#opening-and-closing-clients for details.")

Then we can add a little more context in the documentation.

What do we think?

Copy link
Member Author

@cdeler cdeler Aug 21, 2020

Choose a reason for hiding this comment

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

Developing libraries/SDKs, I try to avoid to use such warnings as UserWarning. This warning is for users' applications, not for a middle layer, presented by the SDK. Users should be able to separate their warning and httpx warnings.

But I definitely agree with you, that ResourceWarning (unfortunately) is too silent for this case.

Copy link
Member Author

@cdeler cdeler Aug 21, 2020

Choose a reason for hiding this comment

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

@tomchristie
I changed the warning type to UserWarning, then I have to fix a lot of warning in the project tests (it's a separated commit)



class StreamContextManager:
Expand Down
42 changes: 42 additions & 0 deletions tests/client/test_async_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -206,3 +206,45 @@ async def __aexit__(self, *args):
"transport.aclose",
"transport.__aexit__",
]


@pytest.mark.usefixtures("async_environment")
async def test_that_async_client_is_closed_by_default():
client = httpx.AsyncClient()

assert client.is_closed


@pytest.mark.usefixtures("async_environment")
async def test_that_send_cause_async_client_to_be_not_closed():
client = httpx.AsyncClient()

await client.get("http://example.com")

assert not client.is_closed

await client.aclose()


@pytest.mark.usefixtures("async_environment")
async def test_that_async_client_is_not_closed_in_with_block():
async with httpx.AsyncClient() as client:
assert not client.is_closed


@pytest.mark.usefixtures("async_environment")
async def test_that_async_client_is_closed_after_with_block():
async with httpx.AsyncClient() as client:
pass

assert client.is_closed


@pytest.mark.usefixtures("async_environment")
async def test_that_async_client_caused_warning_when_being_deleted():
async_client = httpx.AsyncClient()

await async_client.get("http://example.com")

with pytest.warns(UserWarning):
del async_client
26 changes: 26 additions & 0 deletions tests/client/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -247,3 +247,29 @@ def __exit__(self, *args):
"transport.close",
"transport.__exit__",
]


def test_that_client_is_closed_by_default():
client = httpx.Client()

assert client.is_closed


def test_that_send_cause_client_to_be_not_closed():
client = httpx.Client()

client.get("http://example.com")

assert not client.is_closed


def test_that_client_is_not_closed_in_with_block():
with httpx.Client() as client:
assert not client.is_closed


def test_that_client_is_closed_after_with_block():
with httpx.Client() as client:
pass

assert client.is_closed
6 changes: 4 additions & 2 deletions tests/client/test_proxies.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,16 @@ def test_transport_for_request(url, proxies, expected):
@pytest.mark.asyncio
async def test_async_proxy_close():
try:
client = httpx.AsyncClient(proxies={"all://": PROXY_URL})
client = httpx.AsyncClient(proxies={"https://": PROXY_URL})
await client.get("http://example.com")
finally:
await client.aclose()


def test_sync_proxy_close():
try:
client = httpx.Client(proxies={"all://": PROXY_URL})
client = httpx.Client(proxies={"https://": PROXY_URL})
client.get("http://example.com")
finally:
client.close()

Expand Down