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

Implement AsyncHTTPConnection.aclose #96

Closed
wants to merge 1 commit into from
Closed

Implement AsyncHTTPConnection.aclose #96

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented May 27, 2020

The base class implementation is empty and python will otherwise emit
a "ResourceWarning unclosed <socket.socket ...>" for the unclosed
sockets.

The base class implementation is empty and python will otherwise emit
a "ResourceWarning unclosed <socket.socket ...>" for the unclosed
sockets.
@tomchristie
Copy link
Member

Hiya!

So, httpcore is a low-level HTTP client, which means that you're required to ensure you're closing connections, eg. as documented in the quickstart...

    http_version, status_code, reason_phrase, headers, stream = await http.request(
        method=b'GET',
        url=(b'https', b'example.org', 443, b'/'),
    )

    try:
        body = b''.join(chunk async for chunk in stream)
    finally:
        await stream.close()

It's really important to use the try ... finally pattern if you're using httpcore at a low level.

Higher level clients like httpx will take care of this for you. Ie. httpx always manages the close for you, either with basic requests, or with .stream() using a context-managed API.

Having said that, we should forcibly close outstanding connections, yup, although this PR isn't quite the right approach. (I'm also surprised that it passed the tests - since the sync & async cases aren't in sync here.)

@tomchristie
Copy link
Member

Closing in favour of #98

@ghost
Copy link
Author

ghost commented May 27, 2020

I did run into this using httpx, which does call aclose on the AsyncHTTPConnection. I've used the async with httpx.AsyncClient() as client context manager stuff.

Should this have worked without the fix? Is there another bug in httpx?

@tomchristie
Copy link
Member

Ah gotcha. Anyways yup, without #98 we're not properly closing connections when the pool is cleaned up. We'll release a 0.9.1 version shortly. Thanks for having filed this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants