Rack: Rack::Chunked middleware conflicts with Rack::Sendfile

[boazsegev]

The Rack::Chunked middleware conflict with the Rack::Sendfile middleware / approach.

The Rack::Chunked doesn't test if the 'X-Sendfile' header family is set, adding the chunked transfer encoding header.

This results in two possible approaches:

1. The server ignores any headers set by the application (losing any cookie data or other custom headers).

2. The encoding header remains intact*, causing the response to fail.

Both approaches result in loss of data (either the cookie data or a failed response).

The best way to fix the issue might be to add tests to the Rack::Chunked middleware, testing for the 'X-Sendfile' header family.

* Consider that the header might have been Transfer-Encoding: gzip and the file X-sendfile: style.css.gz, which means that the header should have remained intact.

This issue originated here.

[ioquatix]

I've actually been thinking about this, and I believe we should remove Rack::Chunked because it's something that should be handled by the web server, not the application.

[boazsegev]

@ioquatix, I agree.

Transport protocol details should probably be considered a server concern.

[acmh]

Is this issue up for grabs?

[ioquatix]

@acmh I guess yes - what do you think should be done? My suggested plan is to remove it in Rack 3.0.

[boazsegev]

@acmh - I already mentioned this, but I second the approach offered by @ioquatix. There are a number of reasons why deprecating the Rack::Chunked middleware would be a good idea, and this issue is just one of them. For example:

• Message formatting is a Server concern (Protocol details) and not an application concern. Message formatting might (and did/will) change with HTTP/2 and HTTP/3.

• By returning this concern to the server layer, servers will be able to automatically use chunked encoding when appropriate (i.e., no content-length header and a response stream that routes to #each ( not a simple Array with a single String member). This could be performed according to the correct protocol specifications and according to the correct protocol version.

• By avoiding a chunked response, testing might be simplified and the response (payload) could be tested more easily, bypassing the need for a client emulation layer. This will also improve performance.

[acmh]

@ioquatix and @boazsegev First, thanks for your time to respond to this message. I want to solve this issue, but I need some guidance since I am not familiar with the codebase.

I want to contribute, but I will understand if this is a complex first issue to solve.

Can you give me an entry point to start looking?

[acmh]

I read the code to understand better the problem. So, if I understood right the @ioquatix suggestions, I will open a PR to:

1 - Remove the loading of Rack::Chunked middleware
2 - Remove the Rack::Chunked file
3 - Remove the proper specs
4 - Updates the changelog

One question: What happens to users of Rack::Chunked middleware? Do we need first warn them that the middleware will be deprecated before removing it?

[boazsegev]

@acmh ,

What happens to users of Rack::Chunked middleware? Do we need first warn them that the middleware will be deprecated before removing it?

This is something someone from the Rack team should address (maybe @tenderlove or @macournoyer ... ?).

IMHO, there should be a deprecation notice (and perhaps a warning) for Rack 2.x versions and version 3.x should be "clean" (no Rack::Chunked middleware).

People expect major version updates to break some things. However, minor and patch version updates normally shouldn't affect the existing API or its behavior.

IMHO, your TODO list could be clarified with this consideration in mind. For example:

1 - Remove the loading of Rack::Chunked middleware
2 - Remove the Rack::Chunked file

Maybe a first stage deprecation notice and second stage removal?

3 - Remove the proper specs
4 - Updates the changelog

Maybe keep the specs and mark them as deprecated as of version X?

[ioquatix]

@tenderlove I think the solution is to deprecate and remove the Rack::Chunked middleware. Are you happy for me to add a deprecation warning in 2.1.0?

[erwanlr]

I second the removal of the Rack::Chunked middleware

[ioquatix]

I implemented the first part of this: I removed Rack::Chunked from Rack::Server and released it in 2.1.1 today.

It turns out that the chunked middleware was using HTTP_VERSION in 2.0.x which is badly supported... it was fixed everywhere to use SERVER_PROTOCOL, which triggered Rack::Chunked to start "working", and by working, I mean causing chaos by turning response bodies into chunked messes. This broke falcon, and probably other servers too that were not expecting it.

The next step is to figure out if anyone is actually using Rack::Chunked, add deprecation notices, don't load it by default, etc. It can be done in the 2.2 release.

[ioquatix]

@boazsegev can you confirm that the latest 2.1.1 release no longer causes this issue and if so close it? Thanks!

[boazsegev]

I confirm that version 2.1.1 doesn't have this issue unless Rack:Chunked is manually used.

Thanks, @ioquatix 👏🏻👏🏻👍🏻

I'm mostly offline for the next two/three weeks. If there's anything more I may do, let me know. It may take me some more time, but I'll get there.

B.