fix: explicitly free connections in case of server errors

[mattiZed]

This fix ensures that connections obtained from httpx are explicitly closed when the HTTP status code hints at a server error. Fixes #44

[will-ockmore]

Thanks for raising this @mattiZed , great writeup.

We need a couple of test cases to verify that we don't get regressions here.

Specifically we should test that we don't leak connections, so these tests fail without your addition.

I'm also wary that this might be happening for responses that are not server errors.

[mattiZed]

Hi @will-ockmore , thanks for looking into this.

I got some leverage to contribute here as one of our projects is blocked by this issue - I will look into your tests tomorrow 👍

[mattiZed]

Hm, I took a look at it:

• one approach I could I see is partially mocking the httpx.Response and asserting the close/aclose calls

Inspecting the connection pool itself I think current tests don't really touch it at all (and probably they shouldn't?) since the tests never really perform actual requests. What do you think?

[will-ockmore]

Mocking the response object is reasonable. It should (I think?) still work to check the connection pool, even with a mocked response; see if that approach is viable. It's clearly a gap in the testing approach and it would be good to plug it with this PR.

[mattiZed]

So, I had a look yesterday - I just tried to monkeypatch Response.close/aclose with a MagicMock or AsyncMock, however that didn't really work. When I finally receive a response in the test, it is not the one that was formerly .close()'d because there was a retry involved which generated a new response. I hope you can follow.

I assume you have a better understanding of the tests alltogether, would you have a look into it?

[will-ockmore]

I've added tests to verify we always close the response when retrying an operation (this is safe as .close and .aclose are idempotent) in #46 , and included your commit there @mattiZed

[mattiZed]

Great, thanks!