Fixed the problem with socket, which might leak when tunnel proxy connection couldn't be establish #223

cdeler · 2020-10-14T10:30:42Z

I extracted a tunnel proxy connection establishing into the separated method, and wrapped it by try-except statement, closing the proxy_connection if it's necessary.

Also I added the unit test and a test fixture from encode/httpx#1360 comments

cdeler · 2020-10-14T10:34:55Z

In the test case I had to filter out any warnings from encode/httpx#825

The couldn't appear in this test case (as it's run under trio), so I felt free to do that

tomchristie · 2020-10-16T13:06:55Z

httpcore/_async/http_proxy.py

@@ -172,33 +172,22 @@ async def _forward_request(

        return status_code, headers, wrapped_stream, ext

-    async def _tunnel_request(


Great stuff! So...

I think the first thing would be to try to get the change footprint of this PR down a bit.

I'd suggest we keep a single _tunnel_request method, rather than splitting out into two methods here?

Can we catch the exception more narrowly? Perhaps we could have a top-level TransportError class, in the same way as httpx. Then, in cases like this we could just catch TransportError.

Actually thinking about it let's treat that second line item independently of this pull request.

@tomchristie do I understand correctly the plan:

to introduce the root exception for all httpcore exceptions, to name the class TransportError. This change should go in the separate PR

as soon as (1) has been merged, to merge _establish_tunnel_proxy_connection back into _tunnel_request, catching TransportError instead of Exception?

Talking about (2) we can try. But also it means that there are cases (some error conditions) when we do not clean up resources. The conditions are exceptions not inherited by TransportError...

No don't worry about (2) at all. We can think about that some other day.

Therefore I'm to just eliminate _establish_tunnel_proxy_connection, moving this code back to _tunnel_request to reduce the PR footprint, yes? (sorry for repeating questions, I want to fully understand you)

@tomchristie
I did that, so the PR became significantly smaller (but it's required to compare it ignoring whitespace changes)

…nection couldn't be established

florimondmanca

Interesting, and a bit fiddly, so I'm being a bit careful :-)

florimondmanca · 2020-10-17T11:06:03Z

tests/async_tests/test_interfaces.py

+    url = (protocol, b"example.com", port, b"/")
+    headers = [(b"host", b"example.org")]


Is the host mismatch between url / headers expected? I think this is somewhat related to the block file passed to pproxy. If so, ad assuming the block file is intended to be a list of URL hosts that cannot be requested through the proxy…

Should we set the blocked hostname to something more explicit like blockedhost.org?

If the different host in url and headers is on purpose and required, add a small comment to explain why?

You are right! I missed that the url and header are different, and it might confuse. Moreover the server name is not so clear. I'm renaming this host to "blockedhost.example.com" (and changing the fixture the same way)

tests/async_tests/test_interfaces.py

florimondmanca · 2020-10-17T11:08:38Z

httpcore/_async/http_proxy.py

+                    ssl_context=self._ssl_context,
+                    socket=proxy_connection.socket,
+                )
+                await self._add_to_pool(connection, timeout)


Do we want to explicitly put this operation outside the try/except block? I assume the only operations that we want to fail upon is socket-related stuff, so proxy_connection.arequest() and proxy_connection.start_tls(). Anything that's not related to it, such as adding to the pool, should be left outside the block, correct?

Yes, you are right, the try-except block should be as narrow as possible

florimondmanca · 2020-10-17T11:09:25Z

tests/utils.py

+
+
+@contextlib.contextmanager
+def create_proxy_block_file(blocked_domains: List[str]):


I'd suggest we add a small docstring here with any relevant docs material from pproxy (eg a link to relevant docs) describing what a "block file" is and what effect it has?

I added some docs to http_proxy_server and create_proxy_block_file

httpcore/_async/http_proxy.py

- renamed the blocked host to blockedhost.example.com - added some docs to http_proxy_server fixture

Co-authored-by: Florimond Manca <florimond.manca@gmail.com>

cdeler · 2020-10-17T18:43:55Z

@florimondmanca
Thank you for the review, I fixed all you mentioned

florimondmanca

Rad :) Great one!

cdeler requested review from a team, florimondmanca and tomchristie October 14, 2020 12:47

tomchristie reviewed Oct 16, 2020

View reviewed changes

cdeler added 2 commits October 16, 2020 20:49

Added test case for the leaked proxy socket

546325a

Fixed the problem with socket, which might leak when tunnel proxy con…

10df7fa

…nection couldn't be established

cdeler force-pushed the fix-leaked-socket branch from edf4a82 to 10df7fa Compare October 16, 2020 17:56

cdeler requested a review from tomchristie October 16, 2020 17:59

florimondmanca reviewed Oct 17, 2020

View reviewed changes

httpcore/_async/http_proxy.py Outdated Show resolved Hide resolved

cdeler and others added 2 commits October 17, 2020 19:30

Fixed some PR issues

c69e392

- renamed the blocked host to blockedhost.example.com - added some docs to http_proxy_server fixture

Update tests/async_tests/test_interfaces.py

dfe46ac

Co-authored-by: Florimond Manca <florimond.manca@gmail.com>

cdeler force-pushed the fix-leaked-socket branch from dfc2c59 to dfe46ac Compare October 17, 2020 16:38

Update httpcore/_async/http_proxy.py

74d87b0

Co-authored-by: Florimond Manca <florimond.manca@gmail.com>

cdeler force-pushed the fix-leaked-socket branch from 9a2a91d to 74d87b0 Compare October 17, 2020 16:47

Narrowed a try-except block in _tunnel_request

0a6bd9c

cdeler requested a review from florimondmanca October 17, 2020 18:44

Merge branch 'master' into fix-leaked-socket

43ac004

cdeler requested a review from a team October 21, 2020 08:39

Merge branch 'master' into fix-leaked-socket

5413954

florimondmanca approved these changes Oct 29, 2020

View reviewed changes

cdeler merged commit 62b4731 into encode:master Oct 30, 2020

cdeler deleted the fix-leaked-socket branch October 30, 2020 08:38

florimondmanca mentioned this pull request Nov 4, 2020

Version 0.12.1 #231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed the problem with socket, which might leak when tunnel proxy connection couldn't be establish #223

Fixed the problem with socket, which might leak when tunnel proxy connection couldn't be establish #223

cdeler commented Oct 14, 2020

cdeler commented Oct 14, 2020

tomchristie Oct 16, 2020

tomchristie Oct 16, 2020

cdeler Oct 16, 2020 •

edited

Loading

tomchristie Oct 16, 2020

cdeler Oct 16, 2020

cdeler Oct 16, 2020

florimondmanca left a comment

florimondmanca Oct 17, 2020

cdeler Oct 17, 2020

florimondmanca Oct 17, 2020

cdeler Oct 17, 2020 •

edited

Loading

florimondmanca Oct 17, 2020

cdeler Oct 17, 2020

cdeler commented Oct 17, 2020

florimondmanca left a comment

		@@ -172,33 +172,22 @@ async def _forward_request(

		return status_code, headers, wrapped_stream, ext

		async def _tunnel_request(

		url = (protocol, b"example.com", port, b"/")
		headers = [(b"host", b"example.org")]



		@contextlib.contextmanager
		def create_proxy_block_file(blocked_domains: List[str]):

Fixed the problem with socket, which might leak when tunnel proxy connection couldn't be establish #223

Fixed the problem with socket, which might leak when tunnel proxy connection couldn't be establish #223

Conversation

cdeler commented Oct 14, 2020

cdeler commented Oct 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeler Oct 16, 2020 • edited Loading

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

florimondmanca 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

cdeler Oct 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeler commented Oct 17, 2020

florimondmanca left a comment

Choose a reason for hiding this comment

cdeler Oct 16, 2020 •

edited

Loading

cdeler Oct 17, 2020 •

edited

Loading