Rack::File serves gzipped assets when available #479

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet

@raggi, After poking around a bit, I think Rack::File is the cleanest place to put this change.

This is a very small addition to Rack::File. I think adding it to Rack::Static would be slightly more complex. For example, since Rack::File would set all compressed content-types to "application/gzip", we would need to duplicate the Rack::File mime type logic in Rack::Static to correct this.

I looked at the header_rule stuff in Rack::Static, but couldn't see any graceful way to reuse it.

LMK what you think.

Owner

raggi commented Jan 4, 2013

Looks good, one last thing:

I would prefer that this was opt-in (although I'm aware we're running out of initializer params). In the very least, I would prefer it had an option to opt-out.

Thoughts?

Contributor

hannesg commented Jan 4, 2013

As I read this the .gz stuff has to be in the same directory as the original files. Can we always assume this? I think it's reasonable to make this configurable alongside the opt-out.

@raggi, I changed #initialize to take an options hash instead and made :serve_gzip opt-in.

I was also considering splitting up Rack::File into multiple classes. It's getting a little crowded. It would work something like this:

  # Rack::Files contains the functionality for parsing and validating PATH_INFO
  # Rack::File is responsible for serving one and only one file, and handling Ranges 
  new Rack::Files('assets', :file_server => Rack::File)

  # You can opt into gzip functionality like so.  Rack::Gzfile would be responsible for deciding whether to server
  # a gzipped file and would delegate the serving functionality to a wrapped Rack::File object
  new Rack::Files('assets', :file_server => Rack::GzFile)

LMK what you think. This might be better for another pull request.

@hannesg, I wrote this specifically to handle the gzipped files that Sprockets generates next to the uncompressed assets. I think this is a pretty standard practice.

Do you know of any use cases where the compressed files are stored in a different directory? I've not heard of any. But if there is a use case I'm missing, then maybe we should add that option.

Also, notice that I removed this false-positive test:

should "not set Content-Type if the mime type is not set" do    
  req = Rack::MockRequest.new(Rack::Lint.new(Rack::File.new(DOCROOT, nil, nil)))
  res = req.get "/cgi/test"
  res.should.be.successful
  res['Content-Type'].should.equal nil
end

This test was passing but the feature wasn't working. It seemed like an odd feature to have, so I just removed it.

What about a single gzip_path_generator option? If it's present, it receives the original path and is expected to generate the path to the gzipped version. The default could be lambda { |path| "#{path}.gz" } (or nil for opt-in behavior).

@jamesarosen That would probably be the best way to implement @hannesg's suggestion. But AFAIK, everyone that is going to use this feature is going to have gzipped files in the same directory with the same name plus ".gz". I'd rather keep it simple unless we see that people actually need this flexibility.

Owner

raggi commented Jan 22, 2013

Apologies, but I'm uncomfortable with the API changed still (particularly the files constructor, which already changed recently).

It's possible, although a little convoluted, to create a temporary workaround by altering path_info to include a .gz, and catching X-Cascade, undoing and retrying when 404'ing.

If we can get this in without changing public APIs significantly, then I'll reconsider adding it to a future 1.5 release.

We could create a Rack::GzFile app that wraps Rack::File and adds the additional functionality. We would have to duplicate the part of Rack::File that looks up the mime type, which I don't really like. But we wouldn't have to touch the public API. Sound good?

maletor commented Feb 21, 2013

Agree with @raggi, would like this, but not the way it changes the public API.

Currently using https://github.com/romanbsd/heroku-deflater/ as a workaround.

I'm happy to do another iteration on this, but I'd like to get some feedback on the proposed API before I do any more work on it.

What does everyone think of creating a new Rack::GzFile app that wraps Rack::File?

maletor commented Feb 26, 2013

I like that idea.

Contributor

gioele commented Feb 26, 2013

@jbaudanza:

We would have to duplicate the part of Rack::File that looks up the mime type, which I don't really like.

Can't that part be split out into an helper used by Rack::File, Rack::GzFile and whatever other middleware need to use it?

Ok I've created a new Rack::GzFile, as discussed. Rack::File is untouched. LMK what you think.

maletor commented Feb 27, 2013

I do fancy this. 👍

For the time being, I've packaged this functionality into a gem:

https://github.com/jbaudanza/action_dispatch-gz_static

This should be useful for anyone running the Rails asset precompiler on Heroku.

michrome commented Apr 3, 2013

👍

Hey guys, any thoughts on this? If this isn't going to make it into Rack, I may reopen the issue on the rails repo.

👍

I'd like to see this as well. 👍

@raggi any thoughts?

balexand commented Aug 1, 2013

@jbaudanza Is there a way to use your Rack::GzFile with Rack:::Static? Possibly this line should be changed to something like:

@file_server = options[:file_server] || Rack::File.new(root)

That way, an instance of Rack::GzFile can be passed in as an option. Or better yet Rack::GzFile could be the default, but I'm not sure if the maintainers of this gem would agree with that.

@balexand Yes, definitely. But I'd like for one of the rack maintainers to chime in before I do any more work on this.

I had also intended on submitting a similar change for ActionDispatch::Static in Rails.

@balexand balexand commented on the diff Aug 1, 2013

lib/rack/gz_file.rb
+ @file_server = Rack::File.new(root, headers, default_mime)
+ @default_mime = default_mime
+ end
+
+ def call(env)
+ path_info = env['PATH_INFO']
+
+ status, headers, body = @file_server.call(
+ env.merge('PATH_INFO' => path_info + '.gz')
+ )
+
+ gzip_available = status != 404
+
+ if !gzip_available || env['HTTP_ACCEPT_ENCODING'] !~ /\bgzip\b/
+ status, headers, body = @file_server.call(env)
+ else
@balexand

balexand Aug 1, 2013

@jbaudanza While testing this in development, I got this Rack::Lint error. Apparently, the Content-Type shouldn't be set for 304 Not Modified response.

image

I solved this by changing this line to:

elsif status != 304

I don't think this is the best solution. I'd be happy to propose a more robust solution if the maintainers of Rack have any interest in this pull-request.

@jbaudanza

jbaudanza Aug 1, 2013

Thanks. good catch!

@jjb

jjb Nov 24, 2013

@jbaudanza remember to fix this...

@jbaudanza

jbaudanza Nov 25, 2013

@jjb holding off on any work on this until I get a nod from a maintainer.

Contributor

schneems commented Oct 31, 2013

Got lead here via a discussion on the Heroku forums https://discussion.heroku.com/t/rack-zippy-gem-serves-your-gz-assets-on-heroku/206/5 . I'm generally 👍 on the idea.

@jjb jjb commented on the diff Nov 24, 2013

test/spec_gz_file.rb
+ File.size(DOCROOT + '/cgi/assets/compress_me.html').to_s)
+ end
+
+ should "serve a compressed file" do
+ res = request.get('/cgi/assets/compress_me.html',
+ 'HTTP_ACCEPT_ENCODING' => 'gzip')
+
+ gz = Zlib::GzipReader.new(StringIO.new(res.body))
+ gz.read.should.equal "Hello, Rack!"
+ res.headers['Vary'].should.equal 'Accept-Encoding'
+ res.headers['Content-Encoding'].should.equal 'gzip'
+ res.headers['Content-Type'].should.equal 'text/html'
+ res.headers['Content-Length'].should.equal(
+ File.size(DOCROOT + '/cgi/assets/compress_me.html.gz').to_s)
+ end
+end
@jjb

jjb Nov 24, 2013

should put a newline here

jjb commented Nov 25, 2013

i wonder if perhaps it would be more appropriate for this middleware to live alongside the regular Rack::File instead of replacing it. it could only serve gz when appropriate, ignore everything else. in a sense this makes the code redundant because you have file-serving code in multiple middleware. but i think overall it will make the code smaller and make its intent more clear and its behavior more bounded.

going even further: instead of serving the file, maybe it should rewrite the request. so if a request comes in for foo.js and accepts gz and foo.js.gz is present, the request is rewritten to be requesting foo.js.gz and then handed down to Rack::File. This will further decrease the code size and complexity.

name it: Rack::AcceptsToFilenameConverter (not really)

@jjb I'm not sure I follow your suggestion. The current implementation of GzFile already delegates most of the file serving functionality to a composed instance of Rack::File. Are you suggesting that instead of composition, we have the caller put the Rack::File middleware downstream manually?

jjb commented Nov 25, 2013

@jbaudanza sorry, wasn't clear

idea 1 is to only use the composed instance to serve .gz files. ignore other files. it's up to the user to include another instance of Rack::File downstream as they see fit. users' needs will vary, so this allows more flexibility.

idea 2 (which i think is even better) is to not have your code do any file serving. only rename the incoming request and send it downstream, presumably to a Rack::File which will then serve the .gz file.

make sense?

Makes sense. I understand the desire to make the middleware more flexible, but in this instance I think it makes sense to leave GzFile tightly coupled with Rack::File. What GzFile is doing is very specific to serving files off of a filesystem, and I don't think it would make sense to have anything downstream other than Rack::File.

On the other hand, let me know if there's another use case I'm missing.

It's interesting to discuss. But, as I said, I'm not planning on doing any more work on this until I get a nod from a maintainer. For now, I moved this functionality into another gem that I use on my projects.

jjb commented Nov 25, 2013

gotcha

i just experimented with my idea 2 and got it working, but encountered a problem. after asking the downstream Rack::Static to serve the file, the .js.gz file is delivered with content type application/x-gzip and no encoding.

jjb commented Nov 29, 2013

@jbaudanza behold, the evolution and manifestation of all the points I was previously clumsily making, in code form! jbaudanza/action_dispatch-gz_static#3

@spastorino spastorino modified the milestone: Rack 2.0, Rack 1.6 Nov 27, 2014

Contributor

vipulnsward commented Oct 15, 2015

Looks like this got lost. Anything needed to get this past the line?

@vipulnsward This functionality now exists in Rails, which is probably where it belongs. Is there any interest in having this in Rack? I was waiting for feedback from a maintainer before doing any more work on it.

Contributor

vipulnsward commented Oct 15, 2015

@jbaudanza maybe I missed this, where in Rails?

Contributor

vipulnsward commented Oct 15, 2015

Ah, got it. Thanks.

Contributor

schneems commented Oct 15, 2015

I added it here rails/rails#16466

gumatias commented Jun 4, 2016

Are there still any interest in merging this PR? It's been open for over 3 years and no activity for the past 8 months.

jjb commented Jun 4, 2016

I have no significant interest, because it's now in rails 4.2+ (if i'm reading rails/rails@90006ac correctly). @jbaudanza, what do you think?

In general it does seems appropriate for this functionality to live in Rack instead of Rails, but I'm saying that without first examining if there is anything special about the Rails implementation and/or what the history is of other Rails middleware being developed independent of Rack or rack-contrib.

Contributor

schneems commented Jun 4, 2016

I wrote the Rails implementation, mostly cribbed from a few other implementations such as this one. I'm torn, I would love to move that behavior to Rack so other frameworks can get the benefit.

I see one downside of making it a default and one complicating factor. The downside is that there is a perf cost for the extra file lookup in the Rails case we can safely assume that you'll have gzipped files since we also generate gzipped files via asset pipeline. That's an assumption we can't make with all other frameworks. The complicating factor is that different people may desire custom functionality for example they may want to support browsers that support brotli in addition to those that support gzip. I'm guessing in the future there will be even more file formats and potentially even more logic that we want to support.

jjb commented Jun 4, 2016

It could be a standalone middleware or a configurable feature on Rack::File.

multiple file types would perhaps be easy to support with a simple mime type / extension mapping

probably the more important question is why does Rails maintain ActionDispatch::Static instead of using Rack::File. Seems like it would be a noble goal to either

  1. use it and contribute changes to it
  2. subclass it
  3. use it alongside another complementary middleware

(that's a big topic and i'm not proposing we dive into that here, but i'm just noting it's probably a more useful discussion to have than the one about this particular PR/functionality).

Contributor

schneems commented Jun 5, 2016

why does Rails maintain ActionDispatch::Static instead of using Rack::File

ActionDispatch::Static does use Rack::File however Rails supports things that Rack::File doesn't. The biggest thing that comes to mind is the html index rendering support. For example if you go to http://localhost:3000/home then it will render public/home.html if it exists or public/home/index.html if that exists.

jjb commented Jun 5, 2016

ActionDispatch::Static does use Rack::File

o rite 😊

Owner

raggi commented Jun 5, 2016

I thought that was added to file a while back, or maybe that was dir?
On Jun 4, 2016 6:48 PM, "John Bachir" notifications@github.com wrote:

ActionDispatch::Static does use Rack::File

o rite 😊


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#479 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAABXHGVaOuOgUBYQ0QEyOvTZfBy0GQWks5qIirzgaJpZM4AVk2u
.

Owner

raggi commented Jun 5, 2016

The bigger problem trying to merge rails static and racks file here is that rails makes certain design assumptions that turn up in many places. This isn't just limited to gzip serving, it's also encoding decisions (rails can assert utf-8 defaults, but rack does not limit itself this way). The rails gzip serving is actually limited to specific content types, which is not the approach one would take for a generic pre-computed-gzip serving implementation (such as the one in nginx). These concerns are subtle, and a glance at the code won't make them obvious, but they're distinct differences in implementation goals that end up as bugs, most frequently for non-US users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment