Skip to content

SSLProtocol violates app Protocol state machine #246

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

Jump to bottom
Closed
jeremy-hiatt opened this issue Apr 16, 2019 · 6 comments · Fixed by #263 or j-usti-n/uvloop#2
Closed

SSLProtocol violates app Protocol state machine #246

jeremy-hiatt opened this issue Apr 16, 2019 · 6 comments · Fixed by #263 or j-usti-n/uvloop#2
Assignees
@fantix

Comments

@jeremy-hiatt
Copy link

jeremy-hiatt commented Apr 16, 2019 •
edited
Loading

  • uvloop version: 0.12.2
  • Python version: 3.5.4
  • Platform: Mac OS X
  • Can you reproduce the bug with PYTHONASYNCIODEBUG in env?: Yes

Some instances of the Protocol class assume connection_lost() can only be called if connection_made() was called at some point. This does not appear to be explicitly guaranteed in the Python documentation, but the following comment strongly suggests that intent:
https://github.com/python/cpython/blob/master/Lib/asyncio/protocols.py#L84

An error raised during the handshake phase triggers a transition back to UNWRAPPED:
https://github.com/MagicStack/uvloop/blob/master/uvloop/sslproto.pyx#L504
Following this code path further, we see that it will force close the transport, which triggers a call back to SSLProtocol.connection_lost(). This is then forwarded onto the app's Protocol instance, even though connection_made() was never called.

Would it be appropriate to change the following guard to also exclude the UNWRAPPED state?
https://github.com/MagicStack/uvloop/blob/master/uvloop/sslproto.pyx#L337
Alternatively a boolean value could be used to track whether or not connection_made() has ever been called.
@jeremy-hiatt jeremy-hiatt changed the title SSLProto violates Protocol state machine SSLProtocol violates app Protocol state machine Apr 16, 2019
@1st1
Copy link
Member

1st1 commented Apr 16, 2019

@fantix any thoughts?

@fantix
Copy link
Member

fantix commented Apr 16, 2019 •
edited
Loading

Yes, it makes perfect sense to honor the CM -> ... -> CL state machine, I don't see a reason why not to. Thanks for the very detailed report and 2 proposals! I think the boolean value is the right way to go - WRAPPED, FLUSHING or SHUTDOWN may transit to UNWRAPPED before connection_lost() is called.

kyuupichan referenced this issue in kyuupichan/aiorpcX Apr 19, 2019
Prepare 0.16.0
b53058f 
* session closing is now robust; it is safe to await session.close() from anywhere
* API change: FinalRPCError removed; raise ReplyAndDisconnect instead.  This responds with
  a normal result, or an error, and then disconnects.
  e.g.  raise ReplyAndDisconnect(23)
        raise ReplyAndDisconnect(RPCError(1, "message"))
* the session base class' private method _close() is removed.  Use await close() instead.
* workaround uvloop bug `https://github.com/MagicStack/uvloop/issues/246`_
@1st1
Copy link
Member

1st1 commented Apr 22, 2019

@fantix Do you plan to implement this yourself?

@fantix
Copy link
Member

fantix commented Apr 22, 2019

Yes! I'll create a PR for that.

@fantix fantix self-assigned this Apr 29, 2019
@1st1
Copy link
Member

1st1 commented Aug 14, 2019

@fantix ping

@fantix
Copy link
Member

fantix commented Aug 14, 2019

Ahhh that slipped through the cracks, let me make one PR now.

fantix added a commit to fantix/uvloop that referenced this issue Aug 14, 2019
@fantix
add app state check
a555e57 
* fixes MagicStack#246
@fantix fantix mentioned this issue Aug 14, 2019
Merged
1 task
fantix added a commit that referenced this issue Aug 24, 2019
@fantix
add app state check (#263)
82104fb 
* add app state check

* fixes #246

* add test to cover the case

* CRF: rename states and fix comment
@j-usti-n j-usti-n mentioned this issue Aug 24, 2019
Merged
@aaliddell aaliddell mentioned this issue Dec 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fantix fantix

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

add app state check fantix/uvloop
add app state check (#263) MagicStack/uvloop
3 participants
@1st1 @jeremy-hiatt @fantix