Consider introducing env['rack.request.headers'] for original headers before transforming them.

[ioquatix]

HTTP/2 is an important protocol and improves the HTTP semantics in a number of key ways, by reducing the complexity of the request meta-data (including path, scheme, authority, etc) as headers.

I'd like to see us migrate away from CGI style HTTP_FOO_BAR headers (from foo-bar:) but this is so ingrained into Rack that it would be almost impossible to change.

My proposal is to start changing tack little by little, the first step would be to introduce env['rack.request.headers'] which are the raw request headers. We can leave it at that, or we can specify it should follow HTTP/2 semantics and include pseudo-headers too (my preference).

In any case, this allows systems which need it, to access the raw underlying headers, while systems that follow the CGI env['HTTP_FOO_BAR'] continue to work as expected.

[jeremyevans]

I don't think this is helpful for performance in HTTP/1, since headers can be mixed case. I agree that it is pretty much impossible to change rack to move away from CGI keys, since the minor performance benefits of doing so are far less than the cost in the loss of backwards compatibility.

Since we have to support CGI keys anyway, supporting original request headers is always going to be strictly slower. If we do choose to include this in SPEC (and I don't think we should), I think it should be optional.

[ioquatix]

Maybe we should consider application level feature flags, like

# config.ru

feature :rack_request_headers

# ... env['rack.request.headers'] is available

[rafaelfranca]

The idea of feature flags are interesting.

How do you see application/libraries adopting this new feature? Or the feature is mostly for rack backward compatibility.

I guess what I'm asking here is: if we didn't care about backwards compatibility and we can change all existing code, how would this new env help us, and how would applications/libraries benefit from it?

[ioquatix]

I discussed this with @tenderlove while I was in Seattle and he was positive on the idea.

I think the advantage of feature flags is it allows us to experiment and make more aggressive changes to Rack without breaking backwards compatibility. Servers could also expose server-specific features and if flags are un-supported it could be indicated clearly at app-load time.

The biggest challenge would be how that looks to middleware, for example, fundamentally changing env would mean some middleware might become incompatible and we'd need a way to propagate that. There was discussion to this end in #1720 but such a change was ultimately rejected.

[ianks]

+1 on this idea. It’s way too painful to parse out the original headers in application code, and to do it correctly today is a dark art

[nateberkopec]

Agreed with @ianks, I often find myself trying to backwards-engineer an original header and it's often less than straightforward!

[ioquatix]

So just to clarify, I think there are two ways forward:

Headers under separate key rack.request.headers.

Existing env headers, like HTTP_ and PATH_INFO etc + rack.request.headers -> original HTTP headers (compatible with Rack but possibly confusing duplicated information).

env = {
  # HTTP/1 request line + rack metadata remains at top level
  "PATH_INFO" => "/foo",
  "RACK_URL_SCHEME" => "https",
  # ... other top level keys

  # Optional HTTP_ keys for compatibility, perhaps some confusion which to add/modify.

  # Raw request headers exposed under here:
  "rack.request.headers" => {
    "accept" => "*/*"
  }
}

Headers blended into env, using pseudo-headers (prefixed with :) for metadata.

Blended env with PATH_INFO but not HTTP_ headers, and all original http headers blended in. HTTP/2 pseudo headers adopted for metadata (slightly less compatible with Rack but more consistent with modern HTTP RFCs).

env = {
  # HTTP/2 pseudo keywords
  ":scheme" => "https",
  ":path" => "/foo"
  
  # Could include RACK_ and HTTP_ headers too (upper case can be considered invalid HTTP header), so they don't bump.

  # Normal (lower case) HTTP headers
  "accept" => "*/*"
}

[jeremyevans]

I'm fine with rack.request.headers, as long as it is optional (requiring it will hurt performance). Switching env format to HTTP 2 style will break most if not all rack servers, middleware, and applications, and the costs of doing so far outweigh the benefits.

[ioquatix]

Switching env format to HTTP 2 style will break most if not all rack servers, middleware, and applications, and the costs of doing so far outweigh the benefits.

Can you clarify this statement because the blended format headers do not intersect with any standard rack headers as defined by the SPEC, the only consideration is headers which contain . (period) characters, like rack.whatever but even these are considered invalid headers by a lot of environments, so in theory don't bump.

If we were willing to wear the long term cost, I would advise us reserving a namespace like :rack.whatever which is considered a pseudo header and restricted to those defined by RFCs. This is the canonical format as outlined in modern HTTP RFCs.

[ioquatix]

If we go down the rack.request.headers route, do we want to introduce some kind of feature flags to enable/disable this functionality? or would it be better to have some kind of middleware which adds it (best effort) if not present?

@nateberkopec @ianks should we aim for this to be a required or optional feature of Rack? How much of a problem is it?

[jeremyevans]

Switching env format to HTTP 2 style will break most if not all rack servers, middleware, and applications, and the costs of doing so far outweigh the benefits.

Can you clarify this statement because the blended format headers do not intersect with any standard rack headers as defined by the SPEC, the only consideration is headers which contain . (period) characters, like rack.whatever but even these are considered invalid headers by a lot of environments, so in theory don't bump.

If the proposal is "Blended env with PATH_INFO but not HTTP_ header", that's obviously going to break almost everything, since almost everything currently depends on HTTP_ prefix for headers.

For "(upper case can be considered invalid HTTP header)", upper case is completely valid for request headers in HTTP 1, so how do you propose to handle those headers? Remember that the vast majority of rack usage is with HTTP 1 and that is not likely to change anytime soon.

For "Could include RACK_", switching the env key prefix for rack-specific keys from rack. to RACK_ as prefix would definitely break things.

If we were willing to wear the long term cost, I would advise us reserving a namespace like :rack.whatever which is considered a pseudo header and restricted to those defined by RFCs. This is the canonical format as outlined in modern HTTP RFCs.

I don't think it makes sense to switch from rack. to :rack. as a env key prefix. Again, huge cost in terms of backwards compatibility, and no significant benefit (at least, I don't consider consistency with HTTP 2/3 RFCs as a significant benefit).

We should only break backwards compatibility if there is a huge benefit in doing so, such as lower case response headers allowing for O(1) response header access and avoiding the need to wrap headers in middleware. Rearranging the deck chairs by switching the names of env keys is going always result in a huge backwards compatibility cost for little if any gain.

[ioquatix]

If the proposal is "Blended env with PATH_INFO but not HTTP_ header", that's obviously going to break almost everything, since almost everything currently depends on HTTP_ prefix for headers.

We can do both styles for compatibility. I'm not suggesting we should do it, just stating that we in theory could do it.

For "(upper case can be considered invalid HTTP header)", upper case is completely valid for request headers in HTTP 1, so how do you propose to handle those headers? Remember that the vast majority of rack usage is with HTTP 1 and that is not likely to change anytime soon.

We should normalise all incoming request headers as lower case.

For "Could include RACK_", switching the env key prefix for rack-specific keys from rack. to RACK_ as prefix would definitely break things.

Yes, for sure, I don't think this is ultimately a good solution, but it's an option to consider.

I don't think it makes sense to switch from rack. to :rack. as a env key prefix. Again, huge cost in terms of backwards compatibility, and no significant benefit (at least, I don't consider consistency with HTTP 2/3 RFCs as a significant benefit).

We can continue to follow CGI style naming conventions but I believe it's more complex than it needs to be and introduces additional complexities like how to handle - and _ character in header names.

Rearranging the deck chairs by switching the names of env keys is going always result in a huge backwards compatibility cost for little if any gain.

I think there are two key gains: request headers and response headers have the same format is easy for users to (1) understand and (2) consume.

I think if we can find a middle ground of rack.request.headers, I'd be okay with that, and I think a solution looks like a middleware that basically implements the above "black magic header conversion" if rack.request.headers doesn't already exist.

I wasn't really planning this for Rack 3.0 but I'd be happy to put together a PR to this effect. I think it keeps everyone happy. But I think we should consider bigger picture if that's the direction we want. Maybe to put it another way, if we were building rack from scratch today, would we (1) opt for CGI style headers, and/or (2) some combination of the approaches proposed above. I'm really interested what that would look like because I think it helps inform the long term path we want to take.

[jeremyevans]

Maybe to put it another way, if we were building rack from scratch today, would we (1) opt for CGI style headers, and/or (2) some combination of the approaches proposed above. I'm really interested what that would look like because I think it helps inform the long term path we want to take.

IMO, that's the wrong way to look at this issue. This would be an appropriate attitude if backwards compatibility was not considered important. I think that backwards compatibility is of paramount importance, and should only be broken in cases where there are major advantages to do so, not just to make things slightly nicer than they are now.

rack.request.headers as an optional feature is fine with me, since it doesn't harm backwards compatibility. If built into servers, servers should probably only enable it if explicitly configured, since it will negatively affect performance (need a separate hash and a lowercase copy of all header keys). I'm fine adding a middleware for it, as well.

[ioquatix]

IMO, that's the wrong way to look at this issue. This would be an appropriate attitude if backwards compatibility was not considered important. I think that backwards compatibility is of paramount importance, and should only be broken in cases where there are major advantages to do so, not just to make things slightly nicer than they are now.

I'm aware of this position on compatibility and totally respect it, but I think it's all about finding a middle ground . Without exploring what the options look like it's hard to know what that is or even whether the compatibility trade offs are worth it.

In other words: "We considered all the options and their various trade offs and decided this was the best approach for the following reasons."

[ioquatix]

I was working on Rack::Request and the Rack::Multipart code today and encountered this exact issue which I found very confusing.

You can see we have constants for some headers, like:

rack/lib/rack/constants.rb

Lines 20 to 21 in cd4b3c9

	CONTENT_LENGTH = 'content-length'
	CONTENT_TYPE = 'content-type'

But these are only valid for response headers, they are not valid env keys (which is at odds with most of the other constants which ARE env keys).

You can see the side effect of this implementation in Rack::Request, e.g.

rack/lib/rack/request.rb

Line 199 in cd4b3c9

def content_length; get_header('CONTENT_LENGTH') end

We have a mismatch between "request environment" which isn't quite headers and response headers.

There are other instances of this problem, e.g.

rack/lib/rack/multipart.rb

Line 26 in cd4b3c9

info = Parser.parse io, content_length, req.get_header('CONTENT_TYPE'), tempfile, bufsize, params

Might I add that CONTENT_TYPE is never a valid HTTP header (at best it would be HTTP_CONTENT_TYPE CGI style). Using get_header to get this environment key makes no sense to me.

I think these examples support a blended approach to headers, although I'll admit it will be a large pain. Maybe something we can work towards for Rack 4?

[jeremyevans]

I was working on Rack::Request and the Rack::Multipart code today and encountered this exact issue which I found very confusing.

You can see we have constants for some headers, like:

rack/lib/rack/constants.rb

Lines 20 to 21 in cd4b3c9

	CONTENT_LENGTH = 'content-length'
	CONTENT_TYPE = 'content-type'

But these are only valid for response headers, they are not valid env keys (which is at odds with most of the other constants which ARE env keys).

Some constants are for some things, other constants are for other things. There are comments stating what each constant is for. Maybe my opinion of you is too high, but I'm surprised you would find this "very confusing". That sounds like an disingenuous exaggeration to push your agenda :).

You can see the side effect of this implementation in Rack::Request, e.g.

rack/lib/rack/request.rb

Line 199 in cd4b3c9

def content_length; get_header('CONTENT_LENGTH') end

We have a mismatch between "request environment" which isn't quite headers and response headers.

I'll grant these are different. There is a necessary difference in that the request environment must contain non-header data. That the header names are mangled differently for request headers and response headers is not a necessary difference, but backwards compatibility is much more important than consistency in this case, IMO.

There are other instances of this problem, e.g.

rack/lib/rack/multipart.rb

Line 26 in cd4b3c9

info = Parser.parse io, content_length, req.get_header('CONTENT_TYPE'), tempfile, bufsize, params

Might I add that CONTENT_TYPE is never a valid HTTP header (at best it would be HTTP_CONTENT_TYPE CGI style). Using get_header to get this environment key makes no sense to me.

The Rack SPEC specifies that CONTENT_TYPE and CONTENT_LENGTH do not have an HTTP_ prefix, for consistency with CGI headers. The SPEC explicitly states that HTTP_CONTENT_TYPE and HTTP_CONTENT_LENGTH should not appear in env.

IMO, Request#get_header is a bad method name, it should really be get_env. Similar for most methods in Request::ENV. Maybe we should add aliases for these?

I think these examples support a blended approach to headers, although I'll admit it will be a large pain. Maybe something we can work towards for Rack 4?

I don't think it's a good tradeoff in Rack 3, and I doubt I'll consider it a good tradeoff in Rack 4. Optional rack.request.headers is as far as I think we should go at this point.

[ioquatix]

Some constants are for some things, other constants are for other things. There are comments stating what each constant is for. Maybe my opinion of you is too high, but I'm surprised you would find this "very confusing". That sounds like an disingenuous exaggeration to push your agenda :).

I think it's confusing to have constants in the same module which look the same, but are used in very different contexts. Even the best engineer with a poor interface can make mistakes. I think the best we can hope to do is take these experiences and use them to shape the developer UX of Rack to be the best it can be.

Anyway just make it clear if it isn't already my agenda is to:

• Make things more consistent,
• Make things easier to understand,
• Modernize interfaces, and
• Follow standards (usually modern ones).

IMO, Request#get_header is a bad method name, it should really be get_env. Similar for most methods in Request::ENV. Maybe we should add aliases for these?

CONTENT_TYPE isn't an HTTP header, so writing request.get_header('CONTENT_TYPE') seems totally wrong to me. Adding an alias plasters of the core of the problem.

Why should these not work?

request = Rack::Request.new(env)
request.get_header('authorization')
request.set_header('authorization', 'token')

response = Rack::Response.new
response.set_header('authorization', 'token')
response.get_header('authorization')

To me, a header has a very specific definition as per the RFCs we follow.

If env has a different internal format, get_header and set_header should adapt this. Otherwise, as you state, this should really just be get_env and set_env. The reality is, getting and setting headers has much more specific semantics. It seems we are in a total mess, semantically speaking.

[jeremyevans]

Some constants are for some things, other constants are for other things. There are comments stating what each constant is for. Maybe my opinion of you is too high, but I'm surprised you would find this "very confusing". That sounds like an disingenuous exaggeration to push your agenda :).

I think it's confusing to have constants in the same module which look the same, but are used in very different contexts. Even the best engineer with a poor interface can make mistakes. I think the best we can hope to do is take these experiences and use them to shape the developer UX of Rack to be the best it can be.

I'm OK with using these as examples of what not to do in the future. :)

Anyway just make it clear if it isn't already my agenda is to:

• Make things more consistent,
• Make things easier to understand,
• Modernize interfaces, and
• Follow standards (usually modern ones).

These are noble goals. However, it's frustrating to me that backwards compatibility concerns are nowhere on your agenda, as they are on the forefront of my agenda.

IMO, Request#get_header is a bad method name, it should really be get_env. Similar for most methods in Request::ENV. Maybe we should add aliases for these?

CONTENT_TYPE isn't an HTTP header, so writing request.get_header('CONTENT_TYPE') seems totally wrong to me. Adding an alias plasters of the core of the problem.

I'm not sure why you would be opposed to adding more accurate names if your agenda is to make things easier to understand. It doesn't fix the problem of the old method name being inaccurate, but it at least allows people to move forward with better names. But I won't push to add aliases if you are against them.

Why should these not work?

request = Rack::Request.new(env)
request.get_header('authorization')
request.set_header('authorization', 'token')

response = Rack::Response.new
response.set_header('authorization', 'token')
response.get_header('authorization')

Absent backwards compatibility concerns, I agree with you that they should. I just don't think changing *_header behavior is worth breaking backwards compatibility.

[ioquatix]

I'm not sure why you would be opposed to adding more accurate names if your agenda is to make things easier to understand. It doesn't fix the problem of the old method name being inaccurate, but it at least allows people to move forward with better names. But I won't push to add aliases if you are against them.

I'm fine with adding aliases but I'm not sure it makes sense when someone can just write request.env[x]?

[ianks]

@nateberkopec @ianks should we aim for this to be a required or optional feature of Rack? How much of a problem is it?

Another way to word this would be: do we want to see adoption for rack.request.headers? What do we want Rack to look like in 10 years? Making it an optional feature would severely inhibit adoption of the feature.

With that being said, as annoying as the CGI style headers are, if the performance hit is significant it may not be worth requiring the feature.

I'm fine with rack.request.headers, as long as it is optional (requiring it will hurt performance).

Does anyone have any benchmarks for this? Would be good to have some numbers for typical apps, then include some of apps that have a couple nasty 4K-8K size header values.

[ioquatix]

The cost for falcon will be close to zero.

The cost for puma might be a little bit:

• Strings for header keys... a few allocations?
• Hash table, one allocation.

Overhead would be minor. Probably in the grand scheme of things irrelevant.

[jeremyevans]

@nateberkopec @ianks should we aim for this to be a required or optional feature of Rack? How much of a problem is it?

Another way to word this would be: do we want to see adoption for rack.request.headers? What do we want Rack to look like in 10 years? Making it an optional feature would severely inhibit adoption of the feature.

I want Rack to look pretty much the same as it looks currently. It works well and does not need major backwards incompatible changes. Hopefully the lower case response header keys will be the last major backwards incompatible change we make.

With that being said, as annoying as the CGI style headers are, if the performance hit is significant it may not be worth requiring the feature.

I'm fine with rack.request.headers, as long as it is optional (requiring it will hurt performance).

Does anyone have any benchmarks for this? Would be good to have some numbers for typical apps, then include some of apps that have a couple nasty 4K-8K size header values.

The issue here is not necessarily in the size of headers, but in the number of headers. This will probably increase the number of allocations required by around 50%, and time spent populating the environment by 80%. How much of a difference that makes depends on the server, request, and application.

The cost for falcon will be close to zero.

Even for HTTP 1, where you have to create a lowercase version of the request header keys, since they aren't required to be in lower case? Seems odd that you would be already creating a lowercase version of the request header keys now, since there would be no use for it.

Overhead would be minor. Probably in the grand scheme of things irrelevant.

Depends on the request I would think. Obviously if you are talking about a request that takes 100ms to produce a response, any changes to the environment are going to be minor in comparison. I'm guessing requiring rack.request.headers would be measurable for fast requests. So benchmarks with a rack app such as lambda{|_| [200, {}, []]} for puma and unicorn would be good to see.

One thing that I don't think has been considered yet is that request headers are mutable. Currently, if you change a header in the environment, other code that looks for the header always get the updated value, since the header value is stored in a single place. Once you store the header value in two places, it becomes impractical to guarantee that you'll see the updated value, considering that a lot of current code modifies the environment directly and not through a method. This is an acceptable risk for a feature you need to opt into. I'm not sure it's acceptable to turn on by default, since it exposes all applications to issues where some code updates the header value in only env, and other code reads the header value from rack.request.headers (or vice-versa).

[ianks]

One thing that I don't think has been considered yet is that request headers are mutable. Currently, if you change a header in the environment, other code that looks for the header always get the updated value, since the header value is stored in a single place. Once you store the header value in two places, it becomes impractical to guarantee that you'll see the updated value, considering that a lot of current code modifies the environment directly and not through a method. This is an acceptable risk for a feature you need to opt into. I'm not sure it's acceptable to turn on by default, since it exposes all applications to issues where some code updates the header value in only env, and other code reads the header value from rack.request.headers (or vice-versa).

For rack.request.headers, I strongly believe the header values should be immutable. The hash itself should also be immutable, since the intent is provide the original headers that were sent.

Even for the current CGI-style headers, I think immutability of the header values would be a good thing. I'd be curious to know how badly things today would break if we froze the values...

[ioquatix]

I think it's reasonable the keys and values are immutable. But env itself can't be, and I guess the next question would env['rack.request.headers'] be immutable too (the hash table itself)? Realistically, it's probably not necessary and there should be no real advantage since the server will have to generate it once per request. Being able to replace or remove values from the headers... well whether you want to or not might be a design question, but I believe we shouldn't prevent it, at least I can't see any reason why it should be prevented.

[jeremyevans]

Header keys should already be immutable, since ruby will use a frozen copy of a string as a hash key, if the key is given as an unfrozen string.

I'm not sure whether making header values immutable is worth the backwards compatibility impact, but I think it's something we can consider.

I agree with @ioquatix that env can't be immutable, and there is no significant advantage to env['rack.request.headers'] being immutable (should it be introduced).

I'm not sure there is a backwards compatible way of introducing env['rack.request.headers'] and intelligently handling the mutability of headers, and I don't think we should consider introducing env['rack.request.headers'] until a such a way is found.

[ioquatix]

I'm not sure there is a backwards compatible way of introducing env['rack.request.headers'] and intelligently handling the mutability of headers, and I don't think we should consider introducing env['rack.request.headers'] until a such a way is found.

To clarify, it seems like the problem you are concerned about is the duplicated headers if we have both env['HTTP_FOO'] and env['rack.request.headers']['foo']. If middleware is looking up the value of the header foo, which one should it consult?

It's true this is an important issue, but I think the answer is pretty easy, at least in the short term, users should be consulting the env directly for compatibility.

The question then becomes what is the point of rack.request.headers and the answer is probably a combination of the above pain points (e.g. proxying requests, - to _ mapping, etc) and what we would like rack to look like in the future.

This is why I suggested maybe a better way is to entirely replace the HTTP_ CGI headers with something closer to the actual headers received by the server itself. Of course, replacement isn't very compatible so the way forward would be:

• Feature flags (i.e. Rack::Builder#options or something similar).
• Middleware which does the compatibility mapping, e.g. Rack::Compatible which handles the mapping either forwards or backwards.

Because any of these options gives us a clean slate, maybe it's best we consider what that should look like, rather than trying to find "compatible" but ultimately not practical solutions.

[jeremyevans]

I'm not sure there is a backwards compatible way of introducing env['rack.request.headers'] and intelligently handling the mutability of headers, and I don't think we should consider introducing env['rack.request.headers'] until a such a way is found.

To clarify, it seems like the problem you are concerned about is the duplicated headers if we have both env['HTTP_FOO'] and env['rack.request.headers']['foo']. If middleware is looking up the value of the header foo, which one should it consult?

That's pretty much it. If you considered headers immutable, this wouldn't be an issue, but since the headers are mutable, this is an issue.

It's true this is an important issue, but I think the answer is pretty easy, at least in the short term, users should be consulting the env directly for compatibility.

OK. That keeps compatibility now. Of course, if you do eventually switch to checking rack.request.headers, you break backwards compatibility at that point, so from a certain point of view, this just kicks the can down the road.

The question then becomes what is the point of rack.request.headers and the answer is probably a combination of the above pain points (e.g. proxying requests, - to _ mapping, etc) and what we would like rack to look like in the future.

How common is it for rack applications to proxy requests to other HTTP servers (as opposed to calling another rack application with the same environment)? My gut feeling is that's a rare case that we don't need to optimize for.

It's true that the - to _ mapping adds a little overhead. Considering that at least for HTTP 1 requests (the vast majority of requests handled by rack servers), you still need to mangle for rack.request.headers to lowercase the keys. I don't think a String#tr! call per key adds much overhead, but there is undoubtedly some.

This is why I suggested maybe a better way is to entirely replace the HTTP_ CGI headers with something closer to the actual headers received by the server itself. Of course, replacement isn't very compatible so the way forward would be:

• Feature flags (i.e. Rack::Builder#options or something similar).
• Middleware which does the compatibility mapping, e.g. Rack::Compatible which handles the mapping either forwards or backwards.

Because any of these options gives us a clean slate, maybe it's best we consider what that should look like, rather than trying to find "compatible" but ultimately not practical solutions.

IMO, the cost of breaking backwards compatibility in this area is not going to be worth the benefits. Ask yourself, who really benefits from changing from CGI env to a separate headers hash? It's not existing users/applications.

If a user/application is already doing the mangling themselves, then switching to a different mangling/storage is breaking things without a benefit.

If a user/application is not already doing the mangling themselves, then they are probably using a web framework which already handles the mangling for them. Changing to a different mangling/storage is then going to be no different to the user/application, and just complicate the related web framework.

I suppose you could argue that while this doesn't make things easier for existing users/applications, it makes things easier for new users/applications. I'll concede that, though I don't think there is a major difference. However, Ruby/Rack is a mature ecosystem, and we should prioritize backwards compatibility over making things easier for new users, especially in cases like this where the benefits of the change are minor.

[ioquatix]

I'm now almost completely convinced that we shouldn't introduce rack.request.headers. However, I think we should explore whether a more holistic change to env makes sense. As such I'll close this issue and open a new one.

[ioquatix]

@nateberkopec @ianks I introduced/fixed Rack::Request#each_header to give you, as closely as we can, the original source headers, in #1880 - I believe this is a good first step to addressing your concerns.