Add initial support for PerMessage Deflate by fanthos · Pull Request #2273 · aio-libs/aiohttp

fanthos · 2017-09-16T15:27:09Z

What do these changes do?

Add initial support for WebSocket deflate compress.
Right now both server and client are supported.

Are there changes in behavior for the user?

No

Related issue number

#673

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the changes folder
- name it <issue_id>.<type> for example (588.bug)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

fafhrd91 · 2017-09-16T15:31:34Z

this is good! but could you add unit and functional tests

Add no takeover extension to server and client.

TODO: add tests

fanthos · 2017-09-17T06:29:12Z

The file upload tests in test_client_functional.py keep failing with HTTP 500 on my device.
I am using Windows 10 1703 with Python 3.6.2.

codecov-io · 2017-09-17T06:39:49Z

Codecov Report

Merging #2273 into master will decrease coverage by <.01%.
The diff coverage is 97.77%.

@@            Coverage Diff             @@
##           master    #2273      +/-   ##
==========================================
- Coverage   97.24%   97.24%   -0.01%     
==========================================
  Files          39       39              
  Lines        8096     8201     +105     
  Branches     1411     1439      +28     
==========================================
+ Hits         7873     7975     +102     
- Misses         97       98       +1     
- Partials      126      128       +2

Impacted Files	Coverage Δ
aiohttp/hdrs.py	`100% <100%> (ø)`	⬆️
aiohttp/http_websocket.py	`98.49% <97.75%> (-0.25%)`	⬇️
examples/web_ws.py	`94.86% <0%> (-0.32%)`	⬇️
examples/client_ws.py	`96.44% <0%> (+0.11%)`	⬆️
tests/autobahn/client.py	`94.72% <0%> (+0.21%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8f83a8...22fb3f9. Read the comment docs.

Add parser test for rsv1

Add tests

Client deflate support should now more complete fix linting

asvetlov

I did not review carefully but left some notes.

asvetlov · 2017-09-17T19:10:57Z

aiohttp/client.py

                    proxy=None,
-                    proxy_auth=None):
+                    proxy_auth=None,
+                    compress=0):


Let's use compress=15 as initial value and don't accept True

asvetlov · 2017-09-17T19:11:50Z

aiohttp/client.py

 from .helpers import (PY_35, CeilTimeout, TimeoutHandle, _BaseCoroMixin,
                      deprecated_noop, sentinel)
 from .http import WS_KEY, WebSocketReader, WebSocketWriter
+from .http_websocket import extensions_gen as ws_ext_gen


Why do you rename imports?
Just use ws_ext_gen and ws_ext_parse in http_websocket.py.

asvetlov · 2017-09-17T19:13:38Z

aiohttp/http_websocket.py

+                   client_notakeover=False):
+    # compress wbit 8 does not support in zlib
+    if compress < 9 or compress > 15:
+        return False


Raise ValueError here.

asvetlov · 2017-09-17T19:17:53Z

aiohttp/http_websocket.py

+    for ext in extensions:
+        if ext.startswith('permessage-deflate'):
+            compress = 15
+            for param in [s.strip() for s in ext.split(';')][1:]:


I believe regex parse is more robust.

Fix client code Add more tests

If reader created with compress=0, it will raise PROTOCOL_ERROR if rvb1 = 1.

asvetlov · 2017-09-19T10:08:55Z

aiohttp/client.py

        if origin is not None:
            headers[hdrs.ORIGIN] = origin
+        if compress:
+            if compress is True:


The check is not needed: let's assume compress is always integer.

asvetlov

Sorry for non-instant review.
The PR is complex, the RFC https://tools.ietf.org/html/rfc7692 is complicated.

Everything looks good but please fix my notes.
Also please make https://codecov.io/gh/aio-libs/aiohttp/pull/2273/diff green.
The PR changes default websockets behavior, I have a feeling that is very important change

asvetlov · 2017-09-21T13:16:53Z

aiohttp/http_websocket.py

+    if compress < 9 or compress > 15:
+        raise ValueError('Compress wbits must between 9 and 15, '
+                         'zlib does not support wbits=8')
+    enabledext = None


Please use a list: enabledext = [].
Append an element to the list every time if needed.
Return '; '.join(enabledext) -- or just a list maybe, do joining in caller.
Join is recommended way for performing such operations, it's faster and more memory effective.
Take a look on https://github.com/python/cpython/blob/master/Lib/asyncio/base_futures.py#L54-L71 for inspiration

asvetlov · 2017-09-21T13:24:09Z

aiohttp/client.py

+                    )
+                    if compress == 0:
+                        pass
+                    elif compress == -1:


I could live with this code style but it looks too C-ish.
What's about raising internal HandshakeError exception defined in http_websocket.py as

class HandshakeError(Exception): pass try: raise HandshakeError('Invalid window size') except HandshakeError as exc: raise WSServerHandshakeError( resp.request_info, resp.history, message=exc.args[0], code=resp.status, headers=resp.headers)

Using exception instead of return value.

asvetlov · 2017-09-22T07:39:31Z

aiohttp/client.py

+                        compress, notakeover = ws_ext_parse(
+                            resp.headers[hdrs.SEC_WEBSOCKET_EXTENSIONS]
+                        )
+                        if compress == 0:


What the check does?

asvetlov · 2017-09-22T07:41:15Z

aiohttp/client.py

+            # websocket compress
+            notakeover = False
+            if compress:
+                if hdrs.SEC_WEBSOCKET_EXTENSIONS in resp.headers:


Use resp.headers.get(hdrs.SEC_WEBSOCKET_EXTENSIONS), do getting the header only once.

asvetlov · 2017-09-22T07:42:26Z

aiohttp/http_websocket.py

+class WSHandshakeError(Exception):
+    """WebSocket protocol handshake error."""
+
+    def __init__(self, message):


Constructor from parent class is ok, isn't it?
Drop __init__ function.

asvetlov · 2017-09-22T07:46:13Z

docs/client_reference.rst

      :param aiohttp.BasicAuth proxy_auth: an object that represents proxy HTTP
                                           Basic Authorization (optional)

+      :param bool compress: Enable Per-Message Compress Extension support


Update method signature too.

asvetlov · 2017-09-23T17:19:14Z

Last question: should we enable websockets by default?
It could break existing programs. But we can fix bugs quickly I hope.
As an option for smooth transition we could use 0 compression by default in next release and switch to 15 in after next.
Thoughts?

fanthos · 2017-09-23T17:53:01Z

I think we can disable compression for client side by default, maybe need more test with other server software to make sure they are compatible.
The server code is tested with Firefox and Chrome and works as excepted.

asvetlov · 2017-09-23T17:54:35Z

@fanthos sounds good

asvetlov · 2017-09-23T17:57:35Z

WebSocketResponse(..., compress=True) should be documented

Disable websocket compression on client side by default

asvetlov · 2017-09-27T07:47:56Z

Thanks!

lock · 2019-10-28T17:04:51Z

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].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

Add initial support for PerMessage Deflate

00bae99

fanthos added 3 commits September 17, 2017 02:20

Add Client websocket deflate init support.(not tested)

405be9a

Add no takeover extension to server and client.

Update parser to support compress parse

5017f01

fix for CI

70ab0e9

TODO: add tests

fanthos added 3 commits September 17, 2017 15:26

add document for compress

e0a459c

Improve compress detect logic

52de28c

Add parser test for rsv1

Complete Client websocket deflate support

048a8f2

Add tests

fanthos force-pushed the master branch from 03f06e3 to 048a8f2 Compare September 17, 2017 11:06

fanthos added 4 commits September 17, 2017 19:06

Merge branch 'master' into master

72127c5

update

4c55414

sort import

3c6029e

Add more tests

79714a7

Client deflate support should now more complete fix linting

fanthos force-pushed the master branch from c55c5c0 to 79714a7 Compare September 17, 2017 15:43

fanthos added 2 commits September 17, 2017 23:53

fix typo

e51f48d

add 2 more tests

f8f0f21

asvetlov reviewed Sep 17, 2017

View reviewed changes

fanthos and others added 7 commits September 18, 2017 16:58

Using regex to parse header

63bd563

Fix client code Add more tests

remove dead code

895ea75

coverage

adf31e0

Merge branch 'master' into master

39a15ef

Add reader deflate flag support.

b45c0ba

If reader created with compress=0, it will raise PROTOCOL_ERROR if rvb1 = 1.

Merge branch 'master' of https://github.com/fanthos/aiohttp

a04c1c9

Merge branch 'master' into master

c3c640e

asvetlov reviewed Sep 19, 2017

View reviewed changes

fanthos and others added 3 commits September 19, 2017 23:39

fixed

99b50d5

Merge branch 'master' of https://github.com/fanthos/aiohttp

268da36

Merge branch 'master' into master

2e961b3

asvetlov requested changes Sep 21, 2017

View reviewed changes

fanthos added 2 commits September 22, 2017 00:59

Using array.join instead of strcat.

b91d1c7

Using exception instead of return value.

Merge branch 'master' of https://github.com/fanthos/aiohttp

c95a7f4

asvetlov requested changes Sep 22, 2017

View reviewed changes

fanthos and others added 2 commits September 22, 2017 19:22

fix

c911749

Update http_websocket.py

01ea9b1

Merge branch 'master' into master

97a2c1e

Update 2273.feature

c98c777

fanthos and others added 5 commits September 24, 2017 02:07

add docs for server

fff88de

Merge branch 'master' into master

26e7bac

Merge branch 'master' into master

e5c651d

Update client.py

8da8460

Disable websocket compression on client side by default

Update client_reference.rst

22fb3f9

asvetlov approved these changes Sep 26, 2017

View reviewed changes

asvetlov merged commit c988fd0 into aio-libs:master Sep 27, 2017

asvetlov mentioned this pull request Oct 2, 2017

Compression Extensions for WebSocket #673

Closed

jzwinck mentioned this pull request Apr 24, 2018

[QUESTION] Does WebSocketResponse supports gzip transparent compression? #1440

Closed

lock bot added the outdated label Oct 28, 2019

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

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019

Uh oh!

Conversation

fanthos commented Sep 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

Uh oh!

fafhrd91 commented Sep 16, 2017

Uh oh!

fanthos commented Sep 17, 2017

Uh oh!

codecov-io commented Sep 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Sep 23, 2017

Uh oh!

fanthos commented Sep 23, 2017

Uh oh!

asvetlov commented Sep 23, 2017

Uh oh!

asvetlov commented Sep 23, 2017

Uh oh!

asvetlov commented Sep 27, 2017

Uh oh!

lock bot commented Oct 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fanthos commented Sep 16, 2017 •

edited

Loading

codecov-io commented Sep 17, 2017 •

edited

Loading