galacticpuzzlehunt / gph-site

Django app for running a puzzlehunt (open-source version)
MIT License
62 stars 31 forks source link

Code in websocket consumer causes logged out clients to repeatedly get 500 errors #32

Closed enigmathix closed 1 year ago

enigmathix commented 1 year ago

The code in puzzles/messaging.py below makes logged out clients keep trying to connect their websocket to the server and get a 500 error because the connection times out with no response. The reason is is_ok() checks that the user is authenticated before accepting the connection. If the user is logged out and their javascript is still running, it keeps trying to connect and that looks pretty bad in the server logs as it looks like a lot of requests get a 500 error (because websocket connections are frequent).

class BroadcastWebsocketConsumer(WebsocketConsumer):
    def connect(self):
        if not self.is_ok(): return
    (...)
    self.accept()

I haven't looked at the conditions that leads to this situation in the client, but from a server perspective, it would be more elegant to accept the connection request, even if there's no data to send, e.g.:

class BroadcastWebsocketConsumer(WebsocketConsumer):
    def connect(self):
        if not self.is_ok():
            self.accept()
            return

There might be other solutions but that's the simplest I could find, and it keeps the server logs clean. I don't think it has negative effects on the client since they're logged out anyway.

cesium12 commented 1 year ago

The solution taken in the repo is to not try to open the socket at all from the client side if there is no team (see the script in base.html), as (like you noted) there isn't any point in doing so. If there's a bug in that setup, it would be good to fix, but IME it works fine. Otherwise, making this change seems okay just to be extra safe, so I'd be happy to take a PR.

enigmathix commented 1 year ago

I agree the base.html is supposed to take care of this case, but it has happened on a live hunt (Verwald's Treasure) with just one user causing the server logs to look awful.