websocket support #469

miguelgrinberg · 2017-02-20T20:28:59Z

Fixes #12

miguelgrinberg · 2017-02-20T20:30:13Z

Let me know if this is something you are open to incorporate into the project.

If you think this is worthwhile, I can take it to the finish line, which means:

add unit tests
add support for ws routes in blueprints

If you have any issues against merging this patch, let me know, and I'll look into releasing this as a separate extension.

Thanks!

seemethere

Overall good, couple of points just about naming and backslashes. This implementation is good and I think once we have some tests written into it we can merge in. I'll make this a part of the 0.4.0 release.

seemethere · 2017-02-20T20:45:44Z

sanic/app.py

@@ -168,6 +170,40 @@ def add_route(self, handler, uri, methods=frozenset({'GET'}), host=None):
        self.route(uri=uri, methods=methods, host=host)(handler)
        return handler

+    # Decorator
+    def ws(self, uri, host=None):


Can we name something other than wsI don't think it's necessarily explicit in what it actually is. I think something like websocket or web_socket would be good here.

seemethere · 2017-02-20T20:46:17Z

sanic/app.py

@@ -459,6 +495,9 @@ def run(self, host="127.0.0.1", port=8000, debug=False, before_start=None,
        :param protocol: Subclass of asyncio protocol class
        :return: Nothing
        """
+        if protocol is None:
+            protocol = WebSocketProtocol if self.needs_websocket \


Prefer parenthesis to backslashes here.

seemethere · 2017-02-20T20:46:51Z

sanic/app.py

                            backlog=100, stop_event=None):
        """Asynchronous version of `run`.

        NOTE: This does not support multiprocessing and is not the preferred
              way to run a Sanic application.
        """
+        if protocol is None:


Prefer parenthesis to backslashes here.

seemethere · 2017-02-20T20:47:54Z

sanic/ws.py

+class WebSocketProtocol(HttpProtocol):
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.ws = None


I would say ws in this instance too should be renamed to websocket or web_socket

+1 for websocket vs web_socket

I'd say the same 👍

miguelgrinberg · 2017-02-20T23:24:15Z

@seemethere @r0fls added a unit test and addressed your comments. The coverage report in tox is weird, coverage is off while the tests run, so the low coverage numbers you get on all the modules is incorrect. I enabled coverage here and measured 87% for the new websocket.py file, and 85% for the entire sanic package. Let me know if you have any more feedback.

jamesstidard · 2017-02-25T00:37:23Z

I've been playing around with this branch and I'm getting an exception raised after a idle websocket client connection (maybe about a minute).

I created a client directly using websockets, here's the gist, which didn't have this problem.

Here's the Sanic version and the stack trace I recieve:

2017-02-25 00:28:17,904: ERROR: NoneType: None

2017-02-25 00:28:17,905: ERROR: Traceback (most recent call last):
  File "/Users/jamesstidard/Development/Miscellany/Hello-Sanic/venv/src/sanic/sanic/app.py", line 431, in handle_request
    response = await response
  File "/usr/local/var/pyenv/versions/3.6.0/lib/python3.6/asyncio/coroutines.py", line 128, in throw
    return self.gen.throw(type, value, traceback)
  File "/Users/jamesstidard/Development/Miscellany/Hello-Sanic/venv/src/sanic/sanic/app.py", line 193, in websocket_handler
    await handler(request, ws, *args, **kwargs)
  File "/usr/local/var/pyenv/versions/3.6.0/lib/python3.6/asyncio/coroutines.py", line 128, in throw
    return self.gen.throw(type, value, traceback)
  File "/Users/jamesstidard/Development/Miscellany/Hello-Sanic/hellosanic/serve.py", line 29, in websocket
    message = await ws.recv()
  File "/Users/jamesstidard/Development/Miscellany/Hello-Sanic/venv/lib/python3.6/site-packages/websockets/protocol.py", line 284, in recv
    loop=self.loop, return_when=asyncio.FIRST_COMPLETED)
  File "/usr/local/var/pyenv/versions/3.6.0/lib/python3.6/asyncio/tasks.py", line 307, in wait
    return (yield from _wait(fs, timeout, return_when, loop))
  File "/usr/local/var/pyenv/versions/3.6.0/lib/python3.6/asyncio/tasks.py", line 390, in _wait
    yield from waiter
concurrent.futures._base.CancelledError

2017-02-25 00:28:17,906: ERROR: Exception occurred while handling uri: "/ws"
Traceback (most recent call last):
  File "/path/to/project/venv/src/sanic/sanic/app.py", line 431, in handle_request
    response = await response
  File "/usr/local/var/pyenv/versions/3.6.0/lib/python3.6/asyncio/coroutines.py", line 128, in throw
    return self.gen.throw(type, value, traceback)
  File "/path/to/project/venv/src/sanic/sanic/app.py", line 193, in websocket_handler
    await handler(request, ws, *args, **kwargs)
  File "/usr/local/var/pyenv/versions/3.6.0/lib/python3.6/asyncio/coroutines.py", line 128, in throw
    return self.gen.throw(type, value, traceback)
  File "/path/to/project/hellosanic/serve.py", line 29, in websocket
    message = await ws.recv()
  File "/path/to/project/venv/lib/python3.6/site-packages/websockets/protocol.py", line 284, in recv
    loop=self.loop, return_when=asyncio.FIRST_COMPLETED)
  File "/usr/local/var/pyenv/versions/3.6.0/lib/python3.6/asyncio/tasks.py", line 307, in wait
    return (yield from _wait(fs, timeout, return_when, loop))
  File "/usr/local/var/pyenv/versions/3.6.0/lib/python3.6/asyncio/tasks.py", line 390, in _wait
    yield from waiter
concurrent.futures._base.CancelledError

Let me know if there's anymore details I can give you, or if I'm just doing something silly.

miguelgrinberg · 2017-02-25T01:11:06Z

@jamesstidard I missed that, thanks. Sanic imposes a 60 second timeout on handlers, I need to circumvent that for websocket routes.

jamesstidard · 2017-02-25T01:15:09Z

@miguelgrinberg No problem. Thank you for taking the time to implement WebSockets - I got the impression it was a low priority.

miguelgrinberg · 2017-02-25T01:17:41Z

@jamesstidard yeah, probably not high priority for the project admins, but I personally would like to have it so that I can have sanic support in my socketio server.

miguelgrinberg · 2017-02-25T02:13:42Z

@jamesstidard Let me know if your websocket client is now happy.

jamesstidard · 2017-02-25T02:46:24Z

@miguelgrinberg Seems to be behaving, Thank you.

r0fls · 2017-02-25T08:37:03Z

I wouldn't say that it's not high priority. Just haven't had the time to implement it. I am curious though, from an ops perspective it might make more sense to have the websocket application be it's own process (outside of the sanic app). What logic there is for having them run in the same loop, aside from convenience?

r0fls · 2017-02-25T08:38:23Z

Not that I don't support this, since it at least makes sense on a small scale. On a large scale though, is it really the ideal way to run in production?

miguelgrinberg · 2017-02-25T16:10:42Z

@r0fls As I mentioned above, I'm particularly interested in extending my Socket.IO server on to sanic (I already have aiohttp support). The Socket.IO protocol requires a combined HTTP + WebSocket interface on the same host and port, so it is really inconvenient if the servers are running separately, even if they share the same process as you currently recommend.

As far as plain websocket, I think there are use cases that benefit from a separate servers approach, like you say, but there are cases where that is an inconvenience. If you are into microservices, for example, you are probably well set up to have a separate websocket server that can share access to storage and other services. If you have a monolithic server, then there's going to be all sorts of problems if you are forced to host just the websocket service separately.

messense · 2017-03-16T04:07:51Z

@miguelgrinberg
Please take a look at my comment in: #545 (comment)

dfee · 2017-04-04T07:27:55Z

@miguelgrinberg I'm confused by your statement that "timeouts make no sense for websocket routes".

I agree that makes sense, but the code doesn't seem to keep the connection alive. Was that intended, or was that more of a TODO?

I.e. currently, it seems that we need to currently manage the ping/pong to keep sanic from disconnecting.

miguelgrinberg · 2017-04-04T14:45:29Z

@dfee That statement I made was in relation to the bug reported in #545 (comment). The timeouts I was talking about were the ones sanic imposes on all routes, which default to 60 seconds if I remember correctly. A WebSocket route will last for as long as the two parties are connected, so you can't apply that kind of timeout.

but the code doesn't seem to keep the connection alive.

Can you give me more details, or a way to reproduce this?

dfee · 2017-04-05T05:55:40Z

@miguelgrinberg gotcha.

Here's what I've done to work around that auto-disconnect (I manually intervene):

        while True:
            try:
                data = await asyncio.wait_for(self.ws.recv(), 30)
            except asyncio.TimeoutError:
                await self.ws.ping()
                continue

            handle(data) # …

miguelgrinberg · 2017-04-05T06:07:39Z

@dfee this "auto-disconnect" that you mention, is it the client or the server doing it? Sounds like if you need to send pings to the client to prevent a disconnection it might be the client that has that requirement. I guess we can add a background ping to prevent this, but I'm not sure this is a problem in general, it might be specific to the client that you are using.

dfee · 2017-04-05T06:25:36Z

I'm using the latest version of Chrome. Does it not time out for you?

miguelgrinberg · 2017-04-05T06:36:20Z

@dfee I don't recall getting timeouts, but maybe I did not leave the connection idle long enough. As far as I know, there isn't a required ping/pong frequency in the standard, this is left to handle by the client and server on their own. I'll look at how other ws servers do this, I honestly have no idea what the right frequency to ping might be. Consider that if you have 10K clients, you'll be running as many pings every 30 seconds.

dfee · 2017-04-05T19:56:30Z

Oh man. I tried for a couple hours this morning to get to the root of it. I finally did. This is not a problem with websockets or sanic.

I needed to add the following to my nginx config:

proxy_connect_timeout 7d;
proxy_send_timeout 7d;
proxy_read_timeout 7d;

Thanks for your help @miguelgrinberg

websocket support

miguelgrinberg force-pushed the websocket-support branch 2 times, most recently from b02ed96 to 83802dc Compare February 20, 2017 20:36

r0fls approved these changes Feb 20, 2017

View reviewed changes

seemethere requested changes Feb 20, 2017

View reviewed changes

seemethere added this to the 0.4.0 milestone Feb 20, 2017

seemethere modified the milestones: 0.5.0, 0.4.0 Feb 22, 2017

seemethere approved these changes Feb 22, 2017

View reviewed changes

miguelgrinberg force-pushed the websocket-support branch from e285ffe to 60f0ccf Compare February 25, 2017 02:12

miguelgrinberg and others added 7 commits February 27, 2017 22:35

websocket support, using websockets package

6e903ee

addressed feedback

1d6e11c

websocket unit test

3bf7989

websocket routes in blueprints

f90288f

websocket documentation

40ccb4a

handle timeouts and disconnects properly

7560660

Added app to websocket request (#1)

104a7c7

miguelgrinberg force-pushed the websocket-support branch from 11aaeb2 to 104a7c7 Compare February 28, 2017 06:35

miguelgrinberg mentioned this pull request Mar 5, 2017

is it possible to use this in asyncio? miguelgrinberg/python-socketio#63

Closed

seemethere approved these changes Mar 6, 2017

View reviewed changes

seemethere merged commit 8e6678d into sanic-org:master Mar 6, 2017

messense mentioned this pull request Mar 16, 2017

Gunicorn worker #545

Merged

zenixls2 pushed a commit to zenixls2/sanic that referenced this pull request Apr 11, 2017

Merge pull request sanic-org#469 from miguelgrinberg/websocket-support

bceb23a

websocket support

zenixls2 pushed a commit to zenixls2/sanic that referenced this pull request Apr 11, 2017

Merge pull request sanic-org#469 from miguelgrinberg/websocket-support

06140ef

websocket support

r0fls mentioned this pull request Jun 7, 2017

test coverage #781

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

websocket support #469

websocket support #469

miguelgrinberg commented Feb 20, 2017

miguelgrinberg commented Feb 20, 2017

seemethere left a comment

seemethere Feb 20, 2017

seemethere Feb 20, 2017

seemethere Feb 20, 2017

seemethere Feb 20, 2017

r0fls Feb 20, 2017

seemethere Feb 20, 2017

miguelgrinberg commented Feb 20, 2017

jamesstidard commented Feb 25, 2017

miguelgrinberg commented Feb 25, 2017

jamesstidard commented Feb 25, 2017

miguelgrinberg commented Feb 25, 2017

miguelgrinberg commented Feb 25, 2017

jamesstidard commented Feb 25, 2017 •

edited

Loading

r0fls commented Feb 25, 2017 •

edited

Loading

r0fls commented Feb 25, 2017

miguelgrinberg commented Feb 25, 2017

messense commented Mar 16, 2017 •

edited

Loading

dfee commented Apr 4, 2017

miguelgrinberg commented Apr 4, 2017

dfee commented Apr 5, 2017

miguelgrinberg commented Apr 5, 2017

dfee commented Apr 5, 2017

miguelgrinberg commented Apr 5, 2017

dfee commented Apr 5, 2017

websocket support #469

websocket support #469

Conversation

miguelgrinberg commented Feb 20, 2017

miguelgrinberg commented Feb 20, 2017

seemethere left a comment

Choose a reason for hiding this comment

seemethere Feb 20, 2017

Choose a reason for hiding this comment

seemethere Feb 20, 2017

Choose a reason for hiding this comment

seemethere Feb 20, 2017

Choose a reason for hiding this comment

seemethere Feb 20, 2017

Choose a reason for hiding this comment

r0fls Feb 20, 2017

Choose a reason for hiding this comment

seemethere Feb 20, 2017

Choose a reason for hiding this comment

miguelgrinberg commented Feb 20, 2017

jamesstidard commented Feb 25, 2017

miguelgrinberg commented Feb 25, 2017

jamesstidard commented Feb 25, 2017

miguelgrinberg commented Feb 25, 2017

miguelgrinberg commented Feb 25, 2017

jamesstidard commented Feb 25, 2017 • edited Loading

r0fls commented Feb 25, 2017 • edited Loading

r0fls commented Feb 25, 2017

miguelgrinberg commented Feb 25, 2017

messense commented Mar 16, 2017 • edited Loading

dfee commented Apr 4, 2017

miguelgrinberg commented Apr 4, 2017

dfee commented Apr 5, 2017

miguelgrinberg commented Apr 5, 2017

dfee commented Apr 5, 2017

miguelgrinberg commented Apr 5, 2017

dfee commented Apr 5, 2017

jamesstidard commented Feb 25, 2017 •

edited

Loading

r0fls commented Feb 25, 2017 •

edited

Loading

messense commented Mar 16, 2017 •

edited

Loading