miguelgrinberg / flask-sock

Modern WebSocket support for Flask.
MIT License
274 stars 24 forks source link

ws methods called on a ws that is already closed close current connection silently #12

Closed aentwist closed 2 years ago

aentwist commented 2 years ago

I am trying to keep track of connected clients and send messages to them. I connect with one client, store its websocket in memory, and then disconnect. I then connect with another client and try to interact with the websocket of the previous client that is no longer open.

When receive or send are called on a websocket that is already closed, the current connection closes with 1000 Normal Closure. What I would expect is an exception to be thrown so that I can catch it, remove the dead websocket object, and continue on with the current websocket.

I have included code to reproduce this issue and example output.

app.py

clients = []

@sock.route("/test")
def test(ws):
    global clients

    clients.append(ws)
    print(clients, flush=True)

    while True:
        print("Waiting to receive data...", flush=True)
        data = clients[0].receive()
        print("...done", flush=True)
        clients[0].send(data)

Client

$ wscat --connect 'ws://localhost:5000/test'
Connected (press CTRL+C to quit)
> hello, world
< hello, world
> $ ^C
$ wscat --connect 'ws://localhost:5000/test'
Connected (press CTRL+C to quit)
Disconnected (code: 1000, reason: "")
$ 

STDOUT

api_1  | [<simple_websocket.ws.Server object at 0x7fc2af10e640>]
api_1  | Waiting to receive data...
api_1  | ...done
api_1  | Waiting to receive data...
api_1  | [<simple_websocket.ws.Server object at 0x7fc2af10e640>, <simple_websocket.ws.Server object at 0x7fc2af17a610>]
api_1  | Waiting to receive data...
api_1  | 172.18.0.1 - - [03/Oct/2021 01:39:55] "GET /test HTTP/1.1" 200 -
aentwist commented 2 years ago

If there is a LBYL approach to this, that would also work.

miguelgrinberg commented 2 years ago

This is working as designed, it's just that you are doing something that is unusual. Let me try to explain.

The expected use is that within a WebSocket route you will use the ws object that was passed to that route. Under that usage, if the client goes away, this ws object will raise ConnectionClosed. This extension catches this exception and handles it silently by releasing all resources and ending the call. If the application needs to do anything specific at clean up, it can catch ConnectionClosed do what it needs to do, and then re-raise it.

The unusual thing that you are doing is that you are using the ws for client A in the route invocation from client B. This too, raises a ConnectionClosed error, and ends the route, which is what you are seeing. If you want to handle this error yourself, then add a try/except block and catch this exception, and then you have full control on what you want to do there. I hope this makes sense.

aentwist commented 2 years ago

Thanks, I was confused by what was going on behind the scenes. It turns out I might have figured this out by using

try:
    data = clients[0].receive()
    clients[0].send(data)
except Exception as e:
    print(e.__class__, flush=True)

It looks like the built-in catching is done here.


you are doing something that is unusual

Might I ask how you would implement this idea? Should I bring in a messaging queue piece for different ws to talk to each other?


If you want to handle this error yourself

catches this exception and handles it silently by releasing all resources and ending the call

So the resources are already freed when connection A closes, thus I do not need to worry about freeing them when connection B throws this exception for connection A?

aentwist commented 2 years ago

I guess maybe this is where I switch to flask-socketio?

miguelgrinberg commented 2 years ago

Might I ask how you would implement this idea?

Just catch ConnectionClosed in your handlers. If you receive this exception you can remove the ws object that caused it from your clients list and move on.

So the resources are already freed when connection A closes, thus I do not need to worry about freeing them when connection B throws this exception for connection A?

That's correct, in your case receiving a ConnectionClosed when using a ws object from a different client means that you just need to update your client list and keep going. If you receive the exception from the ws object that was given to you in the same handler you are currently running, then it would be a good practice to let the exception bubble up after you do your own handling.

I guess maybe this is where I switch to flask-socketio?

Not sure. Maybe? The event model in Flask-SocketIO may make your usage less confusing/unusual, but I think you can do what you want with this extension, as long as you handle those ConnectionClosed exceptions yourself.

aentwist commented 2 years ago

Thank you again. "less confusing/unusual" is probably a good thing. I will investigate whether the flask-socketio event model has what I am looking for, so as to avoid reinventing the wheel if possible. Otherwise, it is good to know that I am in the clear with this.

The use case is definitely atypical. I am using ws to drive the browser of an embedded device from another client.

aentwist commented 2 years ago

To clarify for future reference, my use case needs 'get client by ID' functionality which requires a mapping of ID to client.

This is certainly possible to implement with flask-sock. You have the connect event. Use this to do clients[id] = ws. The disconnect event can be handled by catching ConnectionClosed, running your code, and then re-throwing it. To maintain the mapping, 'your code' should include del clients[id].

flask-socketio already has this implemented via request.sid and rooms.

The workflow for handling the disconnect event is a good candidate for the docs, either in the example or the API.