Issue warning on unclosed `AsyncClient`. #1197

cdeler · 2020-08-19T09:51:30Z

As a part of #871 it has been decided to prevent httpx.Client and httpx.AsyncClient from sending requests after being closed.

Closes #871

tomchristie

Thanks! ✨😀

Seems simple on the surface, but there's a few bits & pieces to work through.

In particular I'd prefer we don't use the decorator style, and prefer a plain approach instead.
Direct, simple, easier to reason through, neater tracebacks.
I've included some comments to help work through what I think could be tweaked.

tomchristie · 2020-08-19T11:14:03Z

httpx/_client.py

@@ -56,6 +56,18 @@
 KEEPALIVE_EXPIRY = 5.0


+def check_not_closed(method: typing.Callable) -> typing.Callable:


I'd prefer for us not to use a decorator style.
I'll use inline comments below to walk through the alternative...

I've removed the decorator. Simple is better than complex.

tomchristie · 2020-08-19T11:15:13Z

httpx/_client.py

@@ -619,6 +639,7 @@ def _transport_for_url(self, url: URL) -> httpcore.SyncHTTPTransport:

        return self._transport

+    @check_not_closed


I guess we should instead put the check around def send() rather than def request().

Rather than the decorator, I'd prefer the plainer style of starting the method with...

if self._is_closed: raise RuntimeError("Cannot send requests on a closed client instance.")

I guess we should instead put the check around def send() rather than def request().

Done

Rather than the decorator, I'd prefer the plainer style of starting the method with...

Done, but I used self.is_closed in the condition

tomchristie · 2020-08-19T11:17:56Z

httpx/_client.py

@@ -1015,6 +1036,7 @@ def delete(
            timeout=timeout,
        )

+    @check_not_closed


Could we instead just let things pass silently if the client is already closed, and .close() is called again?
So...

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

tomchristie · 2020-08-19T11:19:04Z

httpx/_client.py

@@ -1628,6 +1654,8 @@ async def aclose(self) -> None:
            if proxy is not None:
                await proxy.aclose()

+        self._is_closed = True


We should put self._is_closed = True as the very first line, rather than the very last line, since that'd handle potential race conditions more cleanly.

@tomchristie It a bit confuses me, since there should be another ways to avoid race conditions, e.g. locks.

I saw several same places where race condition is possible

I can try to write a test, which strictly cause a race condition here (if it's possible)

cdeler · 2020-08-19T14:58:30Z

@tomchristie

I wrote the test, which shows the race condition. I can fix it (but I'd like to do it in separate PR since it involves things such as threading.Lock().

Here is the test:

class SlowTestTransport(httpcore.SyncHTTPTransport):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.closing_counter = 0

    def request(
        self,
        method: bytes,
        url: URL,
        headers: Headers = None,
        stream: SyncByteStream = None,
        timeout: TimeoutDict = None,
    ) -> Tuple[bytes, int, bytes, List[Tuple[bytes, bytes]], SyncByteStream]:
        return b"", 0, b"", [(b"", b"")], ByteStream(b"Hello")

    def close(self) -> None:
        self.closing_counter += 1


class SlowClient(httpx.Client):
    @property
    def is_closed(self) -> bool:
        result = super().is_closed

        time.sleep(0.5)

        return result


def test_that_client_handles_race_condition_while_being_closed():
    slow_transport = SlowTestTransport()
    client = SlowClient(transport=slow_transport)

    futures = []
    with ThreadPoolExecutor(max_workers=2) as executor:
        futures.append(executor.submit(client.close))
        futures.append(executor.submit(client.close))

    for _ in concurrent.futures.as_completed(futures):
        pass

    assert slow_transport.closing_counter == 1

tomchristie · 2020-08-19T15:17:02Z

Let's not get into that level of testing at the moment. Yes you can potentially do silly things if you're concurrently closing clients and issuing requests at the same time, but we don't necessarily need to protect users from themselves at quite that level of granularity.

So, working through this has actually made it more clear to me exactly what behaviour we do want here...

Clients should start in ._is_closed = True
Calling into .send() or calling into either __enter__/__aenter__ should set ._is_closed = False
Calling into .close()/.aclose() or calling into either __exit__/__aexit__ should set ._is_closed = True
The Client.__del__ method should check ._is_closed and call .close() if needed.
The AsyncClient.__del__ method should check ._is_closed and warn if needed.
Making a request with a closed client is fine. It just implicitly opens the client.

That's because we don't actually want simply importing this...

client = httpx.AsyncClient()

To raise a warning. Which if we treat the client as open simply by instantiating it, we would.

The client is only opened once you're either using...

async with client: - Enter the context on an existing instance.
async with httpx.AsyncClient() as client: - Enter the context, creating an instance.
await client.request() - Implicitly opening the client, by issuing a request. Not recommended for async, where we advise using the client in a context manager, but perfectly common with httpx.Client().

cdeler · 2020-08-19T15:36:43Z

So, working through this has actually made it more clear to me exactly what behaviour we do want here...
...

Nice, then I'm going to implement that (in this PR)

cdeler · 2020-08-20T10:07:17Z

@tomchristie I've done with the changes you mentioned.

Calling into .close()/.aclose() or calling into either exit/aexit should set ._is_closed = True

I prevent the clients from being double-closed:

class AsyncClient(BaseClient):
    # ...
    async def aclose(self) -> None:
        """
        Close transport and proxies.
        """
        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()

Am I right, or I should remove this check?

tomchristie · 2020-08-20T15:37:48Z

So I tried this locally, and it took me a while to figure out that I wasn't seeing anything because ResourceWarning is ignored by default. I'm not sure what we think about that?

We could issue it as a UserWarning instead, perhaps?
Or a custom subclass?
Or perhaps it's okay as a mostly silent ResourceWarning and we're okay with that?

cdeler · 2020-08-20T15:52:57Z

@tomchristie yes, I didn't pay attention to the fact, that ResourceWarning is silent by default. May be the RuntimeWarning (it's visible)?

exception RuntimeWarning¶
Base class for warnings about dubious runtime behavior.

(c) python language reference (link)

Update
The requests triggers ResourceWarning in this case, for example requests/api.py#L58

tomchristie · 2020-08-21T11:41:17Z

httpx/_client.py

+                "\t>>> async with httpx.AsyncClient() as client:\n"
+                "\t>>>     ...",
+                ResourceWarning,
+            )


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?

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.

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

tomchristie · 2020-08-26T09:32:53Z

@cdeler - Thanks for all your fantastic work on this.

I think it probably is worth issuing a PR just dealing with the "close async clients properly in our test cases" first, since we can pull that one into the master branch, wheras this one we need to keep holding onto until we're ready to issue a new major release. Doing it that way around would help us avoid possibly messy merge conflicts, and would give us two independently reviewable PRs.

cdeler · 2020-08-26T09:34:14Z

I think it probably is worth issuing a PR just dealing with the "close async clients properly in our test cases" first

Yes, sure, I'm to create this "close async clients properly in our test cases" PR today

Update: I created the PR #1219

…ode#871)

…ich is too quiet) to UserWarning (encode#871)

…erge (encode#871)

cdeler · 2020-08-31T15:55:01Z

@tomchristie

According to your comment, I rebased this branch to the master after #1219 .

The only thing which is not done is

Tackle any cleanup in test cases, and always prefer just using httpx.Client unless there's a good reason.

…_when_its_closed

tomchristie

Great stuff, let's get this in now! 👍

tomchristie reviewed Aug 19, 2020

View reviewed changes

cdeler force-pushed the prevent_async_client_from_sending_requests_when_its_closed branch from 350ae77 to 5e2197c Compare August 19, 2020 11:23

cdeler force-pushed the prevent_async_client_from_sending_requests_when_its_closed branch from e56a711 to 7eff422 Compare August 20, 2020 09:57

tomchristie changed the title ~~Prevent Client and AsyncClient from sending any requests when they have been closed (#871)~~ Issue warning on unclosed AsyncClient. Aug 20, 2020

tomchristie added the user-experience Ensuring that users have a good experience using the library label Aug 20, 2020

tomchristie modified the milestone: v1.0 Aug 20, 2020

tomchristie reviewed Aug 21, 2020

View reviewed changes

cdeler force-pushed the prevent_async_client_from_sending_requests_when_its_closed branch 3 times, most recently from 458e6c9 to 9f88f8f Compare August 21, 2020 14:37

cdeler mentioned this pull request Aug 26, 2020

Closing AsyncClient in all tests (#871) #1219

Merged

cdeler added 3 commits August 31, 2020 18:12

Made Client and AsyncClient checking for being closed in __del__ (enc…

987b64a

…ode#871)

Changed the AsyncClient closing warning type from ResourceWarning (wh…

22f663a

…ich is too quiet) to UserWarning (encode#871)

Fixed tests and client's __exit__ and __aexit__ after the difficult m…

8d85e87

…erge (encode#871)

cdeler force-pushed the prevent_async_client_from_sending_requests_when_its_closed branch from e898c07 to 8d85e87 Compare August 31, 2020 15:46

cdeler requested a review from tomchristie September 2, 2020 09:15

Merge branch 'master' into prevent_async_client_from_sending_requests…

44ba0b2

…_when_its_closed

tomchristie mentioned this pull request Sep 2, 2020

Version 0.14.3 #1247

Merged

tomchristie added 2 commits September 2, 2020 12:35

Merge branch 'master' into prevent_async_client_from_sending_requests…

0fd965a

…_when_its_closed

Update test_proxies.py

d6e9be0

Update test_proxies.py

c01e788

tomchristie approved these changes Sep 2, 2020

View reviewed changes

tomchristie merged commit def9f1c into encode:master Sep 2, 2020

cdeler deleted the prevent_async_client_from_sending_requests_when_its_closed branch September 4, 2020 18:12

tomchristie mentioned this pull request Sep 21, 2020

Version 0.15.0 #1301

Merged

4 tasks

florimondmanca mentioned this pull request Sep 24, 2020

Client.close() does not close transports if unused #1317

Closed

geofft mentioned this pull request Sep 28, 2020

Unintuitive behavior with using an AsyncClient after it's been aclosed #1332

Closed

2 tasks

bibajz mentioned this pull request Feb 5, 2021

Unclosed httpx.AsyncClient warning bibajz/bitcoin-python-async-rpc#6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue warning on unclosed `AsyncClient`. #1197

Issue warning on unclosed `AsyncClient`. #1197

cdeler commented Aug 19, 2020 •

edited by tomchristie

Loading

tomchristie left a comment

tomchristie Aug 19, 2020

cdeler Aug 19, 2020

tomchristie Aug 19, 2020

tomchristie Aug 19, 2020

cdeler Aug 19, 2020

tomchristie Aug 19, 2020

tomchristie Aug 19, 2020

cdeler Aug 19, 2020

cdeler Aug 19, 2020 •

edited

Loading

cdeler commented Aug 19, 2020

tomchristie commented Aug 19, 2020

cdeler commented Aug 19, 2020

cdeler commented Aug 20, 2020 •

edited

Loading

tomchristie commented Aug 20, 2020

cdeler commented Aug 20, 2020 •

edited

Loading

tomchristie Aug 21, 2020

cdeler Aug 21, 2020 •

edited

Loading

cdeler Aug 21, 2020 •

edited

Loading

tomchristie commented Aug 26, 2020

cdeler commented Aug 26, 2020 •

edited

Loading

cdeler commented Aug 31, 2020

tomchristie left a comment

		@@ -56,6 +56,18 @@
		KEEPALIVE_EXPIRY = 5.0


		def check_not_closed(method: typing.Callable) -> typing.Callable:

		@@ -619,6 +639,7 @@ def _transport_for_url(self, url: URL) -> httpcore.SyncHTTPTransport:

		return self._transport

		@check_not_closed

Issue warning on unclosed AsyncClient. #1197

Issue warning on unclosed AsyncClient. #1197

Conversation

cdeler commented Aug 19, 2020 • edited by tomchristie Loading

tomchristie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeler Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

cdeler commented Aug 19, 2020

tomchristie commented Aug 19, 2020

cdeler commented Aug 19, 2020

cdeler commented Aug 20, 2020 • edited Loading

tomchristie commented Aug 20, 2020

cdeler commented Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

cdeler Aug 21, 2020 • edited Loading

Choose a reason for hiding this comment

cdeler Aug 21, 2020 • edited Loading

Choose a reason for hiding this comment

tomchristie commented Aug 26, 2020

cdeler commented Aug 26, 2020 • edited Loading

cdeler commented Aug 31, 2020

tomchristie left a comment

Choose a reason for hiding this comment

Issue warning on unclosed `AsyncClient`. #1197

Issue warning on unclosed `AsyncClient`. #1197

cdeler commented Aug 19, 2020 •

edited by tomchristie

Loading

cdeler Aug 19, 2020 •

edited

Loading

cdeler commented Aug 20, 2020 •

edited

Loading

cdeler commented Aug 20, 2020 •

edited

Loading

cdeler Aug 21, 2020 •

edited

Loading

cdeler Aug 21, 2020 •

edited

Loading

cdeler commented Aug 26, 2020 •

edited

Loading