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

Websocket #54

Merged
merged 50 commits into from May 17, 2018

Conversation

3 participants
@yohanboniface
Member

yohanboniface commented Mar 30, 2018

Continuing #53, now from the inside! :)

needs_upgrade = True
allowed_methods = {'GET'}
timeout = 5

This comment has been minimized.

@davidbgk

davidbgk Apr 17, 2018

Contributor

Here and below, uppercase if constants? More consistent with other declarations.

This comment has been minimized.

@yohanboniface

yohanboniface Apr 25, 2018

Member

+1 for uppercase
Plus: what about reading the value from the env var, with a default?
Something like:

TIMEOUT = os.environ.get('ROLL_WEBSOCKET_TIMEOUT', 5)

As general principle, I like the idea of having the first configuration level (setting up a configuration value) with env var, and the second level (changing the behaviour) from inheritance.

headers = '\r\n'.join('{}: {}'.format(*h) for h in headers.items())
data = b'%b %b HTTP/1.1\r\n%b\r\n\r\n%b' % (
method.encode(), path.encode(), headers.encode(), body or b'')
print(data)

This comment has been minimized.

@davidbgk

davidbgk Apr 17, 2018

Contributor

💣

@@ -0,0 +1,69 @@
Websockets inner and outer workings

This comment has been minimized.

@davidbgk

davidbgk Apr 17, 2018

Contributor

Should be put in/linked from docs.

'Sec-WebSocket-Key': 'hojIvDoHedBucveephosh8==',
'Sec-WebSocket-Version': '13'})
#assert ev.is_set()

This comment has been minimized.

@davidbgk

davidbgk Apr 17, 2018

Contributor

Is it set at that point?

This comment has been minimized.

@trollfot

trollfot Apr 17, 2018

Member

It is. I pushed a fix to uncomment that test. I don't remember why it was commented... and it slipped under the radar !

@yohanboniface

Almost there, yay! :)

print(message)
```
NB: while most clients will keep the connection alive and won't expect

This comment has been minimized.

@yohanboniface

yohanboniface Apr 25, 2018

Member

I wonder how much this "advanced" case should be a "how-to" of a "Websockets" section instead.

@@ -0,0 +1,34 @@
# -*- coding: utf-8 -*-

This comment has been minimized.

@yohanboniface

yohanboniface Apr 25, 2018

Member

We can remove this line :)

needs_upgrade = True
allowed_methods = {'GET'}
timeout = 5

This comment has been minimized.

@yohanboniface

yohanboniface Apr 25, 2018

Member

+1 for uppercase
Plus: what about reading the value from the env var, with a default?
Something like:

TIMEOUT = os.environ.get('ROLL_WEBSOCKET_TIMEOUT', 5)

As general principle, I like the idea of having the first configuration level (setting up a configuration value) with env var, and the second level (changing the behaviour) from inheritance.

async def run(self):
try:
self.request.app.websockets.add(self)

This comment has been minimized.

@yohanboniface

yohanboniface Apr 25, 2018

Member

I think I'd prefer let this to the user responsibility if they need it.
Mainly because I feel like this is not generic enough to be in the core (a real usecase would not combine all websockets in once, but maybe only some of them?).
We can add an how-to.
Something like:

@app.listen('startup')
async def on_startup():
    app['chat_websockets'] = set()

async def myhandler(request, ws):
    app['chat_websockets'].add(ws)
    ...

And writting this I see we certainly needs to expose a way to discard. Two new hooks on websocket add and discard?

def __init__(self):
self.routes = self.Routes()
self.hooks = {}
self.hooks = dict()

This comment has been minimized.

@yohanboniface

yohanboniface Apr 25, 2018

Member

Warning: dict() is slower than literal {}.
On Roll init I think it's fair tough. :)

if methods is None:
methods = ['GET']
protocol = protocol.lower()
klass = getattr(self, protocol.title() + 'Protocol')
assert klass is not None

This comment has been minimized.

@yohanboniface

yohanboniface Apr 25, 2018

Member

Suggestion:

klass_attr = protocol.title() + 'Protocol'
klass = getattr(self, klass_attr, None)
assert klass, 'No class handler declared for {} protocol. Add a {} key to your Roll app.'.format(protocol, klass_attr)
@@ -76,8 +84,9 @@ class Client:
# taste if needed.
content_type = 'application/json; charset=utf-8'
def __init__(self, app):
def __init__(self, app, event_loop):

This comment has been minimized.

@yohanboniface

yohanboniface Apr 25, 2018

Member

Why do we need even_loop while it's already attached to the app?

async def handler(request, ws):
results.append(ws.subprotocol)
with liveclient as query:

This comment has been minimized.

@yohanboniface

yohanboniface Apr 27, 2018

Member

Shower thought: I wonder if we can close the client from the fixture instead of needing a context manager for each query.

This comment has been minimized.

@yohanboniface

yohanboniface May 14, 2018

Member

If I recall correctly our discussion, I think we decided to align the liveclient to the client one, so instead of the context manager, simply do something like:

await liveclient.query(…)

trollfot added some commits Mar 6, 2018

Added websockets using the websockets package. Based on Sanic impleme…
…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 …
…transport reference to the request. Fixed subprotocols selection.
Keeping track of the websocket object as well as its related task. Im…
…proved the ws test file, just to prove it works. Now it needs testing.
Added real Transport to testing. Started to test the websockets. Curr…
…ently, the websocket closure, ws.close in the websocket handler is hanging a bit. Still investigating.
Using now a liveclient to test the websockets. This creates a depende…
…ncy on aiohttp, which is unfortunate. Reverted back to the old testing.Transport for the other tests.
Still stuck on that exception handling : how do we handle it ? Break …
…the pipe or not ? Added some more test. One is currently being worked on to determine how to handle a faulty websocket.
Added missing dependency on aiohttp. This is quite a heavy dependency…
…. Maybe there are alternative for the async HTTPClient
Cleaned up the websocket handling. Added comments. Added tests for fa…
…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
First try at using the set_protocol directly. It has side effects on …
…the closing of the comm channel, needs work. Tests fail currently
Using now the state variables from 'websockets' to test the websocket…
… state. The 'Protocols' dict, defining proxies for the different protocols has been put inside the Application class, so it's possible to override it.
Websocket handler/proxy now checks the request for the upgrade. The r…
…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.
Removed useless proxy protocol to directly use the websocket protocol…
… from 'websockets'. With the correct flow, it now works directly.
We now pass the route methods to the protocol handler, to let it deci…
…de if it's suitable or not and avoid hardcoding any decision in the route decorator

trollfot and others added some commits Mar 29, 2018

@@ -62,12 +65,16 @@ class Transport:
def __init__(self):
self.data = b''
self._closing = False
def is_closing(self):

This comment has been minimized.

@yohanboniface

yohanboniface May 14, 2018

Member

Are we using this in some way?

This comment has been minimized.

@trollfot

trollfot May 16, 2018

Member

We don't, but i think a mock Transport should have the basic methods a Transport has, this was a start.

@yohanboniface yohanboniface merged commit 310f61c into master May 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yohanboniface yohanboniface deleted the websocket branch May 17, 2018

@yohanboniface yohanboniface referenced this pull request Jul 11, 2018

Open

HTTP/2 support #61

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment