Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove header bloat introduced by BestStandardsSupport middleware #8914

Merged
merged 1 commit into from
Jan 16, 2013

Conversation

nilbus
Copy link
Contributor

@nilbus nilbus commented Jan 12, 2013

After the Rails server ran for long enough, Chome started returning a net::ERR_RESPONSE_HEADERS_TOO_BIG error on every page load, and curl would error with curl: (27) Avoided giant realloc for header (max is 102400)!. A server restart would reset the headers and made the problem temporarily go away.

Investigating showed that the same IE=Edge, headers were being appended on every request until finally the header size exceeded the 100k limit that browsers impose.

This patch ensures that the IE=Edge and other header variables only get appended to the X-UA-Compatible header if they are not there already, and it makes the test pass that I modified to catch the problem.

The commit that introduced this problem (d8c1404, @nikitug) was applied to both master and 3-2-stable, so this fix should be backported to 3-2-stable as well.

@nilbus
Copy link
Contributor Author

nilbus commented Jan 12, 2013

@carlosantoniodasilva, could you look at this since you originally pulled in the affected code? Thanks!

@carlosantoniodasilva
Copy link
Member

@nilbus thanks, I'll take a look and reply soon.

@@ -17,7 +17,11 @@ def call(env)
status, headers, body = @app.call(env)

if headers["X-UA-Compatible"] && @header
headers["X-UA-Compatible"] << "," << @header.to_s
@header.split(',').each do |component|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what the performance is on introducing a split/each in middleware.

@steveklabnik
Copy link
Member

This obviously needs fixed, yeah.

@nilbus
Copy link
Contributor Author

nilbus commented Jan 13, 2013

I was wondering about that too. I'll benchmark and see if I can get the time down if it's significant.

@guilleiguaran
Copy link
Member

We decided to remove BestStandardsSupport middleware in favor of config.action_dispatch.default_headers

@nilbus
Copy link
Contributor Author

nilbus commented Jan 13, 2013

@guilleiguaran, when will that happen? Depending on how soon, maybe this should be applied anyway for the mean time.

@steveklabnik, I did some benchmarks using some variants. If I cut out the split/each and just search the original header to see if it contains @header, then it only increases the time 15% instead of 60%.

500,000 repetitions
-------------------
original               0.65483  secs    Fastest
fixed_without_loop     0.773287 secs    15% Slower
fixed_using_index      0.819192 secs    20% Slower
fixed_without_split    1.323478 secs    50% Slower
fixed                  1.640181 secs    60% Slower

@carlosantoniodasilva
Copy link
Member

Can you please rebase from current master and squash your commits? I'll merge after that, thanks.

The same headers were being duplicated on every request.
@nilbus
Copy link
Contributor Author

nilbus commented Jan 16, 2013

Squashed and rebased.

rafaelfranca added a commit that referenced this pull request Jan 16, 2013
Remove header bloat introduced by BestStandardsSupport middleware
@rafaelfranca rafaelfranca merged commit cfdd5cb into rails:master Jan 16, 2013
rafaelfranca added a commit that referenced this pull request Jan 16, 2013
Remove header bloat introduced by BestStandardsSupport middleware
Conflicts:
	actionpack/CHANGELOG.md
@rafaelfranca
Copy link
Member

Thank you

@nilbus
Copy link
Contributor Author

nilbus commented Jan 16, 2013

And thanks for the quick response! I'm glad it's fixed.

@carlosantoniodasilva
Copy link
Member

❤️

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

Successfully merging this pull request may close these issues.

5 participants