Integrating websockets directly to the core, using a new route proxy mechanism. #53

trollfot · 2018-03-26T17:51:45Z

No description provided.

…ntation adapted to Roll. Took the liberty to move the protocols to a new file, for more readability.

…transport reference to the request. Fixed subprotocols selection.

…proved the ws test file, just to prove it works. Now it needs testing.

…complete' handler

…ently, the websocket closure, ws.close in the websocket handler is hanging a bit. Still investigating.

…ncy on aiohttp, which is unfortunate. Reverted back to the old testing.Transport for the other tests.

…the pipe or not ? Added some more test. One is currently being worked on to determine how to handle a faulty websocket.

…. Maybe there are alternative for the async HTTPClient

…ilure. As it turns out, the behavior of our failing websocket is normal. It seems Sanic's behavior is not right : a closing websocket shouldn't cut the communication. Decreased the websocket timeout from 10s to 5s. We might need a way to make it a bit more pluggable

…the closing of the comm channel, needs work. Tests fail currently

…ts and liveclient to use now the 'http' lib

trollfot · 2018-03-26T17:54:22Z

Here is a draft of what we discussed on the previous PR.
The websocket is now integrated directly to the 'route' decorator, adding a new 'protocol' keyword. That allows the route decorator to wrap the handler in a "proxy", if a proxy exists for the declared protocol. This proxy, in the case of the websocket, takes charge of the handshake and websocket protocol switch using directly 'set_protocol' on the transport.

davidbgk

Thanks for that focused PR, I'll try to reach out @yohanboniface within the next couple of days to discuss it!

davidbgk · 2018-03-26T18:38:03Z

roll/websocket.py

+        websocket.connection_open()
+        wsprotocol = self.Protocol(websocket)
+        request.transport.set_protocol(wsprotocol)
+        return wsprotocol, websocket


What is the goal of returning wsprotocol here?

It is only just a semantic thing but it could be used when overridden. When i get a "switch_protocol", i expect to get the new protocol back, even if I don't use it directly. This can be removed obviously, if you don't share that feeling.

Alright, let's keep it as is.

This is no longer an issue, we no longer have 2 distinct protocols. The proxy protocol (wsprotocol) is useless and only necessary if we were an extension. Now, it doesn't make sense, it was removed and only "websocket" remains. I should have realized that earlier.

davidbgk · 2018-03-26T18:39:40Z

roll/websocket.py

+        self.writer.close()
+
+
+class WebsocketHandler:


Maybe WebsocketProxy would be a better name given the usage?

Probably, even though both terminologies are probably equaly true here

I was suggesting that because of the later: proxy = self.Protocols.get(protocol, None)

I renamed 'proxy' to 'handler', for consistency. This may be done the other way, if it's judged better over time.

davidbgk · 2018-03-26T18:40:02Z

roll/__init__.py



-class Roll:
+class Roll(dict):


Is that still necessary?

I used it to store the websockets upon creation. If you mean : would it be better to have an attribute rather than a container logic, maybe ? I think it's rather nice to be able to store, for an extension, something in the app itself. It's probably out of scope, I can remove it.

davidbgk · 2018-03-26T18:40:54Z

roll/__init__.py

@@ -382,9 +397,12 @@ def write(self, *args):


 Route = namedtuple('Route', ['payload', 'vars'])
+Protocols = {


Can we move that to Roll in order to be easy to customize?

Sure, let's do this.

davidbgk · 2018-03-26T18:41:24Z

roll/__init__.py

-    def connection_made(self, transport):
-        self.writer = transport
+        except HttpParserUpgrade:
+            self.upgrade = self.request.headers.get('Upgrade')


This is where the magic happens if I understand correctly, worth a comment :)

No, it's just a way to have the requested upgrade somewhere. It's not used yet. the "Upgrade" header should contain "websocket" or "keep-alive, websocket", in case of a websocket. We should test it somewhere, probably in the proxy itself, but this wasn't discussed with you or Yohan, so maybe we can take a decision here.

I'm not sure I get it, how is the request upgraded then?

Can you add a test with "keep-alive, websocket" at that point or is it useless until we actually use it?

I got confused in my comment, i meant the upgrade can contain a list of protocols, just like sub-protocols can be a list of subprotocol, separated with commas, ordered by preference.
I added tests and comments in the code and a RFC link, to be less messy than I am in my explanations. Sorry about that 👎

Right now, I changed to write the info on the request. That makes much more sense than on the protocol: that was really silly.

It is not really parsed, just read as it is. We trust the handler/proxy to do what is right with it, as I did on the websocket one (fairly naively).

davidbgk · 2018-03-26T18:41:39Z

roll/__init__.py

 from collections import namedtuple
 from http import HTTPStatus
 from io import BytesIO
 from typing import TypeVar
-from urllib.parse import parse_qs, unquote
+from urllib.parse import unquote, parse_qs


Sorry, the backtracking between the 4 branches led to silly changes.

davidbgk · 2018-03-26T18:43:24Z

roll/websocket.py

+        # Received data. We refuse the data if the websocket is
+        # already closed. If the websocket is closing, this data
+        # might be part of the closing handshake (closing frame)
+        if self.websocket.state != 3:  # not closed


I prefer the use of State.CLOSED, see: https://github.com/pyrates/roll/pull/51/files#diff-92645e4dbf61c4340d8fdec66cc79a4fR75

Yes, sorry. I restarted the work from the very first PR and forgot to have that change from the later PRs. That's also why I removed the other branches in their entirety (except the 51, that contain other points.). I'll correct that right now

… state. The 'Protocols' dict, defining proxies for the different protocols has been put inside the Application class, so it's possible to override it.

…dingly.

…equest, as a commodity, contains the information in its 'upgrade' property, taken from the UPGRADE header (might need parsing or validation). Failure to find the upgrade info will result in a 426 status, UPGRADE REQUIRED. Added some comments to ease of the understanding of that part.

… from 'websockets'. With the correct flow, it now works directly.

…de if it's suitable or not and avoid hardcoding any decision in the route decorator

trollfot · 2018-03-26T22:55:05Z

I think I covered most of the principal concerns. I'll let it be dissected by both of you now and won't touch it anymore until then.

davidbgk

Thanks for the changes!

davidbgk · 2018-03-27T01:04:17Z

roll/websocket.py

+        if request.upgrade != 'websocket':
+            # https://tools.ietf.org/html/rfc7231.html#page-62
+            # Upgrade needed but none was request or of the wrong type
+            response.status = HTTPStatus.UPGRADE_REQUIRED


Why not raising a roll.HttpError here?

For a silly cross import problem. WebsocketHandler is imported in the init.py file and thus importing from init will create a problem, that's why I used the response, available there.

Maybe we could give errors their dedicated file to avoid that? @yohanboniface thoughts?

davidbgk · 2018-03-27T01:08:22Z

roll/websocket.py

+
+        ws = self.switch_protocol(request)
+        if 'websockets' not in request.app:
+            request.app['websockets'] = set()


How do we ensure that set is flushed after some time? Is that even pertinent? (I have no experience with websockets)

The handler ensures that the websocket is removed from the set. In Sanic, to give an example, they keep all the connections in the same fashion but they clear it thanks to a task that verifies the timeouts. Here, I think I covered all in a try/catch. But there is a risk, if the global handler task is canceled. It could be fixed, if that's a problem, with a done_callback that would ensure that or a recurring task that would check for closed ws.

davidbgk · 2018-03-27T01:12:57Z

tests/test_websockets.py

+            'Sec-WebSocket-Version': '13'})
+
+    assert ev.is_set()
+    assert response.status == 101


Can you use http.HTTPStatus.SWITCHING_PROTOCOLS here and there for consistency?

davidbgk · 2018-03-27T01:13:35Z

tests/test_websockets.py

+    await websocket.close_connection_task
+    assert bdata == b'test'
+    assert websocket.close_reason == ''
+    assert websocket.state == 3


Should be State.CLOSED, same for other tests.

davidbgk · 2018-03-27T01:15:18Z

tests/test_websockets_failure.py

+            'Sec-WebSocket-Key': 'hojIvDoHedBucveephosh8==',
+            'Sec-WebSocket-Version': '13'})
+
+    assert response.status == 426


Same here with http.HTTPStatus.UPGRADE_REQUIRED

davidbgk · 2018-03-27T01:15:34Z

tests/test_websockets_failure.py

+    # Wrong upgrade
+    with liveclient as query:
+        response = await query('GET', '/ws', headers={
+            'Upgrade': 'http2',


You wish ! ;-)

…cket.

yohanboniface · 2018-03-28T15:59:11Z

roll/__init__.py

+            # This header should be parsed and maybe validated if we want
+            # to handle complex declarations, such as :
+            # Upgrade: HTTP/2.0, SHTTP/1.3, IRC/6.9, RTA/x11
+            self.request.upgrade = self.request.headers['UPGRADE'].lower()


Not sure about the pattern yet: seems we are mixing a bit the Protocol and the Request responsibilities.
I feel like the big picture is:
HttpProtocol => Upgrade received => close http response => set_protocol(WSProtocol).

What about a pair session so we can tackle this down at four hands? :)

Pair programming sounds good. Whenever you want

…mplemented the new way, to prove it.

davidbgk · 2018-03-29T16:57:45Z

roll/__init__.py

+            self.request.app.websockets.discard(self)
+            await self.close()
+
+
 class Protocol(asyncio.Protocol):


What about calling it HTTPProtocol?

I did it, it makes plenty sense

davidbgk · 2018-03-29T16:58:09Z

requirements.txt

@@ -1,5 +1,6 @@
 autoroutes==0.2.0
 biscuits==0.1.1
 httptools==0.0.10


Bump to 0.0.11 🎉

…rotocol to use it. Renamed Protocol to HTTPProtocol.

davidbgk · 2018-03-29T17:58:46Z

roll/__init__.py

@@ -399,19 +540,26 @@ class Roll:
    Response = Response
    Cookies = Cookies

+    protocols = {
+        'websocket': WSProtocol,


If protocols are a thing now, I think adding 'http': HTTPProtocol might be pertinent here. Still very unsure, more something to discuss.

yohanboniface · 2018-03-29T19:55:15Z

roll/__init__.py

@@ -268,7 +278,7 @@ def host(self):

 class Response:
    """A container for `status`, `headers` and `body`."""
-    __slots__ = ('app', '_status', 'headers', 'body', '_cookies')
+    __slots__ = ('app', '_status', 'headers', 'body', '_cookies', 'websocket')


No more needed ;)

yohanboniface · 2018-03-29T19:57:51Z

roll/__init__.py

@@ -338,7 +454,12 @@ def on_body(self, body: bytes):
        # FIXME do not put all body in RAM blindly.
        self.request.body += body

+    def on_headers_complete(self):
+        # Lookup the route requested


"Say why do not say what" is my comment mantra ;)
And if you thought a "what" comment was needed here, maybe it's because the naming is not good.
Suggestions: resolve_route, match_route ?

…us, overriden immediatly by the init of status.

…Moved the route lookup in the on_url

yohanboniface · 2018-05-18T18:48:13Z

Superseded by #54 now merged \o/

trollfot added 18 commits March 6, 2018 19:09

Added websockets using the websockets package. Based on Sanic impleme…

6dcba23

…ntation adapted to Roll. Took the liberty to move the protocols to a new file, for more readability.

Fixed silly caching of protocol factory result on the app. Added the …

0a4f0e1

…transport reference to the request. Fixed subprotocols selection.

Keeping track of the websocket object as well as its related task. Im…

7e77fa6

…proved the ws test file, just to prove it works. Now it needs testing.

Removed duplicate assignation and completely removed the 'on_headers_…

7d962b8

…complete' handler

Added real Transport to testing. Started to test the websockets. Curr…

1a0af9d

…ently, the websocket closure, ws.close in the websocket handler is hanging a bit. Still investigating.

Added first websocket test. Still hanging

a99b3c2

Using now a liveclient to test the websockets. This creates a depende…

ca758d5

…ncy on aiohttp, which is unfortunate. Reverted back to the old testing.Transport for the other tests.

Added websockets communication tests.

1d8a2d9

Still stuck on that exception handling : how do we handle it ? Break …

8defc3a

…the pipe or not ? Added some more test. One is currently being worked on to determine how to handle a faulty websocket.

Added missing quote to the URL in websocket html example

301655b

Added missing dependency on aiohttp. This is quite a heavy dependency…

29f8e79

…. Maybe there are alternative for the async HTTPClient

Removed aiohttp dependency. Now using 'requests' with executor.

6164fad

Removed py3.6 formatting. Let's be 3.5 !

2befdaf

Don't use async for with websockets for py3.5's sake

ee571eb

First try at using the set_protocol directly. It has side effects on …

03b91c9

…the closing of the comm channel, needs work. Tests fail currently

Fixed all tests and closure problems

744da0f

Removed 'requests' dependency. Pep8 & pyflakes cleanup. Fixed all tes…

ac4fc4b

…ts and liveclient to use now the 'http' lib

davidbgk reviewed Mar 26, 2018

View reviewed changes

trollfot added 6 commits March 26, 2018 21:29

Using now the state variables from 'websockets' to test the websocket…

feaa7ce

… state. The 'Protocols' dict, defining proxies for the different protocols has been put inside the Application class, so it's possible to override it.

Headers are uppercase: corrected the upgrade exception catching accor…

d39ad38

…dingly.

Removed useless proxy protocol to directly use the websocket protocol…

b247ffb

… from 'websockets'. With the correct flow, it now works directly.

Renamed 'proxy' to 'handler', for consistency

741d726

We now pass the route methods to the protocol handler, to let it deci…

0a5ced8

…de if it's suitable or not and avoid hardcoding any decision in the route decorator

davidbgk reviewed Mar 27, 2018

View reviewed changes

Using explicit variables for the status of the response and the webso…

0dc0071

…cket.

yohanboniface reviewed Mar 28, 2018

View reviewed changes

trollfot added 2 commits March 29, 2018 13:26

Work in progress

3f76f60

Tests were fixed. The new protocol plugin works and websockets were i…

78aeccb

…mplemented the new way, to prove it.

davidbgk reviewed Mar 29, 2018

View reviewed changes

Updated httptools version to include should_upgrade and updated the p…

09e8cd7

…rotocol to use it. Renamed Protocol to HTTPProtocol.

davidbgk reviewed Mar 29, 2018

View reviewed changes

yohanboniface reviewed Mar 29, 2018

View reviewed changes

trollfot added 2 commits March 30, 2018 00:39

Removed unused 'websocket' slot on the response and the init of _stat…

6cc762d

…us, overriden immediatly by the init of status.

Backtracking on the use of an async run method on the http protocol. …

6a6ade4

…Moved the route lookup in the on_url

yohanboniface mentioned this pull request Mar 30, 2018

Websocket #54

Merged

yohanboniface closed this May 18, 2018

		@@ -382,9 +397,12 @@ def write(self, *args):


		Route = namedtuple('Route', ['payload', 'vars'])
		Protocols = {

Integrating websockets directly to the core, using a new route proxy mechanism. #53

Integrating websockets directly to the core, using a new route proxy mechanism. #53

Conversation

trollfot commented Mar 26, 2018

trollfot commented Mar 26, 2018 • edited

davidbgk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trollfot Mar 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trollfot commented Mar 26, 2018

davidbgk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trollfot Mar 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trollfot Mar 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yohanboniface commented May 18, 2018

trollfot commented Mar 26, 2018 •

edited

trollfot Mar 26, 2018 •

edited

trollfot Mar 27, 2018 •

edited

trollfot Mar 27, 2018 •

edited