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

Checklist

The bug is reproducible against the latest release and/or master.
There are no similar issues or pull requests to fix it yet.

Describe the bug

I'm playing around with httpx to learn how async programming works in Python, and I've noticed it's pretty easy to get into a state where I have an async task using an AsyncClient that keeps running after I've exited the with block that created the client, i.e., after the the __aclose__ method of the client has been called.

Here's a simple example in asyncio:

import httpx
import asyncio

async def get(client, url):
    r = await client.get(url)
    print(r.text)

async def main():
    async with httpx.AsyncClient() as client:
        t1 = asyncio.create_task(get(client, "https://httpbin.org/delay/1"))
        t2 = asyncio.create_task(get(client, "https://httpbin.org/delay/2"))

    await t1
    await t2

if __name__ == "__main__":
    asyncio.run(main())

Here's a simple example in trio (where I ran into this):

import httpx
import trio

async def get(client ,url):
    x = await client.get(url)
    print(x.text)

async def main():
    async with trio.open_nursery() as nursery:
        async with httpx.AsyncClient() as client:
            nursery.start_soon(get, client, "https://httpbin.org/delay/1")
            nursery.start_soon(get, client, "https://httpbin.org/delay/2")

if __name__ == "__main__":
    trio.run(main)

These produce the same output: they run the two requests in parallel, print the output, and then display the warning

.../venv/lib/python3.7/site-packages/httpx/_client.py:1777: UserWarning: Unclosed <httpx.AsyncClient object at 0x7fbe37ad33c8>. See https://www.python-httpx.org/async/#opening-and-closing-clients for details.
  f"Unclosed {self!r}. "

It's surprising to me that the code both works and prints the warning. What's actually going on is that the __aclose__ method sets a flag indicating the client's been closed and then calls __aclose__ on the transport, which flushes and closes any open connections, but does not actually leave the transport in an unusable state. That means that new connections can still get created. Since in both cases the tasks are scheduled for after the with block has exited, I think that they deterministically don't start until after __aclose__ finishes, ie., they aren't affected by tearing down any existing connections, and they go on to create new ones. Then the send method goes and unsets the _is_closed flag (which I'm confused about - I see this behavior is intentional per #1197 (comment) but I don't follow why) and you're left with an unclosed client at GC time.

I think it would be helpful for __aclose__ to leave the client in an unusable state, e.g., set self._transport = None, or have send fail if _is_closed is set, if the intention is that clients should not be reopened once closed. That would prevent users from writing code where they think they're cleaning up properly, but they're actually cleaning up too early, while requests are still in flight.

If the intention is they can be reopened, it would be helpful for the URL above to mention this - I was greatly confused because I was using exactly the method in that URL (as you can see above), it just turns out I was unintentionally reopening them. (Perhaps reopening a closed client should issue a warning of its own?)

To reproduce

See above.

Expected behavior

I expect this to either work properly and not print a warning, or to fail, as described above.

Actual behavior

The code works but prints a warning because it works accidentally, as described above.

Debugging material

See above.

Environment

macOS, Python 3.7.3, httpx 0.15.4 from pip, trio 0.17.0.

OS: macOS
Python version: 3.7.3
HTTPX version: 0.15.4
Async environment: Both, as indicated above. trio 0.17.0
HTTP proxy: no
Custom certificates: no

Additional context

none

Or possibly _is_closed should be a tristate: NOT_YET_OPENED → OPEN → CLOSED

Or possibly _is_closed should be a tristate: NOT_YET_OPENED → OPEN → CLOSED

That's the impression I had.

See #1317 for other discussion.

This pull request removes the opening on send behaviour: #1320

@geofft

Thanks for reporting this, @geofft! Indeed an interesting one.

This is somewhat related to #1317. tl;dr: indeed, the current ._is_closed logic as of the latest release is broken, and #1320 aims at fixing that.

Could you install HTTPX from the branch on #1320 and see what's the new behavior your observe there? There's the idea of a separate _seen_io flag in the async case, which somewhat relates to njs' "triflag" idea.

Also interesting that this replicates fairly well on a raw HTTPCore application:

import httpcore
import trio


async def get(transport, url, headers):
    print(f"Request {url}: started")
    _, _, stream, _ = await transport.arequest(b"GET", url, headers)
    body = b"".join([chunk async for chunk in stream]).decode("utf-8")
    print(f"Request {url}: finished")


async def main():
    async with trio.open_nursery() as nursery:
        async with httpcore.AsyncConnectionPool() as transport:
            headers = [(b"host", b"httpbin.org")]
            url1 = (b"https", b"httpbin.org", None, b"/delay/1")
            url2 = (b"https", b"httpbin.org", None, b"/delay/2")
            nursery.start_soon(get, transport, url1, headers)
            nursery.start_soon(get, transport, url2, headers)
        print("Transport closed")


if __name__ == "__main__":
    trio.run(main)

$ python example.py
Transport closed
Request (b'https', b'httpbin.org', None, b'/delay/2'): started
Request (b'https', b'httpbin.org', None, b'/delay/1'): started
Request (b'https', b'httpbin.org', None, b'/delay/1'): finished
Request (b'https', b'httpbin.org', None, b'/delay/2'): finished

That is, even though we have exited the transport context by when requests actually fire off, it doesn't prevent those requests from being made, and doesn't raise any error. This means the connection pool is flushed, then we make requests, potentially leaving connections hanging around until the program terminates.

Don't think that's what we're expecting to happen. 🤔 (I think I'd expect the transport.request() to raise some kind of error saying "nope, you're not inside the transport context, you're doing something wrong".)

@tomchristie

Actually, my intuition above^ is in line with what @tomchristie wrote in #1317 (comment):

I'd suggest we probably want the next proper version bump to hard-enforce that, raising an error if you attempt to send a request on a closed client.

But the tweak there is that this might also be something we'd want to enforce at the transport level.

In the meantime, the workaround is to make sure the client is properly scope above any kind of nursery / task group, so:

import httpx
import asyncio

async def get(client, url):
    r = await client.get(url)
    print(r.text)

async def main():
    async with httpx.AsyncClient() as client:
        t1 = asyncio.create_task(get(client, "https://httpbin.org/delay/1"))
        t2 = asyncio.create_task(get(client, "https://httpbin.org/delay/2"))

        await t1
        await t2

if __name__ == "__main__":
    asyncio.run(main())

And:

import httpx
import trio

async def get(client ,url):
    x = await client.get(url)
    print(x.text)

async def main():
    async with httpx.AsyncClient() as client:
        async with trio.open_nursery() as nursery:
            nursery.start_soon(get, client, "https://httpbin.org/delay/1")
            nursery.start_soon(get, client, "https://httpbin.org/delay/2")

if __name__ == "__main__":
    trio.run(main)

Could you install HTTPX from the branch on #1320 and see what's the new behavior your observe there? There's the idea of a separate _seen_io flag in the async case, which somewhat relates to njs' "triflag" idea.

$ python3 -m venv httpx1320
$ httpx1320/bin/pip install -U pip
$ httpx1320/bin/pip install trio git+https://github.com/encode/httpx@refs/pull/1320/merge
$ httpx1320/bin/python aioproblem.py

prints both requests but not the warning, with either the asyncio or trio variants.

Re the other comments, hard error seems reasonable to me, and yes, swapping the order of the client scope and the task scope fixes things.

Navigation Menu

Repository navigation

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

Checklist

Describe the bug

To reproduce

Expected behavior

Actual behavior

Debugging material

Environment

Additional context

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Checklist

Describe the bug

To reproduce

Expected behavior

Actual behavior

Debugging material

Environment

Additional context

Activity

njsmith commented on Sep 28, 2020

johtso commented on Sep 28, 2020

florimondmanca commented on Sep 29, 2020

florimondmanca commented on Sep 29, 2020

florimondmanca commented on Sep 29, 2020

florimondmanca commented on Sep 29, 2020

geofft commented on Sep 29, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions