Its ideal to set Vary: Accept-Encoding, irrespective of whether gzipped or not

[vipulnsward]

Its ideal to set Vary: Accept-Encoding, irrespective of whether gzipped version exists or not. This is helpful for CDN's to later distinguish assets, based on previous, current copies and introduced gzip version if any.
For ref: https://www.fastly.com/blog/best-practices-for-using-the-vary-header
This change sets Vary header always, to be on safer side

r? @schneems

[prathamesh-sonpatki]

assert_not_nil instead of refute_nil

[schneems]

Can you rebase this branch from master, there's failures on this suite from a bad test that was in master. I believe @prathamesh-sonpatki just fixed it in e55ba53

[schneems]

I'm 👍 on this, seems reasonable.

[vipulnsward]

Updated.

[schneems]

Thanks!

[schneems]

Just considered, maybe there should have been a changelog for this, it is a change in behavior.

[matthewd]

This is incorrect. It was previously setting the header if the path was a candidate for gzipping, regardless of whether the client requested it. Now we just claim that we vary on files where we definitely don't.

[matthewd]

(From my quick read through that article, it's only claiming you should set the header on non-compressed responses where you might otherwise have compressed it, not every single response. At the most extreme, we now claim that a different Accept-Encoding header might have yielded a compressed version of a PNG.)

[vipulnsward]

@matthewd you see some side-effects from this?
Or the comments are related to changelog, and that we should or not have it.

[nateberkopec]

@matthewd I don't think this is "incorrect" (I believe the relevant RFC bits here are all SHOULD and MAY), but it will reduce cache performance for assets which have no gzipped/encoded representation.

Vary: essentially just adds the request's Accept-Encoding header to the resource's cache key. Say I have an application.js resource which is not gzipped/has no gzipped version on my server. Prior to this change, my CDN would serve a single copy of application.js to everyone. After this change, it will serve a different version to each request depending on the Accept-Encoding header of the request. So this will result in unneccessary cache misses for assets which do not have a compressed version.

If you imagine an intermediate CDN cache key as normally looking like this:

"#{hostname}/#{resource_relative_path}"

...it now looks like this after Vary: Accept-Encoding is added to our response:

"#{request_accept_encoding_value}/#{hostname}/#{resource_relative_path}"

You can see why this would be wasteful if the response does not actually have multiple encodings.

[vipulnsward]

@nateberkopec comments was the motivation for the PR. This is especially good to have where sprockets had and then removed and then added back support for default gzip of assets.

[matthewd]

Yeah, "incorrect" was strong.. and the "this" in question was the PR's intention/motivation, rather than the resulting behaviour.

@vipulnsward I'm still not following how this improves things, even for file types that are fundamentally compressible. I believe this should not have been changed, and should be reverted.

[nateberkopec]

If you upgrade sprockets to the version that gzips by default again (I forgot when this happened) and don't change your asset digests, this change will bust the HTTP caches for all of your assets, whereas without this change HTTP caches would still only serve the non-gzipped version even if the request would accept a different encoding.

[nateberkopec]

FWIW I'm also mixed on this change. If you're changing the encodings for a static asset without changing it's digest, you should just bust your caches by incrementing your config.assets.version or changing the digest of the asset somehow. This change decreases cache hit ratios for assets which have no gzipped/encoded representation.

[schneems]

I'm backpedaling now. Thanks for the bump, i'm able to follow what you're saying. I actually think the original behavior was correct. We ALWAYS want set vary header when the gzip asset is available, whether or not the asset being served has been gzipped. That is what that method checks: https://github.com/vipulnsward/rails/blob/067c52f608568e35181830a5c1016e382650e655/actionpack/lib/action_dispatch/middleware/static.rb#L88-L96

We don't want to set Vary for files that will not be served differently based on accept-encoding, such as PNGs.

To note, it looks like this isn't the first time that @matthewd corrected me about the correct logic #16466 (comment) thanks 😉

I'm going to revert this, sorry for the confusion, and for not remembering that thread earlier.

[schneems]

Thanks for the double check @vipulnsward sorry for the about face, the good news is that this still counts as a commit 😉

[vipulnsward]

hmm, check this back again. Thanks for the comments!