Support for chunked encoding? (HTTP/1.1 streaming)

Question

Support for chunked encoding? (HTTP/1.1 streaming)

jarthod opened this issue 4 years ago · 12 comments

Hi,

I noticed that some of my capybara tests were failing when changing some parts of my view in my streamed pages (using render stream: true in Rails). Digging up a bit this seems to be because rack-test simply does not understand the Transfer-Encoding: chunked, and so the resulting html is broken, e.g.:

d
</head><body>
2b
<header><div class="center"><div id="logo">
38
<a class="can-truncate" href=

So depending on where the server decides to split the packets, it'll make the test pass or fail because sometimes the html is roughly valid, and sometimes not.

For the moment I temporarily disabled streaming for RackTest requests (while keeping it on for other http clients like Chrome or Typhoeus clients) using this kind of patch:

class MyController < ApplicationController
  def index
    # instead of render stream: true
    render stream: allow_streaming
  end

  # Disable chunked encoding for RackTest (no User-Agent) as it does not support it
  # Only enable it for Typhoeus and Chrome testing
  def allow_streaming
    !Rails.env.test? or request.headers['HTTP_USER_AGENT'] != nil
  end
end

But my question is: would it make sense for RackTest to understand "chunked" encoding or is it not it's responsibility? For the sake of simplicity and so the tests does what we expect them to, I would say yes it does (and I can try writing a PR for that). But I wanted to check first if it's something that is voluntarily left out for some reason? or if there's any previous discussion about this (couldn't find any)?

For the record, the chunked encoding implementation Rails uses is standard rack.
And in order to decode this encoding from RackTest, we would need to add a bit of code similar to this (only doing so when the Transfer-Encoding is set to chunked of course).

What do you think?
Thanks!

Answer 1 · 2022-05-02T20:17:47.000Z

In general, it's probably a bad idea for applications to implement Transfer-Encoding: chunked directly. That's probably best left to the web server. Puma will automatically use Transfer-Encoding: chunked for HTTP/1.1 requests without Content-Length.

Instead of building a Transfer-Encoding chunked parser for rack-test, I think the better solution is to set env entries indicating HTTP/1.0 requests. No application should be returning a Transfer-Encoding: chunked response to an HTTP/1.0 request. I implemented that in #288.

Answer 2 · 2022-05-02T21:44:47.000Z

#288 was merged at a68ea95

Answer 3 · 2022-05-03T08:26:57.000Z

@jeremyevans thanks for your response and solution, I just tested the master branch in my project (after disabling my HTTP_USER_AGENT temporary workaround) and I can confirm that it has the expected effect: rails streaming is disabled and so rack-test manages to parse the body normally 👍

In general, it's probably a bad idea for applications to implement Transfer-Encoding: chunked directly. That's probably best left to the web server

I agree implementing it manually would be dangerous but using the standard framework streaming feature why would that be bad? This is the standard way to do HTTP streaming AFAIK, that's why it's implemented natively in Rack and Rails and people can use it. Do you think this feature should be removed?

I must say I agree with @ioquatix (in #288) that the testing layer should ideally test the real body format (which 99.99% of users will get) because otherwise we don't really know that it is still working (I can only test with cuprite at the moment). So if at any point streaming (or any other feature using chunked encoding) gets broken, it might slip into production unnoticed depending on how carefully the test were crafted.

Answer 4 · 2022-05-03T10:36:50.000Z

rack-test is really light weight in comparison to say integration testing with a real server, and for that I'd probably just recommend falcon/async-http if you want to fully opt into streaming/websockets/etc.

Answer 5 · 2022-05-03T11:57:29.000Z

Agreed, I am not "fully opt in" though, I'm just using streaming on 2 endpoint to make the UI more responsive (using render-later). And I can properly test these endpoints using integration test so that is fine and not worth a new tech in my situation. I don't really expect to test streaming with rack-test, I just would prefer it not to randomly fail on my endpoints which supports streaming, for example when I test them in controller test for permissions or using rack-test for other reasons like just testing html rendered.

This will be the case after @jeremyevans fix is released so that's good enough for me honeslty because I know what to expect. I'm just saying ideally it would be even better if it could support this because HTTP/1.0 is clearly not the future and people who start using streaming but don't know about this might think their rack-test tests are properly covering for them (because well test is green and body is correct). So it might lead to some bad surprises down the road (very few admittetly), if for example streaming/chunked encoding gets broken for some reason (by Rails or the project itself) but all tests are green because they are disabling it.

Just my opinion of course, I totally understand your position. And if it ever changes just let me know, my offer to contribute this feature will probably still stand :)

Answer 6 · 2022-05-03T14:58:01.000Z

I think the best way to address this would be to build a rack middleware that decodes Transfer-Encoding chunked responses and returns them as plain responses, and use that when testing. I think that is out of scope for rack-test, but I suppose I would be willing to accept a fully line/branch covered implementation, so long as it was opt-in and not used by default.

@jarthod You point out that HTTP/1.0 is clearly not the future. By that logic, HTTP/1.1 is also clearly not the future, and Transfer-Encoding: chunked is not supported in HTTP/2.

Answer 7 · 2022-05-03T15:25:36.000Z

There's already some middleware doing this so shouldn't be too hard but are you thinking about a middleware which needs to be added server-side (in test.rb basically) or can it be added to the rack-test stack dynamically? I'm not sure if this is possible/good practice?

@jarthod You point out that HTTP/1.0 is clearly not the future. By that logic, HTTP/1.1 is also clearly not the future, and Transfer-Encoding: chunked is not supported in HTTP/2.

Good point ^^ though HTTP/2 is not gonna be implemented into rack AFAIK? Some features are added but not the native protocol I mean. So it looks like we'll keep HTTP/1.1 semantics there for a while (with an HTTP/2 compatible server or reverse proxy in front).

Answer 8 · 2022-05-03T15:48:22.000Z

There's already some middleware doing this so shouldn't be too hard but are you thinking about a middleware which needs to be added server-side (in test.rb basically) or can it be added to the rack-test stack dynamically? I'm not sure if this is possible/good practice?

It's always possible to wrap any rack app in middleware. The resulting object is a new rack app.

@jarthod You point out that HTTP/1.0 is clearly not the future. By that logic, HTTP/1.1 is also clearly not the future, and Transfer-Encoding: chunked is not supported in HTTP/2.

Good point ^^ though HTTP/2 is not gonna be implemented into rack AFAIK? Some features are added but not the native protocol I mean. So it looks like we'll keep HTTP/1.1 semantics there for a while (with an HTTP/2 compatible server or reverse proxy in front).

I'm not sure what you mean. Rack does not deal with any version of the native HTTP protocol, that's the job of the web server (by design). Rack works fine with HTTP/2, its design is independent of HTTP version. You can use HTTP/2 for your rack applications using Falcon and probably other ruby webservers that support HTTP/2. There are HTTP/2 features that you cannot really use with Rack, due to Rack's design, but supporting them would probably break backwards compatibility.

Answer 9 · 2022-05-03T16:18:53.000Z

It's always possible to wrap any rack app in middleware. The resulting object is a new rack app

Yes but what I mean is: when you talk about "opt-in", is there any way in rack-test to add an intermediary middleware? or were you simply thinking about manually overriding the def app or Capybara.app to wrap the application inside this middleware. Because if it's the later there's not really any change to PR into rack-test is it? people just need to do this on their end?

I'm not sure what you mean. Rack does not deal with any version of the native HTTP protocol, that's the job of the web server (by design). Rack works fine with HTTP/2, its design is independent of HTTP version. You can use HTTP/2 for your rack applications using Falcon and probably other ruby webservers that support HTTP/2. There are HTTP/2 features that you cannot really use with Rack, due to Rack's design, but supporting them would probably break backwards compatibility.

Sorry I wasn't very clear (I'm not an expert in this either ^^), what I meant is that a lot of HTTP/1.1 behaviors is still "hardcoded" in rack (and rails) which means that even if there's a more generic way to implement it it's not necessarily possible to use yet. The streaming feature I am talking about is a good example, I suppose an HTTP/2 server can use the iterable body object to stream a response (or send it using chunked encoding in HTTP/1.1), providing no other middleware breaks this iterator of course, yet Rails still hardcodes the HTTP/1.1 "chunked" encoding for this use-case and uses the standard Rack middleware for this too (I just noticed this is deprecated and will be removed). I agree with you in this location of the app it should be protocol agnostic and it's the server which should decide in what way it's gonna stream, but unfortunately that's not the case yet. If rack fully supports this feature maybe we need to change Rails implementation then? (sorry for the long message maybe it's not the place to discuss this).

Answer 10 · 2022-05-03T16:34:04.000Z

It's always possible to wrap any rack app in middleware. The resulting object is a new rack app

Yes but what I mean is: when you talk about "opt-in", is there any way in rack-test to add an intermediary middleware? or were you simply thinking about manually overriding the def app or Capybara.app to wrap the application inside this middleware. Because if it's the later there's not really any change to PR into rack-test is it? people just need to do this on their end?

Correct, users would do this on their end. The change to PR into rack-test would be to ship the middleware with rack-test, so people wouldn't need to install it as a separate gem. They would still need to use it manually. As I mentioned, I think it is a bit out-of-scope for rack test.

I'm not sure what you mean. Rack does not deal with any version of the native HTTP protocol, that's the job of the web server (by design). Rack works fine with HTTP/2, its design is independent of HTTP version. You can use HTTP/2 for your rack applications using Falcon and probably other ruby webservers that support HTTP/2. There are HTTP/2 features that you cannot really use with Rack, due to Rack's design, but supporting them would probably break backwards compatibility.

Sorry I wasn't very clear (I'm not an expert in this either ^^), what I meant is that a lot of HTTP/1.1 behaviors is still "hardcoded" in rack (and rails) which means that even if there's a more generic way to implement it it's not necessarily possible to use yet. The streaming feature I am talking about is a good example, I suppose an HTTP/2 server can use the iterable body object to stream a response (or send it using chunked encoding in HTTP/1.1), providing no other middleware breaks this iterator of course, yet Rails still hardcodes the HTTP/1.1 "chunked" encoding for this use-case and uses the standard Rack middleware for this too (I just noticed this is deprecated and will be removed). I agree with you in this location of the app it should be protocol agnostic and it's the server which should decide in what way it's gonna stream, but unfortunately that's not the case yet. If rack fully supports this feature maybe we need to change Rails implementation then? (sorry for the long message maybe it's not the place to discuss this).

Rack can stream an iterable body without chunked encoding regardless of HTTP version. It's supported in HTTP/1.0. The advantage of using chunked encoding in HTTP/1.1 is you can reuse the connection for additional requests (pipelining).

As you saw, we deprecated Rack::Chunked because we determined it's not really something Rack should be doing.

I agree that Rails should change the implementation to not use Transfer-Encoding: chunked by default. I made a similar change in Roda a while back: jeremyevans/roda@302f7fe

Food for thought: let's say the application started returning HTTP/2 frames as the body, instead of a chunked encoding stream. Would you still expect rack-test to try to decode that?

Answer 11 · 2022-05-03T21:23:43.000Z

Just to add a few more thoughts:

Rack::Test should be about testing Rack as an interface not applications running on a servers using actual sockets/wire protocols. This means I'd personally expect the response body to be a plain body conforming to the Rack spec, not some encoded form, and the deprecation and removal of Rack::Chunked should give strong guidance as to what we expect going forward.

If you want to test actual interactions, it's probably more the realm of Capybara/integration testing with an actual server. If you want to test HTTP/2, https://github.com/socketry/falcon-capybara is one option for this.

Sorry I wasn't very clear (I'm not an expert in this either ^^), what I meant is that a lot of HTTP/1.1 behaviors is still "hardcoded" in rack

Rack as it exists mostly implements the CGI RFC, and isn't even fully compatible with the full semantics of HTTP.

https://github.com/rails/rails/blob/main/actionpack/lib/action_controller/metal/streaming.rb

We should definitely fix this.

Answer 12 · 2022-05-03T22:33:48.000Z

Thanks for your responses @jeremyevans and @ioquatix. I understand the direction better and I believe the best course of action here would be to improve the streaming implementation in Rails as you confirmed, to not use chunked encoding by default. I looked for existing PR/issues about this but couldn't find any so maybe that's the PR I should write.