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

Candidate fix for #526 (unsupported _context param) #565

Closed
wants to merge 29 commits into from

Conversation

nat-goodspeed
Copy link
Contributor

@nat-goodspeed nat-goodspeed commented May 3, 2019

Fixes #526

@nat-goodspeed
Copy link
Contributor Author

The first step is to verify that we already have at least one test that demonstrates this issue with Python 3.7.3. If not, such a test must be added.

That's why the initial commit for this PR is only a comment.

@codecov-io
Copy link

codecov-io commented May 3, 2019

Codecov Report

Merging #565 into master will increase coverage by <1%.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #565    +/-   ##
======================================
+ Coverage      46%    46%   +<1%     
======================================
  Files          81     81            
  Lines        7976   7976            
  Branches     1365   1365            
======================================
+ Hits         3723   3725     +2     
+ Misses       3997   3995     -2     
  Partials      256    256
Flag Coverage Δ
#ipv6 15% <ø> (ø) ⬆️
#py27epolls 50% <ø> (ø) ⬆️
#py27poll 50% <ø> (ø) ⬆️
#py27selects 49% <ø> (ø) ⬆️
#py34epolls 42% <ø> (ø) ⬆️
#py34poll 42% <ø> (-1%) ⬇️
#py34selects 42% <ø> (ø) ⬆️
#py35epolls 42% <ø> (ø) ⬆️
#py35poll 42% <ø> (ø) ⬆️
#py35selects 42% <ø> (ø) ⬆️
#py36epolls 42% <ø> (-1%) ⬇️
#py36poll 42% <ø> (ø) ⬆️
#py36selects 42% <ø> (ø) ⬆️
#py37epolls 42% <ø> (ø) ⬆️
#py37poll 42% <ø> (ø) ⬆️
#py37selects 42% <ø> (ø) ⬆️
Impacted Files Coverage Δ
eventlet/green/ssl.py 72% <ø> (ø) ⬆️

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 b9bf369...fe97d9b. Read the comment docs.

1 similar comment
@codecov-io
Copy link

Codecov Report

Merging #565 into master will increase coverage by <1%.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #565    +/-   ##
======================================
+ Coverage      46%    46%   +<1%     
======================================
  Files          81     81            
  Lines        7976   7976            
  Branches     1365   1365            
======================================
+ Hits         3723   3725     +2     
+ Misses       3997   3995     -2     
  Partials      256    256
Flag Coverage Δ
#ipv6 15% <ø> (ø) ⬆️
#py27epolls 50% <ø> (ø) ⬆️
#py27poll 50% <ø> (ø) ⬆️
#py27selects 49% <ø> (ø) ⬆️
#py34epolls 42% <ø> (ø) ⬆️
#py34poll 42% <ø> (-1%) ⬇️
#py34selects 42% <ø> (ø) ⬆️
#py35epolls 42% <ø> (ø) ⬆️
#py35poll 42% <ø> (ø) ⬆️
#py35selects 42% <ø> (ø) ⬆️
#py36epolls 42% <ø> (-1%) ⬇️
#py36poll 42% <ø> (ø) ⬆️
#py36selects 42% <ø> (ø) ⬆️
#py37epolls 42% <ø> (ø) ⬆️
#py37poll 42% <ø> (ø) ⬆️
#py37selects 42% <ø> (ø) ⬆️
Impacted Files Coverage Δ
eventlet/green/ssl.py 72% <ø> (ø) ⬆️

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 b9bf369...fe97d9b. Read the comment docs.

@codecov-io
Copy link

codecov-io commented May 3, 2019

Codecov Report

Merging #565 into master will decrease coverage by <1%.
The diff coverage is 39%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #565   +/-   ##
=====================================
- Coverage      46%    45%   -1%     
=====================================
  Files          81     81           
  Lines        7976   8013   +37     
  Branches     1365   1374    +9     
=====================================
- Hits         3722   3681   -41     
- Misses       3998   4068   +70     
- Partials      256    264    +8
Flag Coverage Δ
#ipv6 15% <9%> (-1%) ⬇️
#py27epolls 49% <32%> (-1%) ⬇️
#py27poll 49% <32%> (-1%) ⬇️
#py27selects 48% <29%> (-1%) ⬇️
#py34epolls 42% <31%> (-1%) ⬇️
#py34poll 42% <31%> (-1%) ⬇️
#py34selects 42% <27%> (-1%) ⬇️
#py35epolls 42% <31%> (-1%) ⬇️
#py35poll 42% <31%> (-1%) ⬇️
#py35selects 41% <27%> (-1%) ⬇️
#py36epolls 42% <31%> (-1%) ⬇️
#py36poll 42% <31%> (-1%) ⬇️
#py36selects 42% <27%> (-1%) ⬇️
#py37epolls 41% <22%> (-2%) ⬇️
#py37poll 41% <22%> (-2%) ⬇️
#py37selects 40% <22%> (-2%) ⬇️
Impacted Files Coverage Δ
eventlet/greenpool.py 97% <100%> (ø) ⬆️
eventlet/green/ssl.py 58% <26%> (-14%) ⬇️
eventlet/db_pool.py 72% <0%> (-11%) ⬇️
eventlet/greenio/base.py 87% <0%> (-2%) ⬇️
eventlet/tpool.py 75% <0%> (-1%) ⬇️
eventlet/hubs/hub.py 90% <0%> (-1%) ⬇️
eventlet/green/http/client.py 36% <0%> (ø) ⬆️
eventlet/hubs/__init__.py 82% <0%> (+1%) ⬆️
... and 1 more

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 bdec067...624b208. Read the comment docs.

@nat-goodspeed
Copy link
Contributor Author

Well. Travis's python37 is 3.7.1, which doesn't drive the problem.

@nat-goodspeed
Copy link
Contributor Author

Okay, so I specified Python 3.7.3 for Travis testing -- and all existing tests passed!

Yet I can reproduce #526 merely by attempting a simple requests HTTPS connection from my local machine with Python 3.7.3 and eventlet.monkey_patch(socket=True).

Means (a) there's some other factor involved than specifically Python 3.7.3 or (b) we don't yet have a test that drives this problem. I'll pursue (b) for now.

@breakpointninja
Copy link

Any update on this? python3.7.4 with celery, eventlet and request just fails.

…tatus.

On child-process timeout, instead of merely returning a string containing
'FAIL - timed out', raise AssertionError so the calling test actually fails.
Adjust test_run_python_timeout() to expect that AssertionError.

Also, even when expect_pass=False, check child-process rc, and raise
AssertionError if it's not in the list of allowable values. Add a new keyword
parameter 'allow' whose default value is [0].

Modify tests.patcher_test.MonkeyPatch.test_typeerror(), which expects rc 1, to
pass allow=[1]. Modify patcher_test.ProcessBase.launch_subprocess() to pass
through arbitrary keyword arguments to run_python() to support that.
The new monkey_patch_socket_https_526.py test script depends on an 'openssl'
executable in the PATH. Using this, it creates temporary certificate files
with which to populate an SSLContext so that a local server can accept HTTPS
connections.

We run it by tests.openssl_test.test_https() calling run_isolated() to launch
a new Python interpreter without eventlet imports. For the same reason, the
test script uses multiprocessing to launch the server as a separate process.
Only the client code imports eventlet and uses monkey_patch(socket=True),
which drives the reported issue.
Print the child process output in addition to reporting its rc.

Fix tests.test_run_python_timeout() str/bytes mismatch.
Also revert specifying exactly Python 3.7.3, which is no longer current.
@nat-goodspeed
Copy link
Contributor Author

Odd. The new test script is currently failing to import requests -- despite the successful pip install requests earlier in the same run.
I assume that failure is typical of them all.

@nat-goodspeed
Copy link
Contributor Author

Yes: I just clicked through all the reported failures. Same thing: missing requests module.

@nat-goodspeed
Copy link
Contributor Author

Good news: we finally have a test that drives the problem!
Bad news: I inadvertently broke another test. Working on that.

@nat-goodspeed
Copy link
Contributor Author

Okay! On this branch we have a single new test that drives the reported problem.

Remove GreenSSLContext.wrap_socket() override, which
blows up with the original
TypeError: wrap_socket() got an unexpected keyword argument '_context'
exception. Instead, set GreenSSLContext.sslsocket_class to GreenSSLSocket,
which obviates the wrap_socket() override.

Also, for Python 3.7, remove GreenSSLSocket.__new__() override, which blows up
because SSLSocket._create() calls __new__() with (default) socket=None.

Python 3.7's SSLSocket._create() does the heavy lifting of instantiating
SSLSocket, whose __init__() method unconditionally raises TypeError to
discourage hand-constructing SSLSocket. Unfortunately, from SSLSocket's
subclass GreenSSLSocket, _create()'s super(SSLSocket, self).__init__(...) call
reaches SSLSocket.__init__(), producing that TypeError. Replicate base-class
_create() into GreenSSLSocket, except directly call socket.socket.__init__(),
which that call is intended to reach.
@paunovic
Copy link

Hi guys. Any updates on this?

@itamarst
Copy link
Contributor

Looks like #526 was closed some other way.

@itamarst itamarst closed this Dec 19, 2023
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.

python 3.7 - wrap_socket() got an unexpected keyword argument '_context'
6 participants