sanic icon indicating copy to clipboard operation
sanic copied to clipboard

Implement catchable WebsocketClosed

Open ahopkins opened this issue 2 years ago • 9 comments

See #2220

In v21.9, a new WebsocketClosed exception was added.

While you can to do the following:

@app.websocker("/")
async def handler(request, ws):
    try:
        ...
    except asyncio.CancelledError:
        print("connection closed")

We would like to add support for this pattern:

@app.websocker("/")
async def handler(request, ws):
    try:
        ...
    except sanic.exceptions.WebsocketClosed:
        print("connection closed")

ahopkins avatar Oct 03 '21 13:10 ahopkins

@ahopkins I think there might've been some miscommunication around what the WebsocketClosed exception is for.

In #2248, WebsocketsClosed exception was added as a replacement for throwing sanic.ServerError exceptions if a user tries to read or write to a websocket connection that is closed or is closing. This also prevented the KeepAlive-ping background task from emitting a ServerError needlessly when the websocket connection was closed.

In theory (if the timing it right) it is possible to catch it like this:

@app.websocker("/")
async def handler(request, ws):
    try:
        while(True):
            ws.write("hello") # <-- this could throw WebsocketClosed when the socket closes
    except sanic.exceptions.WebsocketClosed:
        print("connection closed")

However also in #2248, I introduced more capability for the websockets connection to raise asyncio.CancelledError into the handler, to give the handler early warning that the connection is closing, and prevent the handler from continuing to make send() and recv() calls. So this change significantly decreases the possibility of the handler seeing a WebsocketClosed exception, because it would see asyncio.CancelledError first, and break from its handling.

If we really want the ability to catch a websocket-specific exception, rather than asyncio.CancelledError, then I suggest we create a new exception like WebsocketCancelled that subclasses asyncio.CancelledError, and use that instead.

Then the handler could use code like this to cover both:

@app.websocker("/")
async def handler(request, ws):
    try:
        ...
    except (WebsocketClosed, WebsocketCancelled)
        print("connection closed")

ashleysommer avatar Oct 03 '21 23:10 ashleysommer

@ashleysommer What is the distinction between WebsocketClosed and WebsocketCancelled from an app developer standpoint? Looks like it is all down to random chance of socket closing timing. Using any sort of CancelledError for a loss of connection seems overkill, and I would prefer reserving those to timeouts and non-graceful exit.

Tronic avatar Oct 04 '21 06:10 Tronic

The other thing about CancelledError that would be nice to avoid (if possible) is that it trips a lot of people up when they think they can catch it with Exception.

ahopkins avatar Oct 04 '21 06:10 ahopkins

The difference as I see it: WebsocketClosed is an error. It is triggered by trying to do something you cannot do. Writing to a closed socket is not possible, so your action raises that error. Like dividing by zero.

On the other hand, CancelledError is an asyncio exception that can act like it signal, it gets thrown into the handler when we want to notify the handler the connection is going down. It is not triggered by something the user is trying to do, but by the connection itself. Eg, this would also work with CancelledError:

@app.websocker("/")
async def handler(request, ws):
    try:
        await asyncio.sleep(9999)
    except asyncio.CancelledError:
        print("connection closing")

There is no way to do that with a regular exception like WebsocketClosed.

ashleysommer avatar Oct 04 '21 22:10 ashleysommer

I'm also expecting new exception like WebsocketClosed, otherwise it is impossible determine reason of closed connection.

jekel avatar Oct 21 '21 12:10 jekel

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions.

stale[bot] avatar Mar 02 '22 08:03 stale[bot]

...

ahopkins avatar Mar 02 '22 09:03 ahopkins

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions.

stale[bot] avatar Jun 12 '22 21:06 stale[bot]

May be impacted by: https://github.com/sanic-org/sanic/pull/2513

ahopkins avatar Aug 28 '22 07:08 ahopkins