Usability issue in Client API -- some args can silently override others

[ugtar]

I just spent some time stumbling over this, and I thought it was worth reporting. Providing a custom HTTPTransport to Client instantiation can override several of Client's initialization arguments. While that does seem logical once you understand how everything works, it can still be a source of frustration.

For example, I had the following (please ignore the obvious insecurity here...), where I prepare a Client which will use a client side cert, but won't verify the server. I also provided a transport solely to set the retries value:

        # Prepare the http client session
        cert = (cert_file.name, key_file.name)
        headers = {'Content-Type': 'application/json', 'accept': 'application/json'}
        # use a short connect timeout and a longer response timeout
        timeout = httpx.Timeout(5.0, read=31.0)
        # handle retry of server closed connection
        transport = httpx.HTTPTransport(retries=2)
        session = stack.enter_context(
            httpx.Client(
                headers=headers,
                verify=False,
                cert=cert,
                timeout=timeout,
                base_url=f"https://{BACKEND_API_HOST}",
                transport=transport,
            )
        )

I was surprised to see the call fail due to httpx.ConnectError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self signed certificate (_ssl.c:1131) when I had explicitly set verify=False, because the transport uses its default which is verify=True. The fix, of course, was to set verify and cert on the HTTPTransport directly rather than on the Client, but it took me some debugging and reading the code before I realized that (this works):

        # Prepare the http client session
        cert = (cert_file.name, key_file.name)
        headers = {'Content-Type': 'application/json', 'accept': 'application/json'}
        # use a short connect timeout and a longer response timeout
        timeout = httpx.Timeout(5.0, read=31.0)
        # handle retry of server closed connection
        transport = httpx.HTTPTransport(retries=2, verify=False, cert=cert)
        session = stack.enter_context(
            httpx.Client(
                headers=headers,
                timeout=timeout,
                base_url=f"https://{BACKEND_API_HOST}",
                transport=transport,
            )
        )

I get having this much flexibility in the interface has its pitfalls, but it would be nice if when providing a custom transport, or a custom ssl context for that matter, if the Client constructor could warn about those overriding the Client arguments, especially, since it's not always obvious which of Client's arguments will be affected. For example, setting a transport will override Client's verify and cert arguments, but not timeout which could plausibly be set on transport as well (though today it isn't).

[tomchristie]

Yes, it's a little awkward there, isn't it? I suppose a good starting point would be just listing on this thread which arguments are relevant to this discussion, so we can understand what we'd need to change in order to have better erroring around invalid configurations.

[ugtar]

From what I see initially, Transport will override Client's verify, cert, limits, and trust_env arguments. Defining mounts is also a potential can of worms. Where this gets tricky is that even if warnings are defined for these, if for example, timeout is added to Transport in the future, or conversely, retries is added to Client, someone will have to make sure the appropriate warnings are added. It may be a better tactic to define a behavior.

That is also tricky though, because I can see arguments both ways I.e., that for example, setting verify=XX on transport which is passed to a Client would use transport's value, since that is correct. Or on the other hand, if verify is set on Client, but not on Transport, it would seem to indicate that the intent is the use the Client's value. In addition, it seems logical that a value set on Client might take precedence over the value set on the Transport since the Client usage is more immediate (eg, the life of a context), and perhaps therefore, more imperative than the Transport's values.

Sorry, I don't have any good answers... Maybe just punting on complex behavior in favor of warnings is best.

[bede]

I just ran into this, and was helpully pointed in this direction by @ugtar

I was left confused when composing custom limits and transport when constructing a client, because my limits were overriden by adding transport (with its own limits), as in this example:

async def get_urls(urls: list[str]):
    transport = httpx.AsyncHTTPTransport(retries=5)
    limits = httpx.Limits(max_connections=1)
    async with httpx.AsyncClient(transport=transport, limits=limits) as client:
        tasks = [get_url(client, url) for url in urls]
        return [await f for f in asyncio.as_completed(tasks)] 

async def get_url(client: httpx.AsyncClient, url: str):
    r = await client.get(url)
    if r.status_code == httpx.codes.ok:
        return r.json()

if __name__ == '__main__':
    urls = [f"https://jsonplaceholder.typicode.com/comments/{i}" for i in range(51)]
    records = asyncio.run(get_urls(urls))
    print(records)

[kchen]

I just filed #3234 because I didn't initially find this discussion, but that is the same problem. As I mentioned there, it would be useful if the documentation would be more clear, although that's probably hard. However, I imagine it would be straightforward to raise an exception if a Transport is passed into a new Client along with other options that will be ignored.