Closed PierreRust closed 3 years ago
I just stumbled across this as well. the declaration of the client storage needs to be moved from the class level into the constructor. I personally agree with @PierreRust that this should be fixed.
@PierreRust, I think you should do a PR for it, Took me a while to find the issue. I thought it was something in my code (20K + lines). I kept on thinking I was making a mistake somewhere. 2 days of hunting and I finally checked the library, took me a while to see it. I wasn't looking for that to be the issue.
This is a great lib. does the job and isn't bloated.
I did want to post a simple fix in case anyone else happens across this issue. This should do the trick. put this into a file in your script and import it before you import the websocket_server library. this should monkey patch the library to work properly.
import sys
import websocket_server
from websocket_server import WebsocketServer
del WebsocketServer.clients
_old_websocket_server = WebsocketServer
class WebSocket(WebsocketServer):
__doc__ = _old_websocket_server.__doc__
def __init__(
self,
port,
host='127.0.0.1',
loglevel=websocket_server.logging.WARNING
):
self.clients = []
WebsocketServer.__init__(self, port, host, loglevel)
sys.modules['websocket_server'].WebsocketServer = WebSocket
Do we want this to be changed to having a send_all_global and a send_all_handler? This is a glaring issue that has caused me and some friends quite a few bugs and I'm more then willing to implement a fix once we have a proper direction.
you could have a class method something like send_all_sockets and a method that is send_all_clients. have a class level list for all of the socket instances. and the class method would simply iterate over the sockets and call the send_all_clients.
the issue is the class level clients container. every client connection made gets put into it for every websocket instance made. so if you call the send_all when it iterates over the clients container it is sending to clients that are not apart of the websocket instance. ya end up having data go where ya may not want it.
I am not really 100% sure what the reasoning would be to use a specific websocket instance and be able to access the clients from a completely different websocket instance. But there may be a need by some. Hence the reason for making a class method and also an instance method.
I want to tinker with an idea that may satisfy both needs.
all code blocks are pseudo code. They have not been tested specifically in the websocket_server library
OK so this an idea that works in a test setup of simple class instances and it should as well in the websocket_server lib.
class API():
def run_forever(self):
try:
logger.info("Listening on port %d for clients.." % self.port)
self.serve_forever()
except KeyboardInterrupt:
self.server_close()
logger.info("Server terminated.")
except Exception as e:
logger.error(str(e), exc_info=True)
exit(1)
def new_client(self, client, server):
pass
def client_left(self, client, server):
pass
def message_received(self, client, server, message):
pass
def set_fn_new_client(self, fn):
self.new_client = fn
def set_fn_client_left(self, fn):
self.client_left = fn
def set_fn_message_received(self, fn):
self.message_received = fn
def send_message(self, client, msg):
self._unicast_(client, msg)
@classmethod
def send_message_to_all(cls, msg):
self._multicast_(msg)
class WebsocketServer(ThreadingMixIn, TCPServer, API):
allow_reuse_address = True
daemon_threads = True # comment to keep threads alive until finished
clients = []
id_counter = 0
def __init__(self, port, host='127.0.0.1', loglevel=logging.WARNING):
self.send_message_to_all = self.__send_message_to_all
logger.setLevel(loglevel)
TCPServer.__init__(self, (host, port), WebSocketHandler)
self.port = self.socket.getsockname()[1]
def __send_message_to_all(self, msg):
for client in self.clients:
if client['server'] == self:
self._unicast_(client, msg)
def _message_received_(self, handler, msg):
self.message_received(self.handler_to_client(handler), self, msg)
def _ping_received_(self, handler, msg):
handler.send_pong(msg)
def _pong_received_(self, handler, msg):
pass
def _new_client_(self, handler):
self.id_counter += 1
client = {
'id': self.id_counter,
'handler': handler,
'address': handler.client_address,
'server': self
}
self.clients.append(client)
self.new_client(client, self)
def _client_left_(self, handler):
client = self.handler_to_client(handler)
self.client_left(client, self)
if client in self.clients:
self.clients.remove(client)
def _unicast_(self, to_client, msg):
to_client['handler'].send_message(msg)
def _multicast_(self, msg):
for client in self.clients:
self._unicast_(client, msg)
def handler_to_client(self, handler):
for client in self.clients:
if client['handler'] == handler:
return client
Now what i did here was I had the instance override the class method of the parent class. This way it can be used both ways.
from websocket_server import WebsocketServer
instance1 = WebsocketServer(1234)
instance2 = WebsocketServer(4321)
instance1.send_message_to_all('message sent to only instance1 clients')
WebsocketServer.send_message_to_all('message sent to all clients on all websocket servers')
I think this would be the most elegant solution. and the one that makes the most sense.
being able to access one instance of a websocket server from another instance I do not believe is the best idea. You may even consider changing the clients container to have a mangled name. Then add a property that will iterate the mangled name container and only return clients that pertain to it.
In the original code there was no way to tell what client belong to what websocket connection even if you iterated over the devices container. No marker that was easily had. Without really digging into it but I think that client['handler'].server might point back to the websocket instance, but I am not sure. Event if you do not make clients a mangled name at least make it a private one.
This adds the mangled name which offers protection against inadvertently sending data to the wrong websocket connection if accessed from a websocket instance.
class WebsocketServer(ThreadingMixIn, TCPServer, API):
allow_reuse_address = True
daemon_threads = True # comment to keep threads alive until finished
__clients = []
id_counter = 0
def __init__(self, port, host='127.0.0.1', loglevel=logging.WARNING):
self.send_message_to_all = self.__send_message_to_all
logger.setLevel(loglevel)
TCPServer.__init__(self, (host, port), WebSocketHandler)
self.port = self.socket.getsockname()[1]
@property
def clients(self):
return list(
client for client in self.__clients
if client['server'] == self
)
def __send_message_to_all(self, msg):
for client in self.clients:
self._unicast_(client, msg)
def _message_received_(self, handler, msg):
self.message_received(self.handler_to_client(handler), self, msg)
def _ping_received_(self, handler, msg):
handler.send_pong(msg)
def _pong_received_(self, handler, msg):
pass
def _new_client_(self, handler):
self.id_counter += 1
client = {
'id': self.id_counter,
'handler': handler,
'address': handler.client_address,
'server': self
}
self.__clients.append(client)
self.new_client(client, self)
def _client_left_(self, handler):
client = self.handler_to_client(handler)
self.client_left(client, self)
if client in self.__clients:
self.__clients.remove(client)
def _unicast_(self, to_client, msg):
to_client['handler'].send_message(msg)
def _multicast_(self, msg):
for client in self.__clients:
self._unicast_(client, msg)
def handler_to_client(self, handler):
for client in self.__clients:
if client['handler'] == handler:
return client
I mean the simple and (IMO) correct solution is to just move the clients declaration inside init.
class WebsocketServer(ThreadingMixIn, TCPServer, API):
allow_reuse_address = True
daemon_threads = True # comment to keep threads alive until finished
id_counter = 0
def __init__(self, port, host='127.0.0.1', loglevel=logging.WARNING):
self.clients = []
logger.setLevel(loglevel)
TCPServer.__init__(self, (host, port), WebSocketHandler)
This makes it instance specific which makes it act as one would expect by default, In the vain of keeping backwards compatibility however....
Yeah it is. But i do not know if the placement of clients was intentional.
This has been an issue for a long while and I do not know if the original author is still supporting it or not. So we do not know the original design and purpose.
there are a bunch of ????'s on that portion of it.
I did submit a PR for the modified version of the code above. There are errors in the code above. so it was for example purposes as stated.
So the original design simply dealt with a single instance. But all this makes sense so v0.5.1
should have this fixed.
In
WebsocketServer
class, theclients
attribute is defined at the class level, not as an instance attribute. Is there any good reason for this design?This means that when you use several
WebsocketServer
(on different ports) and use thesend_message_to_all
method on a server instance, the message is sent to all clients, including the clients that connected to another server !The fix is trivial and I can make a Pull Request if this could be changed.
Thanks for this very useful lib BTW ! :smile: