use aclosing/closing for async/sync generators

[graingert]

No description provided.

[graingert]

    @pytest.mark.asyncio
    async def test_default_default_headers():
        config = Config(app=app, loop="asyncio", limit_max_requests=1)
        async with run_server(config):
>           async with httpx.AsyncClient() as client:

tests/test_default_headers.py:18: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
venv/lib/python3.10/site-packages/httpx/_client.py:1345: in __init__
    self._transport = self._init_transport(
venv/lib/python3.10/site-packages/httpx/_client.py:1393: in _init_transport
    return AsyncHTTPTransport(
venv/lib/python3.10/site-packages/httpx/_transports/default.py:226: in __init__
    ssl_context = create_ssl_context(verify=verify, cert=cert, trust_env=trust_env)
venv/lib/python3.10/site-packages/httpx/_config.py:49: in create_ssl_context
    return SSLConfig(
venv/lib/python3.10/site-packages/httpx/_config.py:73: in __init__
    self.ssl_context = self.load_ssl_context()
venv/lib/python3.10/site-packages/httpx/_config.py:85: in load_ssl_context
    return self.load_ssl_context_verify()
venv/lib/python3.10/site-packages/httpx/_config.py:122: in load_ssl_context_verify
    context = self._create_default_ssl_context()
venv/lib/python3.10/site-packages/httpx/_config.py:159: in _create_default_ssl_context
    context.options |= ssl.OP_NO_TLSv1
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <ssl.SSLContext object at 0x7fbe6e053140>
value = ssl.OP_NO_COMPRESSION|ssl.OP_ENABLE_MIDDLEBOX_COMPAT|ssl.OP_CIPHER_SERVER_PREFERENCE|ssl.OP_NO_SSLv3|ssl.OP_NO_TLSv1|0x80000054

    @options.setter
    def options(self, value):
>       super(SSLContext, SSLContext).options.__set__(self, value)
E       DeprecationWarning: ssl module: Setting OP_NO_SSL* or SSL_NO_TLS* options is deprecated is deprecated

/usr/lib/python3.10/ssl.py:621: DeprecationWarning

looks like OP_NO_(SSL|TLS)* is deprecated is deprecated

[Kludex]

Should we add the Python 3.10 workflow as well @tomchristie ?

[tomchristie]

@Kludex Yes, I'd suggest that we treat that as a separate pull request.

[graingert]

related: https://github.com/encode/httpcore/pull/364/files

[tomchristie]

So, my review at the moment would be that there's several different things being addressed here, and I'd probably rather see each individual thing tackled as a separate PR, which I've always found helps with clean thorough review processes.

We've got:

• The aclosing change. Which is actually quite neatly done. It conflicts somewhat with the try...finally closing approach in Treat warnings as errors #1687. It might be viewed as an improvement over that, but it'd be good to see it in an isolated PR follow up to Treat warnings as errors #1687.
• The ssl.PROTOCOL_TLS -> ssl.PROTOCOL_TLS_CLIENT change, which is pretty unambiguous. I assume that'll be all that's strictly needed in order to get the warnings removed from 3.10.
• The other SSL changes which are branching specifically to 3.10, which I'd like to review in an isolated PR.

All good stuff, but just needs a bit of separation in order to address each review part with sufficient intent and precision.

However, either ways around we really need to wait until #1687 is resolved before we consider pushing on with this.

[florimondmanca]

Seems like we're good to go with this one? I like the switch to closing/aclosing as well, quite neat. Thank you!

[graingert]

It looks like there's a problem with the type annotation of async_generator.aclosing python-trio/trio-typing#31

[graingert]

Also the other backport of this: jazzband/contextlib2#27

[tomchristie]

I'm not sure? I'm as convinced of the benefits of this.

We're perfectly neat & tidy with our try...finally usage here, and it's not obvious to me if the trade-off is actually worth it. It's a little extra obfuscation to use closing/aclosing, and bumps up our minimum dependency on async_generator.

[graingert]

blocked by jazzband/contextlib2#42

Dismiss

[tomchristie]

I'd like us to pass on this.

The trade-off from replacing two cases of try...finally with aclosing/closing context managers doesn't seem worth it.