Skip to content

Set suppress_ragged_eofs default based on SSLSocket defaults #695

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 12, 2021
Merged

Set suppress_ragged_eofs default based on SSLSocket defaults #695

merged 1 commit into from
Apr 12, 2021

Conversation

reese-allison
Copy link
Contributor

@reese-allison reese-allison commented Mar 24, 2021 •
edited by temoto
Loading

I have an HTTPS connection that required Connection: close to be sent in the header and it was closing as soon as the data was sent. I cannot post the server URL, but it was raising the below error:

Exception has occurred: SSLError
EOF occurred in violation of protocol (_ssl.c:2570)
  File "C:\Users\allisree\Envs\ami37\Lib\site-packages\urllib3\response.py", line 449, in _error_catcher
    raise SSLError(e)
  File "C:\Program Files\Python37\Lib\contextlib.py", line 130, in __exit__
    self.gen.throw(type, value, traceback)
  File "C:\Users\allisree\Envs\ami37\Lib\site-packages\urllib3\response.py", line 541, in read
    raise IncompleteRead(self._fp_bytes_read, self.length_remaining)
  File "C:\Users\allisree\Envs\ami37\Lib\site-packages\urllib3\response.py", line 576, in stream
    data = self.read(amt=amt, decode_content=decode_content)
  File "C:\Users\allisree\Envs\ami37\Lib\site-packages\requests\models.py", line 753, in generate
    for chunk in self.raw.stream(chunk_size, decode_content=True):
  File "C:\Users\allisree\Envs\ami37\Lib\site-packages\requests\models.py", line 831, in content
    self._content = b''.join(self.iter_content(CONTENT_CHUNK_SIZE)) or b''
  File "C:\Users\allisree\Envs\ami37\Lib\site-packages\requests\sessions.py", line 697, in send
    r.content
  File "C:\Users\allisree\Envs\ami37\Lib\site-packages\requests\sessions.py", line 542, in request
    resp = self.send(prep, **send_kwargs)
  File "C:\Users\allisree\Envs\ami37\Lib\site-packages\requests\api.py", line 61, in request
    return session.request(method=method, url=url, **kwargs)
  File "C:\Users\allisree\Envs\ami37\Lib\site-packages\requests\api.py", line 119, in post
    return request('post', url, data=data, json=json, **kwargs)
  File "C:\Users\allisree\Downloads\test.py", line 15, in <module>
    r = requests.post('https://myurl.com', headers=headers, data=data)

This is a result of suppress_ragged_eofs defaulting to True in SSLSocket, but defaulting to None in GreenSSLSocket when monkey_patched. This only occurs in Python 3.7+.

@temoto
Copy link
Member

temoto commented Mar 24, 2021

To construct test code, please describe HTTP interaction in more detail. Eventlet is on client side? Who sends Connection: close header? Is it the server that closes the connection?

@reese-allison
Copy link
Contributor Author

reese-allison commented Mar 24, 2021 •
edited
Loading

eventlet==0.30.2
requests==2.25.1

Unfortunately I cannot share the URL:

import eventlet
eventlet.monkey_patch()

import requests

headers = {
    'Connection': 'close'
}

r = requests.post('https://myurl.com', headers=headers)
pass

Eventlet is being used to patch requests so that it is non-blocking. I use non-blocking requests to make API calls from a server that is running on eventlet.

When the remote server closes the monkey patched requests SSL connection in Python 3.6, everything operates as expected and the request returns content as normal. When the request is sent in Python 3.7+, the request is terminated and raises an SSLError when the connection is closed because suppress_ragged_eofs is None in GreenSSLSocket instead of True like it is in SSLSocket. Monkey patching ssl is causing this error because GreenSSLSocket has the incorrect default value for suppress_ragged_eofs in Python 3.7+.

@codecov
Copy link

codecov bot commented Mar 24, 2021 •
edited
Loading

Codecov Report

Merging #695 (5e3b76b) into master (e16fcab) will decrease coverage by 0%.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #695   +/-   ##
======================================
- Coverage      44%     44%   -1%     
======================================
  Files          87      87           
  Lines       11920   11920           
  Branches     1780    1780           
======================================
- Hits         5289    5288    -1     
- Misses       6227    6228    +1     
  Partials      404     404
Flag Coverage Δ
ipv6 16% <ø> (ø)
py27epolls 56% <ø> (-1%) ⬇️
py27poll 56% <ø> (-1%) ⬇️
py27selects 55% <ø> (ø)
py35epolls 49% <ø> (-1%) ⬇️
py35poll 49% <ø> (-1%) ⬇️
py35selects 49% <ø> (+<1%) ⬆️
py36epolls 49% <ø> (+<1%) ⬆️
py36poll 49% <ø> (-1%) ⬇️
py36selects 49% <ø> (+<1%) ⬆️
py37epolls 49% <ø> (+<1%) ⬆️
py37poll 49% <ø> (+<1%) ⬆️
py37selects 49% <ø> (+<1%) ⬆️
py38epolls 40% <ø> (-1%) ⬇️
py38openssl 39% <ø> (ø)
py38poll 40% <ø> (ø)
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% <ø> (ø)
eventlet/queue.py 77% <0%> (-1%) ⬇️

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 e16fcab...5e3b76b. Read the comment docs.

@temoto
Copy link
Member

temoto commented Mar 24, 2021

I failed to write a test that would fail with current code.

temoto
temoto approved these changes Mar 24, 2021
View reviewed changes
Copy link
Member

@temoto temoto left a comment

Choose a reason for hiding this comment

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

LGTM

@reese-allison
Copy link
Contributor Author

@temoto
When do pull requests get merged?
I saw two other approved pull requests that haven't been merged and one is more than one year old

@temoto
Copy link
Member

temoto commented Apr 12, 2021

@reese-allison here I waited for other reviewers. If you want to accelerate merge of something in particular, write ping there.

@temoto temoto merged commit 377b4fb into eventlet:master Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@temoto temoto temoto approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@reese-allison @temoto