Skip to content

Tweak transport closing#1320

Closed
lovelydinosaur wants to merge 6 commits into
masterfrom
tweak-transport-closing
Closed

Tweak transport closing#1320
lovelydinosaur wants to merge 6 commits into
masterfrom
tweak-transport-closing

Conversation

@lovelydinosaur
Copy link
Copy Markdown
Contributor

@lovelydinosaur lovelydinosaur commented Sep 24, 2020

Closes #1317

  • Always close transports if client.close() is called, even if no requests have been made.
  • Treat client instances as "open" as soon as they're instantiated, even if no requests have been made.

florimondmanca
florimondmanca previously approved these changes Sep 24, 2020
Copy link
Copy Markdown
Contributor

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

Exactly what I had in mind :)

Comment thread httpx/_client.py
@johtso
Copy link
Copy Markdown
Contributor

johtso commented Sep 24, 2020

We probably want to cover the new transport closing behavior in the tests:

class TestTransportClosing:
    def __init__(self):
        # Note that we're including 'proxies' here to *also* run through the
        # proxy context management, although we can't easily test that at the
        # moment, since we can't add proxies as transport instances.
        #
        # Once we have a more generalised Mount API we'll be able to remove this
        # in favour of ensuring all mounts are context managed, which will
        # also necessarily include proxies.

        self.transport = Transport()
        self.client = httpx.Client(transport=self.transport, proxies="http://www.example.com")

    def test_with_context_manager(self):
        assert self.transport.events == []

        with self.client:
            pass

        assert self.transport.events == [
            "transport.__enter__",
            "transport.close",
            "transport.__exit__",
        ]

    def test_without_context_manager(self):
        assert self.transport.events == []

        self.client.close()

        assert self.transport.events == [
            "transport.close"
        ]

@johtso
Copy link
Copy Markdown
Contributor

johtso commented Sep 24, 2020

Also, we now get a lot of unclosed client UserWarnings when running the tests.

@lovelydinosaur lovelydinosaur added the do not merge PRs that should not be merged label Sep 24, 2020
@lovelydinosaur lovelydinosaur mentioned this pull request Sep 24, 2020
@lovelydinosaur
Copy link
Copy Markdown
Contributor Author

Also, we now get a lot of unclosed client UserWarnings when running the tests.

Okay, right, which kinda gets to the heart of the two different options here.

We can either:

  • Suggest that transport instances shouldn't hold resources on init, but should either do so on __enter__ or if opened implicitly the first time a .request() is made.
  • Treat clients as open as soon as they're instantiated, and if they've not been explicitly closed, then close them on __del__.

We're in a slightly different position with Client and AsyncClient because Client.__init__ might perform I/O and allocate resources, and Client.__del__ is always able to release them, whereas AsyncClient.__init__ won't ever perform I/O, and AsyncClient.__del__ is unable to perform I/O to release resources.

@florimondmanca florimondmanca removed the do not merge PRs that should not be merged label Sep 24, 2020
@lovelydinosaur
Copy link
Copy Markdown
Contributor Author

Also, we now get a lot of unclosed client UserWarnings when running the tests.

So, I've made a slight tweak here, for AsyncClient only...

__del__ should only raise a warning, if the client is unclosed and has performed I/O. (A request or __aenter__)

(We don't care about this for Client because __del__ never needs to raise a warning, since it can always just call .close on an unclosed client)

Not sure what we think about this.

Incidentally wrt. CacheControl if you're expecting to expand it into sync+async support then you will want to only initialise the resources either on __enter__ or on the first time request is called, because async can't make I/O in the __init__ call. (because it's always a sync method)

@johtso
Copy link
Copy Markdown
Contributor

johtso commented Sep 24, 2020

Was thinking exactly along these lines, makes sense to me.

Incidentally wrt. CacheControl if you're expecting to expand it into sync+async support then you will want to only initialise the resources either on __enter__ or on the first time request is called, because async can't make I/O in the __init__ call. (because it's always a sync method)

Ah yes, look forward to exploring the a/sync rabbit hole!

@lovelydinosaur lovelydinosaur added the user-experience Ensuring that users have a good experience using the library label Sep 24, 2020
@florimondmanca
Copy link
Copy Markdown
Contributor

@tomchristie Heads up — we've got an interesting real-world case that we could test this new logic against in #1332.

@lovelydinosaur
Copy link
Copy Markdown
Contributor Author

Superseeded by #1346

@lovelydinosaur lovelydinosaur deleted the tweak-transport-closing branch October 7, 2020 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

user-experience Ensuring that users have a good experience using the library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Client.close() does not close transports if unused

3 participants