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 SSLError event processing. #501

Closed
wants to merge 2 commits into from
Closed

Conversation

kherrala
Copy link
Contributor

Do not try to use IOLoop to handle socket events.
Poll for events using select.
Catch socket timeout exception.
Remove unused class variable DO_HANDSHAKE.

Poll for events using select.
Catch socket timeout exception.
Do not try to use IOLoop to handle socket events.
Remove unused class variable DO_HANDSHAKE.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.13%) when pulling 8dab34a on kherrala:master into f8c263f on pika:master.

@kherrala
Copy link
Contributor Author

It seems there's another bug handling ssl.SSL_ERROR_WANT_READ in _handler_error(). Current behaviour is to disconnect immediately. I seem to be having all sorts of strange disconnect behaviour running on weak 3G connection.

Update IOLoop event handler on SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE.
Update event_state flags only if SSLError does not occur.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) when pulling c3f390c on kherrala:master into e27b537 on pika:master.

@kherrala
Copy link
Contributor Author

Sure if you can make it work that way, but the current behaviour is to loop eternally doing the handshake and never return control back to running the IOLoop. The other problem was not catching socket exceptions during handshake, which is not handled well by the caller.

@kherrala kherrala changed the title Fix SSL handshake event processing. Fix SSLError event processing. Sep 10, 2014
@kherrala
Copy link
Contributor Author

kherrala commented May 4, 2015

Any plans getting this to master? It has been working fine for me. Using standard select() during initial handshake doesn't seem like a big issue to me.

@vitaly-krugl
Copy link
Member

Using select() here explicitly will interfere with the native framework's (e.g., Tornado) concurrency model. ***But the major issue with forcing the use of select() here is that it has serious limitations with respect to max socket file descriptor number; so, this would cause failures (or require changes) in programs that create many thousands of sockets and other file descriptor-based objects. poll/epoll/etc. don't have that problem.

@gmr
Copy link
Member

gmr commented May 20, 2015

Closing due to not using native adapters for behavior which seems wrong. Thanks for your patch. If you want to (and it's still needed), please resubmit without the select based implementation, based upon current master.

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

4 participants