Implement aiohttp.web websockets #220

asvetlov · 2015-01-02T13:34:10Z

Hi.
I've made websockets support in aiohttp.web.

The code still has no full test coverage but basic tests are passed.

Please review.

asvetlov · 2015-01-02T15:17:20Z

The code is fully covered!

fafhrd91 · 2015-01-02T17:32:14Z

aiohttp/web.py

+            try:
+                msg = yield from self._reader.read()
+            except Exception as exc:
+                raise WebSocketClosed(code=None, message=str(exc)) from exc


i don't think we should catch this exception, or we should make distinction

protocol violation, like binary format errs

client disconnection exception, i think this exception should be re-raised.

Isn't exception.__cause__ enouth?

it doesn't matter, up to you. i think it should act similar to ClientDisconnectedError exception from normal http handler.

fafhrd91 · 2015-01-02T17:39:25Z

i like implementation! we just need better closing procedure.

asvetlov · 2015-01-02T18:02:49Z

Please take a look: I've chosen GeneratorExit as base class for WebSocketClosed exception.

You can override it by catching error directly but if you don't do this RequestHandler will close HTTP connection.

Do you agree with GeneratorExit design?

asvetlov · 2015-01-02T18:09:13Z

i think this is unsafe. it is high likely GeneratorExit raises on GC (i.e. process exit)

Sorry, I don't follow

asvetlov · 2015-01-02T18:30:02Z

Yet another issue: what if web handler will create tasks those will wait for .receive()?
I can forbid the case by pinning Task.current_task() on WebSocketResponse.start()

fafhrd91 · 2015-01-02T18:49:43Z

whenever i get GeneratorExit, it happens during process termination. during normal work we should never get GeneratorExit, in most cases it is indicator of something totally wrong.
i just think that explicit GeneratorExit catch is wrong.

On Jan 2, 2015, at 10:09 AM, Andrew Svetlov notifications@github.com wrote:

i think this is unsafe. it is high likely GeneratorExit raises on GC (i.e. process exit)
Sorry, I dont' follow

—
Reply to this email directly or view it on GitHub #220 (comment).

fafhrd91 · 2015-01-02T18:50:50Z

i think we need WebSocketDisconnected exception and it should be DisconnectedError.

On Jan 2, 2015, at 10:02 AM, Andrew Svetlov notifications@github.com wrote:

Please take a look: I've chosen GeneratorExit as base class for WebSocketClosed exception.

You can override it by catching error directly but if you don't do this RequestHandler will close HTTP connection.

Do you agree with GeneratorExit design?

—
Reply to this email directly or view it on GitHub #220 (comment).

fafhrd91 · 2015-01-02T18:52:20Z

i don’t think it is good idea. from my experience with websockets, it is usually require 2 tasks one for reading and one for writing.

On Jan 2, 2015, at 10:30 AM, Andrew Svetlov notifications@github.com wrote:

Yet another issue: what if web handler will create tasks those will wait for .receive()?
I can forbid the case by pinning Task.current_task() on WebSocketResponse.start()

—
Reply to this email directly or view it on GitHub #220 (comment).

…adding origins also).

…, inherit WebClosedError from DisconnectedError instead of GeneratorExit

asvetlov · 2015-01-03T18:02:16Z

@fafhrd91 please review again.
I hope I've fixed discussed problems.

asvetlov · 2015-01-03T18:10:55Z

Server should have the single task for reading websocket messages.
Also it may tasks for writing ones.
If server calls .close() from reading task it should wait for handshake by .receive().
If it calls .close() from writing ones may call .wait_closed() for waiting for closing -- the handshake will be done in .receive() by reading task.
send_* and ping are forbidden after .close(), any received messages after the call are ignored.

fafhrd91 · 2015-01-03T18:46:04Z

aiohttp/web.py


-        # log access
-        self.log_access(message, None, resp_msg, self._loop.time() - now)
+        except DisconnectedError:


this will catch disconnects from aiohttp.client

ServerHttpProtocol.start() already handles ClientDisconnectedError, change start()

fafhrd91 · 2015-01-03T18:49:34Z

lgtm, except two notes regarding exceptions.

…, get rid of checking for disconnection in web.RequestHandler

asvetlov · 2015-01-03T18:55:18Z

Exceptions are fixed.

On Sat, Jan 3, 2015 at 8:49 PM, Nikolay Kim notifications@github.com
wrote:

lgtm, except two notes regarding exceptions.

—
Reply to this email directly or view it on GitHub
#220 (comment).

Thanks,
Andrew Svetlov

asvetlov · 2015-01-03T18:55:59Z

Please review again.

fafhrd91 · 2015-01-03T18:57:12Z

awesome! :)

lgtm

Implement aiohttp.web websockets

asvetlov · 2015-01-03T18:59:47Z

@fafhrd91 thanks for reviews

lock · 2019-10-30T04:03:16Z

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

fafhrd91 reviewed Jan 2, 2015
View reviewed changes

asvetlov added 13 commits January 3, 2015 11:09

Implement aiohttp.web websockets

9bf30fe

Fix flake8 errors

d5a1649

More tests

0129974

Add test for raising exception from WebSocketResponse.receive()

40dd19d

Add nonfunctional tests for aiohttp.web.websockets

b7330dd

Fix closing web RequestHandler on GeneratorExit

e333680

Add comment on reason for closing transport on GeneratorExit

4d2071f

Fix grammar

b69e82c

Add test for handling websocket protocols

cb87c4a

Fix test for websocket protocols

fce528c

Make WebSocketResponse protocols parameter keyword-only (think about …

717d338

…adding origins also).

Add closing, can_close and wait_closed to WebSocketResponse

6791d9c

Convert internal HttpProcessingError to HTTPException-derived classes…

94d79e3

…, inherit WebClosedError from DisconnectedError instead of GeneratorExit

asvetlov force-pushed the websockets branch from a3fec06 to 94d79e3 Compare January 3, 2015 16:29

asvetlov added 3 commits January 3, 2015 19:48

Test closing handshake

7bbb7e6

Force non-keep-alive for websocket connections

5169738

Dont catch internal errors from socket stream reader

96f39be

asvetlov added 4 commits January 3, 2015 20:18

Add test for wait_closing() before start()

806ef8f

Add test for drain() before start()

e60437d

Fix typo

c1168ad

Add check for eating messages after close

1ea6bb8

fafhrd91 reviewed Jan 3, 2015
View reviewed changes

Move WebSocketClosed to errors.py, rename it to WebSocketDisconnected…

d0d6f6a

…, get rid of checking for disconnection in web.RequestHandler

asvetlov added a commit that referenced this pull request Jan 3, 2015

Merge pull request #220 from KeepSafe/websockets

04e9620

Implement aiohttp.web websockets

asvetlov merged commit 04e9620 into master Jan 3, 2015

asvetlov deleted the websockets branch January 3, 2015 18:59

fafhrd91 mentioned this pull request Jan 3, 2015

websockets #194

Closed

lock bot added the outdated label Oct 30, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement aiohttp.web websockets #220

Implement aiohttp.web websockets #220

asvetlov commented Jan 2, 2015

asvetlov commented Jan 2, 2015

fafhrd91 Jan 2, 2015

asvetlov Jan 2, 2015

fafhrd91 Jan 2, 2015

fafhrd91 commented Jan 2, 2015

asvetlov commented Jan 2, 2015

asvetlov commented Jan 2, 2015

asvetlov commented Jan 2, 2015

fafhrd91 commented Jan 2, 2015

fafhrd91 commented Jan 2, 2015

fafhrd91 commented Jan 2, 2015

asvetlov commented Jan 3, 2015

asvetlov commented Jan 3, 2015

fafhrd91 Jan 3, 2015

fafhrd91 commented Jan 3, 2015

asvetlov commented Jan 3, 2015

asvetlov commented Jan 3, 2015

fafhrd91 commented Jan 3, 2015

asvetlov commented Jan 3, 2015

lock bot commented Oct 30, 2019

Implement aiohttp.web websockets #220

Implement aiohttp.web websockets #220

Conversation

asvetlov commented Jan 2, 2015

asvetlov commented Jan 2, 2015

fafhrd91 Jan 2, 2015

Choose a reason for hiding this comment

asvetlov Jan 2, 2015

Choose a reason for hiding this comment

fafhrd91 Jan 2, 2015

Choose a reason for hiding this comment

fafhrd91 commented Jan 2, 2015

asvetlov commented Jan 2, 2015

asvetlov commented Jan 2, 2015

asvetlov commented Jan 2, 2015

fafhrd91 commented Jan 2, 2015

fafhrd91 commented Jan 2, 2015

fafhrd91 commented Jan 2, 2015

asvetlov commented Jan 3, 2015

asvetlov commented Jan 3, 2015

fafhrd91 Jan 3, 2015

Choose a reason for hiding this comment

fafhrd91 commented Jan 3, 2015

asvetlov commented Jan 3, 2015

asvetlov commented Jan 3, 2015

fafhrd91 commented Jan 3, 2015

asvetlov commented Jan 3, 2015

lock bot commented Oct 30, 2019