miguelgrinberg / simple-websocket

Simple WebSocket server and client for Python.
MIT License
79 stars 17 forks source link

Handle disconnects #1

Closed pirumpi closed 3 years ago

pirumpi commented 3 years ago

Closing the websocket on the clients triggers an exception that doesn't terminate the connection. Let terminate that connection 👌

pirumpi commented 3 years ago

You are right, it most be a better way to handle clients closing the socket connection, I will need more time reviewing the life cycles.

On Fri, Apr 30, 2021 at 3:19 AM Miguel Grinberg @.***> wrote:

@.**** commented on this pull request.

In src/simple_websocket/ws.py https://github.com/miguelgrinberg/simple-websocket/pull/1#discussion_r623737195 :

  • elif isinstance(event, BytesMessage):
  • self.input_buffer.append(event.data)
  • self.event.set()
  • if out_data:
  • self.sock.send(out_data)
  • return keep_going
  • elif isinstance(event, TextMessage):
  • self.input_buffer.append(event.data)
  • self.event.set()
  • elif isinstance(event, BytesMessage):
  • self.input_buffer.append(event.data)
  • self.event.set()
  • if out_data:
  • self.sock.send(out_data)
  • return keep_going
  • except:

This is not a good idea. You can't just catch and silence every possible exception.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/miguelgrinberg/simple-websocket/pull/1#pullrequestreview-649034837, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARG7HHBB6XBD34G4P775N3TLJY2JANCNFSM433FSWIQ .

pirumpi commented 3 years ago

What would be the proper way to close a socket connection from the server side? Right now I'm just calling

sock.close(1006, 'Unauthorized Connection')

But at that point the code is trying to handle the CloseConnection event by trying to send through the same socket

image

That sequence throw the following exception:

    self.run()
  File "/Users/carlosmartin/opt/anaconda3/envs/cuss-epip/lib/python3.8/threading.py", line 870, in run
    self._target(*self._args, **self._kwargs)
  File "/Users/carlosmartin/opt/anaconda3/envs/cuss-epip/lib/python3.8/site-packages/flask_sock/ws.py", line 88, in _thread
    self.connected = self._handle_events()
  File "/Users/carlosmartin/opt/anaconda3/envs/cuss-epip/lib/python3.8/site-packages/flask_sock/ws.py", line 97, in _handle_events
    out_data += self.ws.send(event.response())
  File "/Users/carlosmartin/opt/anaconda3/envs/cuss-epip/lib/python3.8/site-packages/wsproto/__init__.py", line 62, in send
    data += self.connection.send(event)
  File "/Users/carlosmartin/opt/anaconda3/envs/cuss-epip/lib/python3.8/site-packages/wsproto/connection.py", line 99, in send
    raise LocalProtocolError(
wsproto.utilities.LocalProtocolError: Connection cannot be closed in state ConnectionState.CLOSED
miguelgrinberg commented 3 years ago

The piece of code that you copied above is the code that handles a close event that comes from the client. It is not the code that handles a close event issued from the server side. At least that is not the intention.

How do I reproduce this? Do you have a complete server and client I can use?

pirumpi commented 3 years ago

This code should reproduce the error

from flask import Flask
from flask_sock import Sock
from threading import Timer, Thread
from time import sleep

app = Flask(__name__)
sock = Sock(app)

class Client():
    def __init__(self):
        self.clients = []

    def add_client(self, ws):
        self.clients.append(ws)
        Timer(10, self.close_connection, (ws,)).start()
        Thread(target=self.check_disconnect, args=(ws,)).start()

    def echo_msg(self, ws):
        data = ws.receive()
        ws.send(data)

    def check_disconnect(self, ws):
        try:
            while ws.connected:
                print("Is connected")
                sleep(5)
            print("Socket Closed forever")
            self.clients.remove(ws)
        except Exception as e:
            print(e)

    def close_connection(self, ws):
        ws.close(1006, "Invalid Message")

client = Client()

@sock.route('/')
def index(ws):
    client.add_client(ws)
    while True:
        client.echo_msg(ws)
    print("Client disconnected")

def main():
    app.run()

if __name__ == "__main__":
    main()
miguelgrinberg commented 3 years ago

You have this in your route function:

    while True:
        client.echo_msg(ws)

When your timer hits and you close the websocket, the while-loop above will still continue to write. This is a bad design, if you are going to close the socket you should first notify this loop that it needs to exit. In fact, exiting the loop is a much cleaner way to close the websocket than calling disconnect on a separate thread.

pirumpi commented 3 years ago

Ok, I'll change the code to use another reference for the while loop. But how would I pass a custom reason to the closing event?

pirumpi commented 3 years ago

I tried this, but keep getting the same error

from flask import Flask
from flask_sock import Sock
from threading import Timer, Thread
from time import sleep

app = Flask(__name__)
sock = Sock(app)

class Client():
    def __init__(self):
        self.clients = []

    def add_client(self, ws):
        self.clients.append(ws)
        Timer(10, self.close_connection, (ws,)).start()
        Thread(target=self.check_disconnect, args=(ws,)).start()

    def echo_msg(self, ws):
        data = ws.receive()
        ws.send(data)

    def active_client(self, ws):
        if ws in self.clients:
            return True
        return False

    def check_disconnect(self, ws):
        try:
            while ws.connected:
                print("Is connected")
                sleep(5)
            print("Socket Closed forever")
        except Exception as e:
            print(e)

    def close_connection(self, ws):
        self.clients.remove(ws)
        ws.close(1006, "Invalid Message")

client = Client()

@sock.route('/')
def index(ws):
    client.add_client(ws)
    while client.active_client(ws):
        client.echo_msg(ws)
    print("Client disconnected")

def main():
    app.run()

if __name__ == "__main__":
    main()

Also the code won't continue until the ws.receive call is completed right? Not sure how to close the socket without calling ws.close method

miguelgrinberg commented 3 years ago

Try this:

from flask import Flask
from flask_sock import Sock
from threading import Timer, Thread
from time import sleep

app = Flask(__name__)
sock = Sock(app)

class Client():
    def __init__(self):
        self.clients = []

    def add_client(self, ws):
        self.clients.append(ws)
        Timer(10, self.close_connection, (ws,)).start()
        Thread(target=self.check_disconnect, args=(ws,)).start()

    def echo_msg(self, ws):
        data = ws.receive()
        ws.send(data)

    def active_client(self, ws):
        if ws in self.clients:
            return True
        return False

    def check_disconnect(self, ws):
        try:
            while ws.connected:
                print("Is connected")
                sleep(5)
            print("Socket Closed forever")
        except Exception as e:
            print(e)

    def close_connection(self, ws):
        self.clients.remove(ws)

client = Client()

@sock.route('/')
def index(ws):
    client.add_client(ws)
    while client.active_client(ws):
        client.echo_msg(ws)
    ws.close(1006, "Invalid Message")
    print("Client disconnected")

def main():
    app.run()

if __name__ == "__main__":
    main()
pirumpi commented 3 years ago

The issue with that is that ws.receive is a blocking call, so even after removing the socket from the client list, that expression doesn't get executed

while client.active_client(ws):
       client.echo_msg(ws) # <-- this method is blocking, and it won't allowed the rest to execute
ws.close(1006, "Invalid Message") # <-- this never gets call
pirumpi commented 3 years ago

I was trying to get it running on repl.it but flask_sock doesn't install with poetry https://replit.com/@pirumpi/Flask-Sock#main.py

miguelgrinberg commented 3 years ago

I missed the receive() call, okay, I see the problem now.

Two options I can think of:

  1. the receive() function accepts a timeout argument. That allows you to make this function non-blocking.
  2. If you keep calling the disconnect from another thread, then I expect the blocked receive() call in the main thread to raise an exception. So you have to catch the exception there and handle it by returning from your route.
pirumpi commented 3 years ago

I like the timout idea, which I implemented with the other really useful suggestions. Now I'm getting the following exception

Exception in thread Thread-2:
Traceback (most recent call last):
  File "C:\ProgramData\Anaconda3\lib\threading.py", line 932, in _bootstrap_inner
Is connected
    self.run()
  File "C:\ProgramData\Anaconda3\lib\threading.py", line 870, in run
    self._target(*self._args, **self._kwargs)
  File "C:\ProgramData\Anaconda3\lib\site-packages\flask_sock\ws.py", line 83, in _thread
    in_data = self.stream.recv(self.receive_bytes)
OSError: [WinError 10038] An operation was attempted on something that is not a socket

And this is the latest implementation

from flask import Flask
from flask_sock import Sock
from threading import Timer, Thread
from time import sleep

app = Flask(__name__)
sock = Sock(app)

class Client():
    def __init__(self):
        self.clients = []

    def add_client(self, ws):
        self.clients.append(ws)
        Timer(10, self.close_connection, (ws,)).start()
        Thread(target=self.check_disconnect, args=(ws,)).start()

    def echo_msg(self, ws):
        data = ws.receive(timeout=10)
        ws.send(data)

    def active_client(self, ws):
        if ws in self.clients:
            return True
        return False

    def check_disconnect(self, ws):
        try:
            while ws.connected:
                print("Is connected")
                sleep(5)
            print("Socket Closed forever")
        except Exception as e:
            print(e)

    def close_connection(self, ws):
        print("Closing connection")
        self.clients.remove(ws)

client = Client()

@sock.route('/')
def index(ws):
    client.add_client(ws)
    while client.active_client(ws):
        client.echo_msg(ws)
    ws.close(1006, "Invalid Message")
    print("Client disconnected")

def main():
    app.run()

if __name__ == "__main__":
    main()
miguelgrinberg commented 3 years ago

I don't understand why you get this exception. Line 83 is inside a try/except block, and the OSError exception that you are getting is one of the exceptions I'm catching. Are you running on a modified version of this package by any chance?

pirumpi commented 3 years ago

Nope, I'm running an unmodified version of the code. I think the exception is not getting catch by the following exception catch block

 while self.connected:
            try:
                in_data = self.stream.recv(self.receive_bytes)
            except ConnectionResetError:
                self.connected = False
                break
            except Exception as ex:
                template = "An exception of type {0} occurred. Arguments:\n{1!r}"
                message = template.format(type(ex).__name__, ex.args)
                print(message)
            self.ws.receive_data(in_data)
            self.connected = self._handle_events()
miguelgrinberg commented 3 years ago

@pirumpi the code that you are showing me does not look at all like the code that is in this repository, so you do seem to be running on modified code. This is what the code should look like:

        while self.connected:
            try:
                in_data = self.sock.recv(self.receive_bytes)
            except (OSError, ConnectionResetError):
                self.connected = False
                self.event.set()
                break
            self.ws.receive_data(in_data)
            self.connected = self._handle_events()

Try running flask install --upgrade flask-sock simple-websocket to see if that clears the problem for you.

pirumpi commented 3 years ago

I just install on a new environment, and the latest code doesn't not have the OSError on the except section. Maybe it is not published yet?

miguelgrinberg commented 3 years ago

@pirumpi Okay, I think I figured it out. The problem that you have is not with this package, it is with Flask-Sock. I was assuming your issue was here because you wrote in this repo, but your problem is with the other package. Please upgrade Flask-Sock to 0.3.0 and let me know if that helps.

pirumpi commented 3 years ago

The latest update works great! thanks for the hard work 👍 You should have a support author Paypal button or something, you are on top of every PR

miguelgrinberg commented 3 years ago

@pirumpi I did not enable contributions on this repo yet, but if you feel inclined: https://github.com/miguelgrinberg/flask-socketio. There is a sponsor link in that one, and a few others. Thanks!