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

Workaround OpenSSL 1.1.1 session-ticket issue #1171

Merged

Conversation

njsmith
Copy link
Member

@njsmith njsmith commented Aug 2, 2019

Fixes #819

@codecov
Copy link

codecov bot commented Aug 2, 2019

Codecov Report

Merging #1171 into master will decrease coverage by 6.93%.
The diff coverage is 91.11%.

@@            Coverage Diff             @@
##           master    #1171      +/-   ##
==========================================
- Coverage   99.55%   92.62%   -6.94%     
==========================================
  Files         106      106              
  Lines       12751    12767      +16     
  Branches      978      980       +2     
==========================================
- Hits        12694    11825     -869     
- Misses         36      877     +841     
- Partials       21       65      +44
Impacted Files Coverage Δ
trio/tests/test_highlevel_ssl_helpers.py 100% <100%> (ø) ⬆️
trio/_ssl.py 96.48% <30%> (-3.52%) ⬇️
trio/tests/test_ssl.py 99.71% <98.71%> (ø) ⬆️
trio/_core/_windows_cffi.py 0% <0%> (-100%) ⬇️
trio/_wait_for_object.py 0% <0%> (-100%) ⬇️
trio/_subprocess_platform/windows.py 0% <0%> (-100%) ⬇️
trio/_subprocess_platform/kqueue.py 0% <0%> (-100%) ⬇️
trio/_windows_pipes.py 0% <0%> (-100%) ⬇️
trio/_core/_io_windows.py 0% <0%> (-91.29%) ⬇️
trio/tests/test_wait_for_object.py 9.02% <0%> (-90.98%) ⬇️
... and 33 more

@njsmith
Copy link
Member Author

njsmith commented Aug 2, 2019

The remaining test failure should be fixed by #1172

@njsmith njsmith closed this Aug 2, 2019
@njsmith njsmith reopened this Aug 2, 2019
@njsmith
Copy link
Member Author

njsmith commented Aug 2, 2019

Not sure why codecov isn't updating its comment. If you click through then it says that everything is covered.

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

The change appears to implement your heuristic but I don't feel qualified enough to approve, sorry!

@@ -40,7 +40,10 @@ class FakeHostnameResolver(trio.abc.HostnameResolver):


# This uses serve_ssl_over_tcp, which uses open_ssl_over_tcp_listeners...
async def test_open_ssl_over_tcp_stream_and_everything_else():
# noqa is needed because flake8 doesn't understand how pytest fixtures work.
Copy link
Member

Choose a reason for hiding this comment

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

I think you can fix the flake8 issue by removing the unused client_ctx import

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not unused, it's making the fixture function visible to pytest...

Copy link
Member

@pquentin pquentin Aug 2, 2019

Choose a reason for hiding this comment

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

Sorry about that, I'm used to projects where pytest discovers the fixtures through conftest.py

@oremanj oremanj merged commit d9d96d9 into python-trio:master Aug 2, 2019
@oremanj
Copy link
Member

oremanj commented Aug 2, 2019

I'm a big fan of the analysis on #819 - thank you for fixing this!

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.

Deadlocks caused by openssl's handling of TLS 1.3 session tickets
3 participants