Improve handling of socket errors #221
Conversation
- Use the default connection timeout set by socket.setdefaulttimeout(). * I.e, don't specify one, until there is [an API][1] for timeouts, it's probably better for libraries to be able to set the socket timeout outside of hyper via [setdefaulttimeout][2] - Updates error handling to simulate blocking behaviour when the socket is in timeout mode (which is internally a kind of non-blocking mode). Specifically, during reads, add * handling of specific ssl.SSLErrors WANT_READ and WANT_WRITE * handling of errno.EAGAIN and errno.EINTR [1]: python-hyper#187 [2]: https://docs.python.org/2/library/socket.html#socket.setdefaulttimeout
tbetbetbe
force-pushed
the
development-ssl-connection-error-reduction
branch
from
aa18b56 to
1d1d981
Compare
|
Firstly, thanks @tbetbetbe! This is a good set of changes. I have some concerns about the branch for ssl errors though. Specifically, I'm concerned about repeatedly looping on Does the python ssl module do something to avoid that being a risk? |
Not that I'm aware of. The PR as is addresses a set of sporadic but consistent test failures I was seeing in a library I'm working on that's based on hyper. ssl.SSL_ERROR_WANT_WRITE should only get raised if something is writing to the socket when it's in timeout mode. I believe it's common to treat it as a transient failure here, on the assumption that any concurrent writer seeing that transient failure will handle it by continuing. I.e, the approach is effective if the potential readers and writer of the socket are all handling the transient errors in the same way. Because of that, it's may be a good idea to replace the call to sendall with a loop around send that does the same kind of handling. At the moment, if sendall fails with a transient SSL error, recovery will not be possible as we don't know how much data was written. Another approach is to place a timeout on the loop as in ssl_compat I'd be happy to either
Any of these are OK, as long the tests failures I was seeing remain resolved. Which approach do you prefer ? |
|
I think just merging this is fine: consider me swayed by your argument. =D |
|
If I'm not mistaken, this PR has introduced a NameError in the code - please observe line 545 in Thanks goes to pylint for warning about this. :) |
|
Yup, sorry that's correct. That should be |
|
Thanks both @matangover and @tbetbetbe! Some day this week I'll be cleaning up the linter errors in this project and enforcing flake8 compliance in the CI suite: consider yourselves warned. ;) |
|
@tbetbetbe There's no need for a follow-up PR: I've started work on the linting today, and so I'll be sweeping it up anyway. =) |
|
See #226. |
probably better for libraries to be able to set the socket timeout
outside of hyper via setdefaulttimeout
is in timeout mode (which is internally a kind of non-blocking
mode). Specifically, during reads, add