Sprockets 4.1.0 much slower due to changed header capitalisation

[Fjan]

Upgrading sprockets from 4.0.3 to 4.1.0 slows it down considerably, my system tests run about 15%-20% slower.

A bisect reveals that this commit is to blame: cdab3b8

Without that commit the performance of version 4.1.0 is identical to 4.0.3.
I think that the lower casing of the headers probably breaks caching somewhere?

[rafaelfranca]

I don't see how that change would slowdown anything in sprockets, that change was just matching the behavior already existing in rack. Maybe it is related to the system test browser?

[Fjan]

That was my thought as well, so it was the last commit I tried to revert when looking into it, but I double confirmed my finding and tried two different browsers for the system test with the same result. Are you not seeing a slow down in system tests with sprockets 4.1.0?

[rafaelfranca]

Nothing noticeable, no. Can you try to reproduce in a new app so we can try to isolate?

[Fjan]

A new app wouldn't have any meaningful tests in them. If it doesn't happen for you then we can park this until we find someone else who can corroborate, perhaps it's something odd in our infrastructure. I can monkey patch for now.

[rafaelfranca]

Yeah, I'll keep this open. I'm really surprised that would have any impact given that is how the rack spec expects headers to be returned, as lowercase strings. Maybe something else is not following the rack spec and expecting thing to not be in lower case.

[rafaelfranca]

@dhh did you notice any slowdown in your system test suite after upgrading sprockets to 4.1.0?

[Fjan]

Ok, I dug into this a bit deeper. It actually comes down to a single header "Cache-Control", as soon as you lower-case that in sprockets it breaks caching and sprockets has to serve up way more assets. On a hunch a grepped the stack for the word "Cache-Control" and I noticed that the rack gem (2.2.3.1) has the word "Cache-Control" in title case: when I monkey patched rack to use the lower case version it worked again.

Long story short: the header "Cache-Control" needs to have the same capitalisation in Rack and in Sprockets. Rack plans to start using lower case headers in rack 3, but uses title case headers in rack 2.

Not sure where to go from here. According to the RFC headers are case insensitive so rack is to blame, but they already fixed that in rack 3. Perhaps we can detect the rack version in Sprockets. Or perhaps this issue is not big enough and it will anyway resolve itself once rack 3 rolls around. I'm simply monkey patching the word "Cache-Control" in the code until then. Feel free to close the issue.

[rafaelfranca]

Thank you for investigating this. I'll just revert t hat change. sprockets doesn't allow rack 3 yet. https://github.com/rails/sprockets/blob/main/sprockets.gemspec#L14

[ioquatix]

Not sure where to go from here. According to the RFC headers are case insensitive so rack is to blame, but they already fixed that in rack 3.

Who is actually using Rack::CACHE_CONTROL? Basically, it the person consuming then headers shouldn't be doing case sensitive key lookup. I saw the same issue in other gems e.g. assuming headers["Location"] is the correct header for redirects when in theory it could be headers["LoCaTiOn"] etc.

[ioquatix]

In other words, if the problem is in a Rack middleware doing response[1][CACHE_CONTROL] I will personally fix it to be case insensitive.

[matthewd]

Code Search found me one mention that might be common/relevant enough to be interesting: https://github.com/marcotc/rack-brotli/blob/bee3dfbbc0f835cecccbd916ffeffb37858c02b7/lib/rack/brotli/deflater.rb#L97 (@Fjan are you using rack-brotli?)

.. and one that's likely less widely-used: (cc @Hamms) https://github.com/code-dot-org/code-dot-org/blob/0054ea63b8d9a09d84d304484f3ffd53b47e1f07/dashboard/config/initializers/no_transform_paths.rb#L21

I did also incidentally notice a couple of places using a similar pattern with other headers, too:

• https://github.com/18F/identity-idp/blob/dd9d24cc16f2da919001d1d0321b80b1322a7ab6/lib/session_store_migration.rb#L44
• https://github.com/ahx/openapi_first/blob/30622e6b29d26edc2e21f9f869f3e8ee23660ec1/lib/openapi_first/response_validation.rb#L28
• https://github.com/thoughtbot/action_client/blob/2872eaeedcd31f269697c6717a32652bc6f672e6/lib/action_client/base.rb#L180

(a string-based equivalent is likely full of false positives, but I still wonder how many are real / [already wrong, but nevertheless] going to be similarly affected)

[Fjan]

@ioquatix > Who is actually using Rack::CACHE_CONTROL?

In rack I see these two lines doing a case-sensitive match for the existence of the Cache-Control header:
https://github.com/rack/rack/blob/03dd490d4e62d83ca148fdf0883d2ce3b191dfee/lib/rack/response.rb#L302
https://github.com/rack/rack/blob/03dd490d4e62d83ca148fdf0883d2ce3b191dfee/lib/rack/etag.rb#L37
My guess is it's one of those two that fails or interferes when we down case the header in sprockets. But that's just a guess, it could also be a middleware interfering as you suggest. I spent a little more time than I intended to on this issue, so I'm going to leave it here, sorry.

[ioquatix]

I've started a discussion here about how we should handle Rack 2.x middleware: rack/rack#1915

[ioquatix]

This should be fixed in Rack 2.2.4, are you able to test?

[Fjan]

Yes, I can confirm it's fixed in rack 2.2.4, thank you! (Sprockets already rolled back the change to the header capitalisation, but I also tested it with the previous version that introduced lower case headers. There was no slowdown either way)