Add support for precompressed (for example gzip) content #108

gmokki · 2016-04-11T10:00:44Z

Disabled by default (should it be enabled by default?).
There should be no overhead if disabled.
If opts.precompressed is set to true, tries to serve both .br and .gz files, but custom configuration (encoding to extension mappings) is also supported

If enabled then for every file that is served we look up each configured file extension (even if browser does not request compressed variants) and if any are found we set the Vary header. This is required for caching proxies that the clients my be using to work correctly.

If matching file between Accept-Encoding and enabled precompression formats is found then the compressed file contents are served with Content-Encoding header. If multiple supported matches are found the first configured in the preferences will be used.

dougwilson · 2016-05-24T04:50:00Z

Thanks, @gmokki! I have a question regarding the added functionality:

If I were to enable precompressed with this code and I have a folder full of *.tar.gz files, how can I prevent the clients from accessing the files as *.tar (as this will cause the client to automatically ungzip potentially giant tarballs)?

gmokki · 2016-05-24T06:17:48Z

I do not think there can be anything to prevent that (at least with this patch). It works the same way as apache/nginx etc when configured with support for precompressed resources.
I'd say the main defence is that one does not link to .tar files, but directly to .tar.gz files. If you directly refer to the .gz file the patch does not set the content-encoding headers and thus does not cause the client to decompress the content.

gmokki · 2016-05-27T13:47:01Z

More concrete answer:
Assuming you have files file.tar and file.tar.gz in folder where the uncompressed file.tar.gz contents matchs exactly to to the file.tar.

Before this patch:

When requesting file.tar.gz you get the exact (compressed) file.
- the Accept-Encoding header with or without gzip has no effect
When requesting file.tar you get the exact (uncompressed) file.
- the Accept-Encoding header with or without gzip has no effect

After this patch:

When requesting file.tar.gz you get the exact (compressed) file.
- the Accept-Encoding header with or without gzip has no effect
When requesting file.tar
- without Accept-Encoding: gzip header you get the exact (uncompressed) file
- with Accept-Encoding: gzip header you get the tar.gz file with Content-Encoding: gzip response
  - the only difference is that the transferred wire data is smaller and the http client transparently decompresses to the file.tar as requested

For applications using the precompressed feature they just need to make sure that the contents of the uncompressed and compressed files match. As long as that holds no difference can be observed by the compilant http client users.

dougwilson · 2016-05-27T14:44:51Z

Ah, @gmokki, gotcha. Yea, I looked at the nginx source and what they do is just unconditionally add .gz to the file name when the client wants to get the gzip encoding, and there are no other real checks. I think this is probably OK, but I think the PR needs a lot more documentation fleshed out.

gmokki · 2016-05-30T11:31:50Z

Can you give a hint of what kind of extra documentation in addition to the short README.md documentation you would like to have.

dougwilson · 2016-05-30T21:56:17Z

Hi @gmokki, currently the README change in this pull request is only the following:

Enable or disable serving of precompressed content, defaults to false.
If set to true checks for brotli and gzip. An example value to serve
other formats: [{encoding: 'bzip2', extension: '.bz2'}]

There are a lot of questions that raises, including how to even use it. What values can I use for encoding? What about extension? How does this module determine which file to serve? How do I layout the files on the disk? How can we avoid questions like the one I had above?

I'm not asking you these questions, they are just what comes to mind when reading the README. We'll need enough information in the README for someone to use the feature. Consider that you may take it for granted what "precompressed" means and how to lay it out, but many people have never done that before.

gmokki · 2016-05-31T20:06:06Z

I added the details you requested into the documentation with much longer clarifications. I also added a few generic tips on offline compression.

dougwilson · 2016-05-31T20:11:23Z

index.js

+  var header = this.req.headers['accept-encoding']
+  if (header) {
+    this._precompressionFormats.forEach(function (format) {
+      if (header.indexOf(format.encoding) >= 0) accepted.push(format.extension)


Looks like this is broken for web browsers that send "no-gzip, deflate", which was common for a time (Apache, and I assume others, understand no-gzip).

This also does not seem to take into account the quality parameters at all, which is very common from CDNs, as they usually want a specific encoding to store.

dougwilson · 2016-05-31T20:12:05Z

Thanks! I just realized I still have not actually looked at the implementation, so doing a little bit now.

dougwilson · 2016-05-31T20:15:08Z

index.js

+  }
+
+  function sendPreferredContent(p, stat, contents) {
+    if (contents.length) self.res.setHeader('Vary', 'Accept-Encoding')


This will overwrite any other Vary already set. Let's append this to the field name if already set instead of overwriting.

hacksparrow · 2016-06-01T12:26:28Z

This is a good feature to have. @gmokki can you can address the issues pointed out by @dougwilson?

gmokki · 2016-06-01T18:57:20Z

Yes, I will definitely try to address all pointed issues in next few days. Thank you for the through review and comments.

dougwilson · 2016-06-01T19:05:15Z

Thanks, @gmokki! It sounds like this won't make it for the Express.js 4.14 release cut tonight, then. It'll probably be a few months before the next minor, and this module is locked to Express.js release schedules, so you'll have plenty of time to address!

gmokki · 2016-06-01T19:39:25Z

I just fixed all the issues that @dougwilson pointed out.

I have already added a encoding quality parsing function to tomcat (https://github.com/apache/tomcat/blob/trunk/java/org/apache/catalina/servlets/DefaultServlet.java#L1096) - I'll try to add it now to here too.
But if that feature does not make it in time would it be possible to merge this without the encoding quality parsing? None of the common browsers send quality values. Actually so far all web servers that I have patched with brotli support just assumed that gzip;q=0 meant that gzip was supported.

dougwilson · 2016-06-01T19:57:05Z

Thanks, @gmokki! It looks like there is still the outstanding stat issue (unless you fixed it in one of the commits?). The quality issue is a real issue, at least from CDNs. I maintain the compression module, which does similar negotiation, and I definitely know people have had real issues with this, and since this serves static files, which are pretty much the main thing to put behind a CDN, then it is an issue here as well, and I would rather get this done right than trying to rush this & make mistakes.

hacksparrow · 2016-06-01T20:01:40Z

+1 for not rushing it.

gmokki · 2016-06-01T20:32:13Z

I just added code for the accept-encoding quality handling.

I just added comment on why the stats are needed always when precompressed option is enabled. If that were not the case the precompression option could default to on.

I think I have now really addressed all comments. Do provide more feedback on bugs/missing features and I'll try to address them too.

dougwilson · 2016-06-01T20:35:34Z

Thanks, @gmokki! In order to get the 4.14 release ready for tonight and our TC meeting, I cannot look any more at this PR for now. I'll take another look tomorrow after 4.14 for sure. There is still the comment regarding the .br file framing vs the stream if you can take a look into that as well!

dougwilson · 2016-06-01T20:38:53Z

Also, the CI tests are still taking their sweet time to run (they still haven't gotten to your pushes from earlier today), so we have to at least wait on those to consider merging.

Also, real quick, I do have a couple more thoughts on the header stuff: have you considered using the modules we have already owned by the Express TC to do some of that heavy lifting? Did they not work out (and would it be possible for us to modify them so they can be used here)? Most of them can be found in https://github.com/jshttp (our organizations are https://github.com/expressjs , https://github.com/pillarjs and https://github.com/jshttp).

gmokki · 2016-06-01T21:10:05Z

I'm not an authority on the brotli file formats nor their official extension.
For the extension I can say that all other web servers I know have the brotli static file serving support (ngingx, tomcat, jetty) and apache examples all refer to extension being .br.
The file format is outside of the scope of this module. It is the users responsibility to make sure that file ending with .gz/.br can be decompressed by the browser.

I originally tried to avoid extra dependencies. But I could use https://github.com/jshttp/vary to modify the vary header and https://github.com/jshttp/negotiator to parse the preferred accept-encoding header. I'll try to modify the code to use them tomorrow. Now that the code has tests for both cases taking them into use should be simple.

dougwilson · 2016-06-01T21:20:28Z

For the extension I can say that all other web servers I know have the brotli static file serving support (ngingx, tomcat, jetty) and apache examples all refer to extension being .br.

That doesn't make it the correct thing to do. For example, I could have just accepted this and been in the same boat. Just because others do something doesn't automatically make it correct. In order to accept this PR as-is, with .br being check by default, it has to make sense, and as such, I need you to perform the investigation I asked about the framing, otherwise it will have to wait until I have time to do that depth of digging, which I will do in the order I have received PRs to be fair to all other people who have open PRs. This PR is currently #82 in the line...

gmokki · 2016-06-02T14:12:20Z

@dougwilson: I think you are correct that adding explicit brotli support is premature. The file format is not yet finalized. I just pushed a new version that removes brotli from the default set of formats and updates the documentation to match.

I also tried using the https://github.com/jshttp/accepts for choosing the best encoding format. Unfortunately that does not yet work because the module does not honor the server preference order for values that have equal quality level in the client request.

dougwilson · 2016-06-03T19:00:50Z

I also tried using the https://github.com/jshttp/accepts for choosing the best encoding format. Unfortunately that does not yet work because the module does not honor the server preference order for values that have equal quality level in the client request.

Is it possible to get that added? I think I remember seeing that wasn't in there when dealing with compression issues, so it would be nice if we can get that there to keep us from continuing to punt on the issue :)

gmokki · 2016-09-16T10:00:22Z

My negotiator pull request jshttp/negotiator#49 is still not merged, but I updated this pull request to use it. I also did some worst case benchmarking: when enabling the precompressed check with one extra stat the performance goes down at most by 20% when serving 1 byte files to localhost with single infinite depth http pipeline connection. I doubt that it is observable in actual use cases.

…the browsers. - optimize png images with pngquant (quality 95) and advpng - optimize svg images with svgo - create precompressed variants of js, css, svg files: gz using zopfli and br using brotli Currently using forked versions of expressjs/send/negotiator while waiting for the PRs to go through: - jshttp/negotiator#49 - pillarjs/send#108

…the browsers. - create precompressed variants of js, css, svg files: gz using zopfli and br using brotli Currently using forked versions of expressjs/send/negotiator while waiting for the PRs to go through: - jshttp/negotiator#49 - pillarjs/send#108

gmokki · 2017-04-10T07:24:20Z

Rebased to fix merge conflicts. Unfortunately the negotatior PR is still pending.

DylanPiercey · 2017-08-08T03:57:40Z

Is there anything that could be done to aid in landing this PR?

dougwilson added the pr label May 24, 2016

dougwilson self-assigned this May 27, 2016

dougwilson added the needs docs label May 27, 2016

dougwilson reviewed May 31, 2016
View reviewed changes

dougwilson removed the needs docs label Jun 1, 2016

dougwilson added the enhancement label Jun 1, 2016

dougwilson mentioned this pull request Jun 26, 2016

Option to prefer gz files. #112

Closed

gmokki changed the title ~~Add support for precompressed (gzip and brotli) content~~ Add support for precompressed (for example gzip) content Jun 27, 2016

gmokki mentioned this pull request Jul 23, 2016

Change resolved Accept-Encoding to honour server side order preferece jshttp/negotiator#49

Closed

gmokki force-pushed the master branch from 6a7574a to 85ad87c Compare July 31, 2016 21:29

gmokki force-pushed the master branch from 99c7aeb to f0f126b Compare September 16, 2016 09:49

gmokki mentioned this pull request Nov 23, 2016

Precompress and optimize static files during build and serve them to browsers HSLdevcom/digitransit-ui#1030

Closed

3 tasks

dougwilson added ideas needs rebase labels Feb 19, 2017

gmokki force-pushed the master branch from f0f126b to 9fced00 Compare April 10, 2017 06:08

Mikko Tiihonen added 2 commits April 10, 2017 09:27

Add support for precompressed (gzip) content

216c3d7

Use new configurable negotiator api for encoding sorting

fddade5

gmokki force-pushed the master branch from 9fced00 to fddade5 Compare April 10, 2017 06:28

gajus mentioned this pull request May 8, 2017

Add an option to try a file based on Accept-Encoding expressjs/serve-static#85

Closed

dougwilson force-pushed the master branch from 4f438e7 to a17c053 Compare October 11, 2018 03:11

coreyfarrell mentioned this pull request Apr 13, 2019

Support for static compression? fastify/fastify-static#104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for precompressed (for example gzip) content #108

Add support for precompressed (for example gzip) content #108

gmokki commented Apr 11, 2016

dougwilson commented May 24, 2016

gmokki commented May 24, 2016

gmokki commented May 27, 2016

dougwilson commented May 27, 2016

gmokki commented May 30, 2016

dougwilson commented May 30, 2016

gmokki commented May 31, 2016

dougwilson May 31, 2016

dougwilson commented May 31, 2016

dougwilson May 31, 2016

hacksparrow commented Jun 1, 2016

gmokki commented Jun 1, 2016

dougwilson commented Jun 1, 2016

gmokki commented Jun 1, 2016

dougwilson commented Jun 1, 2016

hacksparrow commented Jun 1, 2016

gmokki commented Jun 1, 2016

dougwilson commented Jun 1, 2016

dougwilson commented Jun 1, 2016

gmokki commented Jun 1, 2016

dougwilson commented Jun 1, 2016

gmokki commented Jun 2, 2016

dougwilson commented Jun 3, 2016

gmokki commented Sep 16, 2016

gmokki commented Apr 10, 2017

DylanPiercey commented Aug 8, 2017

Add support for precompressed (for example gzip) content #108

Are you sure you want to change the base?

Add support for precompressed (for example gzip) content #108

Conversation

gmokki commented Apr 11, 2016

dougwilson commented May 24, 2016

gmokki commented May 24, 2016

gmokki commented May 27, 2016

dougwilson commented May 27, 2016

gmokki commented May 30, 2016

dougwilson commented May 30, 2016

gmokki commented May 31, 2016

dougwilson May 31, 2016

Choose a reason for hiding this comment

dougwilson commented May 31, 2016

dougwilson May 31, 2016

Choose a reason for hiding this comment

hacksparrow commented Jun 1, 2016

gmokki commented Jun 1, 2016

dougwilson commented Jun 1, 2016

gmokki commented Jun 1, 2016

dougwilson commented Jun 1, 2016

hacksparrow commented Jun 1, 2016

gmokki commented Jun 1, 2016

dougwilson commented Jun 1, 2016

dougwilson commented Jun 1, 2016

gmokki commented Jun 1, 2016

dougwilson commented Jun 1, 2016

gmokki commented Jun 2, 2016

dougwilson commented Jun 3, 2016

gmokki commented Sep 16, 2016

gmokki commented Apr 10, 2017

DylanPiercey commented Aug 8, 2017