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.

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

bridge: call connection_lost "soon" #20881

Merged
merged 2 commits into from
Aug 13, 2024
Merged

bridge: call connection_lost "soon" #20881

merged 2 commits into from
Aug 13, 2024

Commits on Aug 13, 2024

  1. bridge: call connection_lost "soon" 

    The standard Python asyncio Transport classes protect their caller from
having to deal with exceptions when calling .write().  In case an
exception does occur, they capture it and close the transport, passing
the exception on to the connection_lost() handler.  This makes it a lot
easier to interact with the transport: because OS-level errors are
handled out of band, protocols don't have to consider that any write
might fail.

Our Transport classes do the same thing, but there's an important
difference: the standard library defers the call to .connection_lost(),
but our transports invoke it immediately.

This creates a weird scenario whereby your connection_lost() handler can
get called in the middle of your connection_made() handler, if you
should happen to do a .write() there which fails.  Since
connection_lost() reasonably assumes that connection_made() has run, you
can get into all kinds of trouble.  Worst of all, once
.connection_lost() is done running, control flow returns back to your
.connection_made() handler, which continues running as if nothing has
happened in the meantime.

Let's adopt the standard Python behaviour here and defer the
.connection_lost() call.  We need to adjust some unit tests which
assumed the old behaviour: adjust them to ensure that we specifically
*don't* have that behaviour anymore.

This is something that I always knew we did differently than the Python
standard library, but I didn't understand the purpose of the additional
complexity until now.  Thanks goes to Martin for the initial research to
uncover this issue.
    @allisonkarlitskaya
    allisonkarlitskaya committed Aug 13, 2024
    Configuration menu
    Copy the full SHA
    422689c View commit details
    Browse the repository at this point in the history

  2. bridge: add missing ProtocolChannel._transport initializer 

    This is the only thing in the class which isn't initialized and it's
causing issues.  Fix that.

Thanks for tracking this down go to Martin as well :)
    @allisonkarlitskaya
    allisonkarlitskaya committed Aug 13, 2024
    Configuration menu
    Copy the full SHA
    562ae22 View commit details
    Browse the repository at this point in the history