search

juliocesar / rack-pagespeed

Page optimizations done at the Rack level
http://rack-pagespeed.heroku.com
244 stars 22 forks source link

Content-Length Header not Being Updated #2

Closed logicaltext closed 14 years ago

logicaltext commented 14 years ago

Rack::Bundle should update the 'Content-Length` header (if set) to reflect the change in the response length. Here's a patch that makes this so.

EDIT: Sorry, the spec wasn't properly worded (it was originally in its own 'Content-Length header' context). I've fixed this here.

juliocesar commented 14 years ago

Hey hey! Having a look now. Thanks for that!

logicaltext commented 14 years ago

Sure, no worries! I've been playing around with Rack::Bundle this weekend (I'll be using it for an upcoming project I'm deploying on Heroku), and I started to play around with some potential performance improvements (particularly serving the bundles via Rack::File, and also avoiding unnecessary reads when possible). I've tried to keep my commits small, so you can cherry-pick some of them if they take your fancy!

juliocesar commented 14 years ago

Ok, I read through your specs, and it seems they're asserting if Rack::ContentLength is there, then the Content-Length header will get set to the correct value. The thing is: that test should sit in the Rack::ContentLength codebase rather than Rack::Bundle, because in essence you're checking if Rack::ContentLength does it's job. Or rather, you're asserting if things will be alright if Rack::ContentLength is present, and the answer to that will always be yes provided it works (and it does).

This is a tough question to answer. I could recommend people to use Rack::ContentLength alongside with RB if they're using "pure Rack", and considering the approach of having each middleware do it's own little job, that would be the way to go. On the other hand, frameworks such as Rails and Sinatra have their own way to handle Content-Length, which might make it unnecessary.

Did you experience any breakage with a framework by any chance?

logicaltext commented 14 years ago

that test should sit in the Rack::ContentLength codebase rather than Rack::Bundle, because in essence you're checking if Rack::ContentLength does it's job. Or rather, you're asserting if things will be alright if Rack::ContentLength is present, and the answer to that will always be yes provided it works (and it does).

Actually, that wasn't my intention. My spec happens to use Rack::ContentLength, because it was a quick and easy way to add the Content-Length header, but you can't guarantee that people are using Rack::ContentLength in their Rack stacks, and even if they are, it might be upstream of Rack::Bundle. Because Rack::Bundle does change the content-length (by replacing the links to JavaScript and CSS files), it should be responsible for changing the Content-Length header.

I could recommend people to use Rack::ContentLength alongside with RB if they're using "pure Rack", and considering the approach of having each middleware do it's own little job, that would be the way to go.

As a piece of middleware, I think Rack::Bundle should stand on its own and not have to depend on Rack::ContentLength being present downstream. That's why I think Rack::Bundle should handle this itself.

Did you experience any breakage with a framework by any chance?

Not with a framework, but if you use Rack::Lint in the spec (along with Rack::ContentLength), and Rack::Bundle doesn't adjust the content-length, Rack::Lint will complain. So, if someone was using a piece of middleware that adjusted the Content-Length header, and then they also used Rack::Bundle after the header was adjusted, the value of the header will be wrong.

For more about middleware changing the Content-Length header, see the comments on this screencast over at Railscasts, particularly comments 10 and 12. As far as my specs are concerned, they were very much inspired by some specs for the JSONP middleware in the rack-contrib project.

juliocesar commented 14 years ago

Understood. Ok, the approach of "update if it exists" sounds sensible. I figured forcibly updating the length (as in adding a Content-Length irregardless of it existing or not) would mean doing more than it should do, on top of me not being quite sure on what to do about UTF-8 in this case.

I'll add in it. Thanks a lot! Really appreciated this discussion.

logicaltext commented 14 years ago

Ok, the approach of "update if it exists" sounds sensible. I figured forcibly updating the length (as in adding a Content-Length irregardless of it existing or not) would mean doing more than it should do

Agreed. From my (limited) research, it seems like the Content-Length header isn't mandatory, so it makes sense for Rack::Bundle to adjust it only if it's already there. That's also the way that the JSONP middleware does it, and it seems sensible.

on top of me not being quite sure on what to do about UTF-8 in this case.

That's a really good point. I must confess I'm a little confused by UTF-8 and character encodings (particularly the differences between the different versions of Ruby). I checked out the source of Rack::ContentLength, and it uses #bytesize rather than #length, and I also found a somewhat related conversation that talks about using Rack::Utils#bytesize instead of String#length. Having said that, I'm not sure if it's necessary on Ruby >= 1.8.7.

I'll add in it. Thanks a lot! Really appreciated this discussion.

Great. Thanks!

juliocesar commented 14 years ago

Done! I went for String#length, which I'm positive it's UTF-8 unsafe. It'll do for now though. Bumped to 0.2.1 and pushed to Rubygems.

Thanks!