FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

use aclosing/closing for async/sync generators by graingert · Pull Request #1671 · encode/httpx · GitHub

  1. encode
  2. httpx

Repository navigation

use aclosing/closing for async/sync generators - #1671

Closed
graingert wants to merge 3 commits into
encode:masterfrom
graingert:patch-1
ConversationCommits (3)ChecksFiles changed
Closed

use aclosing/closing for async/sync generators#1671
graingert wants to merge 3 commits into
encode:masterfrom
graingert:patch-1

Conversation

graingert commented Jun 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

No description provided.

graingert commented Jun 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author 
    @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

graingert mentioned this pull request Jun 8, 2021
Merged
Kludex reviewed Jun 8, 2021
View reviewed changes
Comment thread httpx/_config.py Outdated
graingert requested a review from Kludex June 8, 2021 17:54

Kludex commented Jun 8, 2021

Copy link
Copy Markdown
Contributor

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

lovelydinosaur reviewed Jun 9, 2021
View reviewed changes
Comment thread httpx/_config.py Outdated

lovelydinosaur commented Jun 9, 2021

Copy link
Copy Markdown
Member

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

graingert mentioned this pull request Jun 14, 2021
Merged
lovelydinosaur added the refactor Issues and PRs related to code refactoring label Jun 15, 2021

graingert commented Jun 15, 2021

Copy link
Copy Markdown
Contributor Author

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

lovelydinosaur commented Jun 16, 2021

Copy link
Copy Markdown
Member

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.

graingert changed the title avoid deprecated ssl.PROTOCOL_TLS use aclosing/closing for async/sync generators Jun 16, 2021
florimondmanca previously approved these changes Jun 24, 2021
View reviewed changes

florimondmanca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

florimondmanca removed the request for review from Kludex June 24, 2021 10:12

graingert commented Jun 24, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

graingert commented Jun 24, 2021

Copy link
Copy Markdown
Contributor Author

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

lovelydinosaur commented Jun 24, 2021

Copy link
Copy Markdown
Member

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 commented Jul 1, 2021

Copy link
Copy Markdown
Contributor Author

blocked by jazzband/contextlib2#42

lovelydinosaur commented Sep 7, 2021

Copy link
Copy Markdown
Member

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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lovelydinosaur lovelydinosaur left review comments
+2 more reviewers
Kludex Kludex left review comments
florimondmanca florimondmanca left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

refactor Issues and PRs related to code refactoring

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL