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

Fixing missed arguments for pools when init transports #857

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

NewUserHa
Copy link

Summary

related to PR: encode/httpx#2997

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@karpetrosyan
Copy link
Member

karpetrosyan commented Dec 11, 2023

Let's start with an understanding of what a proper signature for http proxy connections is.

Here is the signature of AsyncForwardHTTPConnection class.

class AsyncForwardHTTPConnection(AsyncConnectionInterface):
    def __init__(
        self,
        proxy_origin: Origin,
        remote_origin: Origin,
        proxy_headers: Union[HeadersAsMapping, HeadersAsSequence, None] = None,
        keepalive_expiry: Optional[float] = None,
        network_backend: Optional[AsyncNetworkBackend] = None,
        socket_options: Optional[Iterable[SOCKET_OPTION]] = None,
        proxy_ssl_context: Optional[ssl.SSLContext] = None,
    ) -> None:

I believe we are missing a few parameters here, as it is simply a wrapper for the AsyncHTTPConnection, which has the following signature:

class AsyncHTTPConnection(AsyncConnectionInterface):
    def __init__(
        self,
        origin: Origin,
        ssl_context: Optional[ssl.SSLContext] = None,
        keepalive_expiry: Optional[float] = None,
        http1: bool = True,
        http2: bool = False,
        retries: int = 0,
        local_address: Optional[str] = None,
        uds: Optional[str] = None,
        network_backend: Optional[AsyncNetworkBackend] = None,
        socket_options: Optional[Iterable[SOCKET_OPTION]] = None,
    ) -> None:

The constructor of AsyncForwardHTTPConnection contains the following code:

        self._connection = AsyncHTTPConnection(
            origin=proxy_origin,
            keepalive_expiry=keepalive_expiry,
            network_backend=network_backend,
            socket_options=socket_options,
            ssl_context=proxy_ssl_context,
        )
        self._proxy_origin = proxy_origin
        self._proxy_headers = enforce_headers(proxy_headers, name="proxy_headers")
        self._remote_origin = remote_origin

So we need all of the arguments from AsyncHTTPConnection PLUS proxy_origin, remote_origin, proxy_ssl_context, and proxy_headers for the proxy logic in AsyncForwardHTTPConnection.

Here are the arguments that the proxy class lacks:

  • http1
  • http2
  • retries
  • local_address
  • uds

Also, in the ForwardHTTPConnection, we could change the argument proxy_ssl_context to ssl_context because, unlike the TunnelHTTPConnection, we only have one ssl layer, so proxy_ssl_context is, in my opinion, a little bit ambigious here.

@tomchristie
Copy link
Member

Understood, thanks for this @NewUserHa.
Looks like there's some unintended reorderings here.
If those can be removed we can get a lower footprint to this PR, which will make it more neatly reviewable.

@NewUserHa
Copy link
Author

The HttpProxy and Socks5Proxy only need either proxy_url or uds, should default proxy_url to ""?

@karpetrosyan
Copy link
Member

In this PR, we can simply enforce the ordering of retries, local_address, and uds arguments, and open another for adding retries to Proxy.

For uds support in proxy classes, let's just open a discussion first.

@Panda1283
Copy link

The HttpProxy and Socks5Proxy only need either proxy_url or uds, should default proxy_url to ""?

@NewUserHa
Copy link
Author

discussion for uds support in proxy classes: #858

@NewUserHa
Copy link
Author

done.
now this PR only:

  • added lacked retries, local_address, uds, socket_options parameters and corresponding doc strings;
  • and reordered these parameters for consistency.

@NewUserHa
Copy link
Author

NewUserHa commented Dec 14, 2023

the SocksProxy still lacks local_address, uds;
And retries is in the class only but Socks5Connection does not use it, can open another PR for these.

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.

4 participants