celluloid / reel

UNMAINTAINED: See celluloid/celluloid#779 - Celluloid::IO-powered web server
https://celluloid.io
MIT License
596 stars 87 forks source link

case-insensitivity for header field names #152

Closed kenichi closed 10 years ago

kenichi commented 10 years ago

while testing angelo on heroku, i discovered that heroku's router is downcasing request header keys. this is valid according to:

http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2

this change should make all access to request header keys insensitive, though probably not in a very performant way.

there's most likely a way better way of handling this... but wanted to get this on the radar. this app actually works on heroku with the beta websocket support enabled. note the Gemfile.

ungoldman commented 10 years ago

:+1:

tarcieri commented 10 years ago

Note that Reel did something like this originally (actually it put headers in the canonical form instead of downcasing) but we removed it for performance reasons. See e.g. 9e0aeb2544

I know Mongrel did something like this, and perhaps that's where Heroku got the idea from, but doing this eagerly is somewhat expensive, especially for headers that are never accessed.

kenichi commented 10 years ago

i definitely understand the performance reasons, and i don't like the way rack upcases and prepends 'HTTP_' to everything; but, for situations like this where header keys are not actually required to be in a certain case, it doesn't work unless the case-insensitivity is there. i also don't like that heroku is doing that to the requests, but alas.

note that this does not eagerly change header keys; it should only run the block when a key is not found. so i think it's basically the difference between one more Hash being allocated by the merge, if the key is never accessed. in siege tests, it seems to drop about 150 transactions / sec.

tarcieri commented 10 years ago

Okay, my bad, I clearly didn't read the code. This is a neat trick, and seems ok to me, thanks!