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

ssl: context wrapped listener failed to supply _context in accept() #655

Merged
merged 1 commit into from Oct 20, 2020

Conversation

temoto
Copy link
Member

@temoto temoto commented Oct 18, 2020

#651

Seems correct, but could potentially break a lot of code, so calling for extensive public testing.

pip install -U https://github.com/eventlet/eventlet/archive/i651.zip and try to run your software.

@temoto
Copy link
Member Author

temoto commented Oct 18, 2020

Last version fails on Python 2.7 - 3.6, works on 3.7+.

I'm tired today, will continue later. Error with keyfile argument.

Traceback (most recent call last):
  File "/home/travis/build/eventlet/eventlet/tests/ssl_test.py", line 342, in test_context_wrapped_accept
    peer, _ = server_tls.accept()
  File "/home/travis/build/eventlet/eventlet/eventlet/green/ssl.py", line 407, in accept
    keyfile=self.keyfile,
AttributeError: 'GreenSSLSocket' object has no attribute 'keyfile'

Copy link
Contributor

@jstasiak jstasiak left a comment

Choose a reason for hiding this comment

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

Works for me on Python 3.8.5 and looks reasonable.

@temoto
Copy link
Member Author

temoto commented Oct 18, 2020

Works now.

@codecov-io
Copy link

codecov-io commented Oct 18, 2020

Codecov Report

Merging #655 into master will increase coverage by 0%.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #655   +/-   ##
======================================
  Coverage      44%     44%           
======================================
  Files          87      87           
  Lines       11841   11841           
  Branches     1777    1778    +1     
======================================
+ Hits         5278    5279    +1     
+ Misses       6167    6166    -1     
  Partials      396     396           
Flag Coverage Δ
#ipv6 16% <ø> (ø)
#py27epolls 56% <ø> (+<1%) ⬆️
#py27poll 56% <ø> (ø)
#py27selects 55% <ø> (+<1%) ⬆️
#py35epolls 49% <ø> (-1%) ⬇️
#py35poll 49% <ø> (+<1%) ⬆️
#py35selects 49% <ø> (+<1%) ⬆️
#py36epolls 49% <ø> (+<1%) ⬆️
#py36poll 49% <ø> (ø)
#py36selects 49% <ø> (ø)
#py37epolls 49% <ø> (+<1%) ⬆️
#py37poll 49% <ø> (ø)
#py37selects 49% <ø> (+<1%) ⬆️
#py38epolls 41% <ø> (+<1%) ⬆️
#py38poll 41% <ø> (+<1%) ⬆️
#py38selects 40% <ø> (-1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
eventlet/green/ssl.py 48% <ø> (+<1%) ⬆️
eventlet/hubs/__init__.py 70% <0%> (-1%) ⬇️
eventlet/greenio/base.py 80% <0%> (-1%) ⬇️
eventlet/patcher.py 31% <0%> (-1%) ⬇️
eventlet/greenthread.py 81% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a185b1a...fbcc0de. Read the comment docs.

@jstasiak
Copy link
Contributor

How does the code work on Python older than 3.8 with keyfile, certfile not being passed through?

@temoto
Copy link
Member Author

temoto commented Oct 19, 2020

@jstasiak

How does the code work on Python older than 3.8 with keyfile, certfile not being passed through?

Sorry, it's a working black magic for me, currently. Test passes, so apparently they are read from context. So I was wondering is it redundant to pass them at all. And can you believe it, all key/cert related kwargs were redundant. It passes py2.7 and 3.7 without extra 3.7 condition. Hope it will pass all tests.

Thank you.

@temoto
Copy link
Member Author

temoto commented Oct 19, 2020

@jstasiak and for shorter diff too, I've only noticed it too late after push, was too much excited about dropping cert/key kwargs. Done.

Copy link
Contributor

@jstasiak jstasiak left a comment

Choose a reason for hiding this comment

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

Yeah it was just a detail, LGTM regardless.

@temoto temoto merged commit fbcc0de into master Oct 20, 2020
@temoto temoto deleted the i651 branch October 20, 2020 00:42
@temoto temoto linked an issue Oct 20, 2020 that may be closed by this pull request
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.

SSLSocket.accept fails when monkey-patched
3 participants