Support brotli compression of HTTP responses #2945

roganov · 2018-04-18T12:29:39Z

What do these changes do?

This PR adds support for br (brotli) content-type compression.

I have also refactored compressors and decompressors to a separate module (aiohttp.compression) because there are some differences between zlib's and brotli's intefraces.

Related tests have also been refactored a bit, now pytest's parametrization is used more aggressively.

For now brotli compression of websocket responses is not supported,

Are there changes in behavior for the user?

aiohttp now can compress responses using brotli compression algorithm if brotlipy is installed.

Related issue number

Closes #2518

asvetlov · 2018-04-18T12:35:32Z

Thanks for PR.
I have some notes, will make a detailed review in the evening.

codecov-io · 2018-04-18T15:45:48Z

Codecov Report

Merging #2945 into master will decrease coverage by 0.17%.
The diff coverage is 89.58%.

@@            Coverage Diff             @@
##           master    #2945      +/-   ##
==========================================
- Coverage   97.99%   97.81%   -0.18%     
==========================================
  Files          40       41       +1     
  Lines        7520     7613      +93     
  Branches     1318     1333      +15     
==========================================
+ Hits         7369     7447      +78     
- Misses         48       56       +8     
- Partials      103      110       +7

Impacted Files	Coverage Δ
aiohttp/http_writer.py	`100% <100%> (ø)`	⬆️
aiohttp/client_reqrep.py	`97.27% <100%> (ø)`	⬆️
aiohttp/multipart.py	`95.41% <100%> (-0.06%)`	⬇️
aiohttp/http_parser.py	`97.98% <100%> (-0.07%)`	⬇️
aiohttp/web_response.py	`98.14% <100%> (-0.04%)`	⬇️
aiohttp/compression.py	`87.8% <87.8%> (ø)`

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 26802e0...91e5dc3. Read the comment docs.

asvetlov

Good work.
I have some notes about implementation but very like the overall design.

Switching to ContentCoding usage instead of strings in both client and server API would be nice (but we should keep supporting strings during deprecation period)

asvetlov · 2018-04-20T08:42:11Z

aiohttp/compression.py

+
+    @classmethod
+    def get_from_accept_encoding(cls, accept_encoding):
+        accept_encoding = accept_encoding.lower()


Would be nice to parse AcceptEncoding header properly and respect weights instead of selecting the first supported compression based on server preference order.
Multiple Accept-Encoding headers are allowed by HTTP spec also.
We can either accept a list of headers (req.headers.getall('Accept-Encoding')) or headers param itself.

asvetlov · 2018-04-20T08:47:03Z

aiohttp/compression.py

+                return coding
+
+    @classmethod
+    def values(cls):


What is the usage of the method?
Could we utilize Enum.__members__ or another existing Enum API?

asvetlov · 2018-04-20T08:49:04Z

aiohttp/compression.py

+        return _values
+
+
+def get_compressor(encoding):


Could we switch here to ContentCoding enum? Maybe make it a ContentCoding member function?

The same for decompressor.

asvetlov · 2018-04-20T08:50:54Z

aiohttp/compression.py

+        self._finished = False
+
+    @classmethod
+    def gzip(cls):


I prefer making GZipCompressor class derived from ZlibCompressor over class methods for gzip and deflate compressors building.

asvetlov · 2018-04-20T08:54:32Z

aiohttp/compression.py

+        return self._compress.finish()
+
+
+def decompress(encoding, data):


As I see the function is used only once in multipart module.
Maybe direct usage of decompressor classes there is enough?

roganov · 2018-04-25T18:11:28Z

Just a status update: have been busy lately, but planning to finish by the end of the week.

asvetlov · 2018-04-25T19:54:12Z

Sounds good

asvetlov · 2018-06-21T05:19:01Z

Is there a progress on it?

roganov · 2018-06-21T17:13:10Z

HI
I'm very sorry it's taking me so long. I plan to finish next week.

asvetlov · 2018-06-21T17:46:19Z

No problem

asvetlov · 2018-11-30T21:38:48Z

There are many conflicts now.
I suspect after applying black the difference will grow.
@roganov do you want to make it done?
Otherwise I'll close the PR

roganov · 2018-12-03T09:55:28Z

I'll close this one and reopen when I'm ready. Really sorry, don't have enough free time currently.

lock · 2019-12-03T10:17:41Z

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.

roganov added 2 commits April 18, 2018 00:35

Refactor compression and decompression

65fcaf3

Support brotli compression

6d9a1cd

Fix Windows tests

91e5dc3

roganov force-pushed the brotli-everywhere branch from cfd1a80 to 91e5dc3 Compare April 18, 2018 15:16

asvetlov requested changes Apr 20, 2018

View reviewed changes

roganov closed this Dec 3, 2018

lock bot added the outdated label Dec 3, 2019

lock bot locked as resolved and limited conversation to collaborators Dec 3, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support brotli compression of HTTP responses #2945

Support brotli compression of HTTP responses #2945

roganov commented Apr 18, 2018 •

edited

Loading

asvetlov commented Apr 18, 2018

codecov-io commented Apr 18, 2018 •

edited

Loading

asvetlov left a comment

asvetlov Apr 20, 2018

asvetlov Apr 20, 2018

asvetlov Apr 20, 2018

asvetlov Apr 20, 2018

asvetlov Apr 20, 2018

roganov commented Apr 25, 2018 •

edited

Loading

asvetlov commented Apr 25, 2018 via email •

edited

Loading

asvetlov commented Jun 21, 2018

roganov commented Jun 21, 2018

asvetlov commented Jun 21, 2018

asvetlov commented Nov 30, 2018

roganov commented Dec 3, 2018

lock bot commented Dec 3, 2019

		return self._compress.finish()


		def decompress(encoding, data):

Support brotli compression of HTTP responses #2945

Support brotli compression of HTTP responses #2945

Conversation

roganov commented Apr 18, 2018 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

asvetlov commented Apr 18, 2018

codecov-io commented Apr 18, 2018 • edited Loading

Codecov Report

asvetlov left a comment

Choose a reason for hiding this comment

asvetlov Apr 20, 2018

Choose a reason for hiding this comment

asvetlov Apr 20, 2018

Choose a reason for hiding this comment

asvetlov Apr 20, 2018

Choose a reason for hiding this comment

asvetlov Apr 20, 2018

Choose a reason for hiding this comment

asvetlov Apr 20, 2018

Choose a reason for hiding this comment

roganov commented Apr 25, 2018 • edited Loading

asvetlov commented Apr 25, 2018 via email • edited Loading

asvetlov commented Jun 21, 2018

roganov commented Jun 21, 2018

asvetlov commented Jun 21, 2018

asvetlov commented Nov 30, 2018

roganov commented Dec 3, 2018

lock bot commented Dec 3, 2019

roganov commented Apr 18, 2018 •

edited

Loading

codecov-io commented Apr 18, 2018 •

edited

Loading

roganov commented Apr 25, 2018 •

edited

Loading

asvetlov commented Apr 25, 2018 via email •

edited

Loading