Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Catch thrown OSError by python 3.7 when creating a connection #1694

Merged
merged 1 commit into from
Mar 13, 2019

Conversation

danjo133
Copy link
Contributor

@danjo133 danjo133 commented Jan 8, 2019

This is needed to use kafka-python with python 3.7.

It builds on #1669 and the solution is a slightly modified version of a comment on #1661


This change is Reviewable

@danjo133
Copy link
Contributor Author

danjo133 commented Jan 8, 2019

Should I update tox.ini, setup.py, .travis.yml as well to add Python 3.7 support/testing?

@dpkp
Copy link
Owner

dpkp commented Jan 13, 2019

python bug report here: https://bugs.python.org/issue31122

It seems like wrap_socket() could also raise OSError?

@danjo133
Copy link
Contributor Author

So, that gets raised when a socket disconnects? I would say that is another error. This PR is for OSError that is raised when trying to connect normally. I can add a catch for 31122 as well if you want though. How do you want it handled in that case? should it raise SSLEOFError or set errno to something like ENOTCONN ? If I understand the bug report, then if you get hit by that, then your connection has been closed and you need to try to reconnect in some way, which I guess should be up to the user.

@danjo133
Copy link
Contributor Author

@dpkp Do you want me to do anything more/different with this PR?

@dpkp
Copy link
Owner

dpkp commented Mar 13, 2019

We've finally got python 3.7 automated testing working on travis, so I think we're good to go. Would love more test coverage wrt SSL connections though.

@dpkp dpkp merged commit 1904b53 into dpkp:master Mar 13, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants