-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
bpo-44011: New asyncio ssl implementation #17975
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
Conversation
The code part is finished; seems work ok. |
That's the key and should be relatively easy, since the tests already test asyncio in many cases. |
Codecov Report
@@ Coverage Diff @@
## master #17975 +/- ##
===========================================
+ Coverage 82.12% 83.21% +1.08%
===========================================
Files 1956 1571 -385
Lines 589902 415286 -174616
Branches 44477 44509 +32
===========================================
- Hits 484486 345569 -138917
+ Misses 95763 60052 -35711
- Partials 9653 9665 +12
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for working on this! The rest looks good to me.
Oh, looks like we failed to include this in 3.9. @fantix can we commit it at least now? |
Also pinging @asvetlov |
Yes, I believe it's fine to move on. The only missing part is
I'll try to help with the porting when time. |
@asvetlov would you consider rebasing your branch?
Which is not among your changes. |
@dimaqq I'll give it a try this weekend - there're new changes to be added here: MagicStack/uvloop#385 |
This is a very large PR. I don't think I'll be able give you a proper review until feature freeze. We should get together and figure out which features of OpenSSL I can expose to improve the asyncio implementation. For example OpenSSL 1.1.1 has a proper state machine. |
@tiran the PR is a backport of uvloop battle-tested implementation. @fantix (the author of this part of uvloop) already reviewed it. I'm open for new OpenSSL-related improvements but the PR makes a value as is. The current SSL/TLS implementation in asyncio is subtle and error-prone, trust me. This PR is a way to fix the current state. |
Go for it! :) I now realized that I got auto-assigned as reviewer because one of the files contains the string "ssl". |
@asvetlov: Please replace |
Huh, constantly forgetting about |
|
|
|
|
|
|
|
|
|
This reverts commit 5fb06ed and all subsequent dependent commits.
Borrowed from uvloop
https://bugs.python.org/issue44011