Skip to content

Wakeup the waiter with exception if handshake fails #308

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

Merged
merged 1 commit into from
Mar 24, 2020
Merged

Wakeup the waiter with exception if handshake fails #308

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions uvloop/sslproto.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,7 @@ cdef class SSLProtocol:
else:
msg = 'SSL handshake failed'
self._fatal_error(exc, msg)
Copy link
Member

Choose a reason for hiding this comment

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

@asvetlov Hi Andrew! I revisited this one today and tried to write a test - looks like in _fatal_error:

https://github.com/python/cpython/blob/7b3ab5921fa25ed8b97b6296f97c5c78aacf5447/Lib/asyncio/selector_events.py#L711-L721

It will call connection_lost(), which will eventually call _wakeup_waiter():

https://github.com/asvetlov/uvloop/blob/133ce7c81882958056da4a4b908e4704f8f0e31b/uvloop/sslproto.pyx#L324-L349

I tried to send EOF during handshake and couldn't reproduce the hanging situation - I've also tried to remove this _wakeup_waiter() call here in python/cpython#17975 and all asyncio-related tests passed. I'm worried that I might have missed something here, could you please shed some light?

Copy link
Member

Choose a reason for hiding this comment

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

@fantix I think we can pick up this PR and finish it, if @asvetlov isn't available. Or merge it as is if it's correct and add small touchups/tests in a follow up PR.

Copy link
Member

Choose a reason for hiding this comment

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

@1st1 I think we can merge this PR without further tests, because:

  • I don't think this is a bug, because we are always closing the underlying transport, that will trigger SSLProtocol.connection_lost and wakeup the waiter.
  • And the only case when this would hang is that the underlying transport didn't schedule a connection_lost() call on close. But in uvloop we hard-coded the transport to be one of TCPTransport, UnixTransport or SSLTransport, and the user would have to use Cython to create a buggy transport subclass to reproduce this issue. (I tried in CPython as a unit test but it seemed not quite possible)
  • I think making an additional wakeup is harmless, and it may save some time by triggering the waiter early. Therefore, I think we could merge this.

self._wakeup_waiter(exc)
return

if self._loop.get_debug():
Expand Down