Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ FEATURE ] Add websockets #495

Closed
OmgImAlexis opened this issue Apr 27, 2016 · 12 comments
Closed

[ FEATURE ] Add websockets #495

OmgImAlexis opened this issue Apr 27, 2016 · 12 comments

Comments

@OmgImAlexis
Copy link
Collaborator

If websockets were added then we could use them to update the page as new changes come along instead of having to rely on polling the server for new changes. This would also fix the get_messages spam that we currently have.

@OmgImAlexis OmgImAlexis changed the title [ Feature ] Add websockets [ FEATURE ] Add websockets Apr 27, 2016
@p0psicles
Copy link
Contributor

Do you have some examples? Or possibly docs for implementing it in python? I found this https://pypi.python.org/pypi/websockets, but that's min python 3.3

@OmgImAlexis
Copy link
Collaborator Author

Have a look at the docs for tornado, from what I can see there's already an implementation built in, you just need to use it like we already do with normal http requests.

www.tornadoweb.org/en/stable/websocket.html

@p0psicles
Copy link
Contributor

Ooh cool! I'll see if I can replace the messages with this as a POC

@p0psicles
Copy link
Contributor

How should we create the websocket route? Ive got something working using /ws/(.*). But maybe you also want to make this part of the versioned API?

@OmgImAlexis
Copy link
Collaborator Author

OmgImAlexis commented May 3, 2016

Maybe we just use /ws/ and then in the data we add in what it's for.

Like this and then we can just check if the message we get from the server has the event we need.

{
    "event": "notification",
    "data": {
        "title": "Test",
        "body": "This is a test notification!",
        "type": "message"
    }
}

@labrys
Copy link
Contributor

labrys commented May 3, 2016

@OmgImAlexis That sounds good. If in the future we find that its needed we can extend this to additional routes if needed, possibly even mirroring the APIv2 endpoints for route-specific websockets.

@p0psicles
Copy link
Contributor

hmm just discovered that the ws is opened blocking other requests. Meaning it's running under the Tornado thread. But looping in the open() method like shown below, will not work. Maybe create a new thread specific for checking the ui messages?

class WebSocketUIHandler(WebSocketHandler):
    def check_origin(self, origin):
        return True

    def open(self, *args, **kwargs):
        clients.append(self)
        WebSocketHandler.open(self, *args, **kwargs)
        while True:
            messages = {}
            cur_notification_num = 1
            for cur_notification in notifications.get_notifications(self.request.remote_ip):
                messages['notification-' + str(cur_notification_num)] = {'title': cur_notification.title,
                                                                         'message': cur_notification.message,
                                                                         'type': cur_notification.type}
                cur_notification_num += 1
            if messages:
                self.write_message(messages)
            time.sleep(1)

    def on_message(self, message):
        print(u"Received: " + message)

    def data_received(self, chunk):
        WebSocketHandler.data_received(self, chunk)

    def on_close(self):
        clients.remove(self)

@OmgImAlexis
Copy link
Collaborator Author

@p0psicles I'd say open a new thread just for websockets since it'll need to run independently to the web ui.
This is if a new thread doesn't add more stress to the server?

@p0psicles
Copy link
Contributor

p0psicles commented May 3, 2016

Hmm. For what i've read, I have to remove any heavy processing from the handler, and move it to a separate thread. Meaning that the wshandler will only be called, when something has to be send over the socket. This means I have to at least get rid of the loop.

But the RequestHandler doesn't work like this. That one has a limited number of threads, that I can use for each request. I'd rather also have the websockets work like that.

@OmgImAlexis
Copy link
Collaborator Author

Would something like this not work? Since a websocket connection is kept open you should be able to just send anything through as a message and the browser should be able to receive it.

class WebSocketUIHandler(WebSocketHandler):
    def check_origin(self, origin):
        return True

    def open(self, *args, **kwargs):
        clients.append(self)
        messages = {}
        for cur_notification in notifications.get_notifications(self.request.remote_ip):
        self.write_message({
            'event': cur_notification.type,
            'data': {
                'title': cur_notification.title,
                'message': cur_notification.message
            }
        })

    def on_message(self, message):
        print(u"Received: " + message)

    def data_received(self, chunk):
        WebSocketHandler.data_received(self, chunk)

    def on_close(self):
        clients.remove(self)

@p0psicles
Copy link
Contributor

image

@labrys
Copy link
Contributor

labrys commented May 3, 2017

Added to master feature request list - discussion for feature will continue here even though issue is closed.

@labrys labrys closed this as completed May 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants