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

Fix SSL read timeouts in Python 2.7 #1222

Closed
wants to merge 2 commits into from

Conversation

zbristow
Copy link
Contributor

@zbristow zbristow commented Oct 9, 2019

Pull Request check-list

  • Does $ python setup.py test pass with this change (including linting)?
  • Does travis tests pass with this change (enable it first in your forked repo and wait for the travis build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

Description of change

The ssl module in Python 2.7 raises timeouts as ssl.SSLError instead of socket.timeout. When these timeouts are encountered, the error will be re-raised as socket.timeout so it is handled appropriately by the connection.

This issue was discovered when using Redis as a result backend for Celery. When celery was waiting for results in Redis, its connection would time out causing the operation to fail. This only occurred when using secure connections; it works fine with non-secure connections.

The ssl module in Python 2.7 raises timeouts as ssl.SSLError instead of
socket.timeout. When these timeouts are encountered, the error will be
re-raised as socket.timeout so it is handled appropriately by the
connection.

Signed-off-by: Zac Bristow <zbristow@codeaurora.org>
@andymccurdy
Copy link
Contributor

Do we also need to wrap socket.connect? Establishing a connection can raise a socket.timeout error before recv is ever called. What exception is raised when establishing a secure connection trips the socket_timeout timeout?

The ssl module in Python 2.7 raises ssl.SSLError for timeouts in other
socket operations, too. Those operations used by redis.connection will
be wrapped to raise the correct socket.timeout exception.

Signed-off-by: Zac Bristow <zbristow@codeaurora.org>
@zbristow
Copy link
Contributor Author

Do we also need to wrap socket.connect? Establishing a connection can raise a socket.timeout error before recv is ever called. What exception is raised when establishing a secure connection trips the socket_timeout timeout?

I tested this out using netcat to force a timeout and sure enough, an ssl.SSLError timeout was raised during the handshake.

I decided to find all the possible "timed out" error messages in the ssl module and updated the wrapper to raise socket.timeout when it encountered one of the messages. I then added some additional compatibility wrappers to catch all the other paths.

I didn't want to amend the existing commit with these changes without checking if that was acceptable; let me know if you'd want me to do that.

@andymccurdy
Copy link
Contributor

Awesome, thanks. This looks great. I've merged and released 3.3.9 to PyPI.

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.

None yet

2 participants