Skip to content

Set Content-Length response header even for bodies not responding to to_ary#1509

Merged
jeremyevans merged 1 commit intorack:masterfrom
jeremyevans:content_length-no-to_ary
Jan 22, 2020
Merged

Set Content-Length response header even for bodies not responding to to_ary#1509
jeremyevans merged 1 commit intorack:masterfrom
jeremyevans:content_length-no-to_ary

Conversation

@jeremyevans
Copy link
Copy Markdown
Contributor

This to_ary check was previously there to work around an infinite
loop issue due to Rack::Response#to_ary being defined.
Rack::Response#to_ary has been removed, so this check can be removed,
and the middleware can correctly calculate Content-Length for all
valid rack responses.

Fixes #734

…to_ary

This to_ary check was previously there to work around an infinite
loop issue due to Rack::Response#to_ary being defined.
Rack::Response#to_ary has been removed, so this check can be removed,
and the middleware can correctly calculate Content-Length for all
valid rack responses.

Fixes rack#734
@wjordan
Copy link
Copy Markdown
Contributor

wjordan commented Apr 26, 2021

The to_ary check in ContentLength middleware removed by this PR was not previously there to work around the infinite-loop issue related to Rack::Response (#419, which was ultimately fixed by the API change in #1453).

That infinite-loop issue was only indirectly related to ContentLength because the initial workaround for that issue (5b251a9) caused any object wrapped by BodyProxy to no longer respond to #to_ary, which in turn caused ContentLength middleware to incorrectly skip any body responses wrapped by BodyProxy.

In fact, the to_ary check was originally added (7a0476b) in order for ContentLength to only eagerly-load Rack bodies that were Arrays (or objects that could be coerced into Arrays), and pass through any other potentially-streaming bodies without blocking.

See #166 and 9495207 for further related discussion.

This decade-old discussion on how best to handle streaming response bodies in the Rack interface while also supporting full-response-reading middleware such as ContentLength is still ongoing (e.g., #1745).

@jeremyevans
Copy link
Copy Markdown
Contributor Author

@wjordan Thanks for the providing the additional context. I guess I was misled by #734 which said the code was added to address #419 (the infinite loop issue).

I'm not sure if we want to revert the behavior. We'd have to come up with another way to address #734 if we did revert it.

If we decide to treat some bodies differently than other bodies, than we may want to revisit this. However, we should probably make a consistent decision that is applied across all middleware that ship with Rack. Just checking for to_ary doesn't seem to be a good choice for that. Personally, I think the current behavior is fine, where users can set Transfer-Encoding if they don't want want the Content-Length calculated. However, I don't use the middleware personally and I don't have strong feelings for how it should work.

@ioquatix
Copy link
Copy Markdown
Member

No one should be setting the transfer-encoding header since it's invalid for HTTP/2.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rack::BodyProxy with Rack::ContentLength middleware

3 participants