-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Version 2.2.x breaks Rails Actioncontroller:Live streaming #1619
Comments
Sorry for the delay in responding. I think Rails was premature to close this issue. We have not confirmed it is a bug in Rack (it may be, but we don't know yet). It could be due to Rails relying on Rack implementation details that changed between Rack 2.1 and Rack 2.2. Can you please do at least one of the following (preferably both):
|
I am happy to investigate this too, but ideally need an isolated example as outlined above. Otherwise, can you make a rails app (in a github repo) which fails, with instructions on what to do to make it fail. |
This is likely the commit that broke it. 290523f Looking the reported backtrace it is very clear that this is broken in rack not Rails. It is failing when calling
Line 361 in 39d501a
Before that commit the code correspondent to |
A fix would change |
Yeah, it's my fault, I'll fix it. Sorry everyone! |
The issue @rafaelfranca mentions is fixed already in v2.2.2 So there is some other reason why live streaming is not working. Not sure what it is yet, investigating. |
It looks like |
This
|
What should we do? Revert? |
Here is the discussion: #1416 I've often thought
|
Rather than using |
Actually it looks like we can add custom tokens to cache-control: https://tools.ietf.org/html/rfc7234#section-5.2.3 |
I'm out of my depth here, but humour me. The calculation of the I think this is at the core of the issue, but I acknowledge that I am not "in my lane". FWIW. |
In any case, the issue is that |
Yep. The body has to respond to
I could have become confused. The data in the response is complex. But I did
That's what I went with. |
@jefflasslett excellent work debugging the issue and finding a solution. I think a better solution is needed. Maybe this is another point in favour of I think I will implement support for |
Thanks @ioquatix . FWIW, I was content to bypass |
That makes sense. Can you show the code you used to bypass Then, maybe a 3rd option is to sniff the |
Here's a piece of the relevant controller. You can set either or both of Anyway, the controller fragment ...
Probably should set |
Thanks for the update! To me:
Looks like a hack. It's okay, but not great. Because |
:-) |
In rack/rack#1416 the Rack::ETag middleware changed it's behaviour to buffer the response and calculate an ETag digest even though the Cache-Control: no-cache header is set. This breaks the CSV streaming responses so to avoid that we use the hack outlined in rack/rack#1619 and set a Last-Modified header which triggers the skip_caching? condition in the Rack::ETag middleware.
In rack/rack#1416 the Rack::ETag middleware changed it's behaviour to buffer the response and calculate an ETag digest even though the Cache-Control: no-cache header is set. This breaks the CSV streaming responses so to avoid that we use the hack outlined in rack/rack#1619 and set a Last-Modified header which triggers the skip_caching? condition in the Rack::ETag middleware.
We have decided not to backport this, so the fix in Rack 2.x is either:
|
Can someone verify that this issue was fixed in Rack 3.x? I tried to spy a relevant entry in the changelog, but couldn't find any. Thanks in advance! |
It should be. |
Not fixed for me using Rails 7.0.4 + Ruby 3.2.0. Had to disable etag middleware. |
Considering that Rails 7.0.4 requires Rack 2, that's expected. You'll need to wait until Rails supports Rack 3. |
My bad. Sorry! |
For me it wasn't |
About this problem which I also ran into, for the record the fix has already been released in puma |
Yes, the streaming issue (puma/puma#3000) was addressed in Puma 6.0.1 along with other issues (puma/puma#2999). |
I'm developing on a new Rails 7 project today and found the stream response is not work. I'm following this doc (https://api.rubyonrails.org/v7.0.4/classes/ActionController/Live/SSE.html) to learn SSE. It took me hours to find out the issue is about rack... Fortunately I finally found the hack solution here, yes I checked my rack version is |
To confirm for future people trying to get |
I guess the answer is, streaming never really worked correctly with Rack 2.x unless very specific conditions were met, or you used the |
That's a bit hyperbolic. Streaming worked fine in Rack 2 if you were not buffering the body (but Rack::ETag buffered the body by default). In Rack 2, you can either remove Rack::ETag from the middleware stack, or set the |
Well, given that Rails includes |
Oh wow! I added |
totally removing the Etag middleware could bring some performance implications. You should do that only for specific controllers/paths |
It depends on how your statement is interpreted. I interpreted |
Note that with a standard Rails layout the I agree that including Further reading: |
the issue can be time-consuming for Rails users to diagnose but the workaround is simple, I'm not sure I should write here but for future people trying to get
Here's an example of how you can set the def stream
response.headers["Content-Type"] = "text/event-stream"
response.headers["Last-Modified"] = Time.now.httpdate # Add this line if your Rack version is 2.2.x
...
end |
This should all be fixed in Rails 7.1 with Rack 3. |
When you upgrade rack from 2.1.x to 2.2.x the streaming functionality in rails stops working.
A simple controller like this:
will not stream anymore and will send all the data at the end of the cicle.
After many attempts I've pinned down that the culprit is Rack 2.2.x.
with version 2.2.0 if you invoke that controller will raise an exception:
with version 2.2.1 no exception is raised but the streaming does not work. same on version 2.2.2
I've tested it with Rails 5.1.x and 6.0.2.1 (the latest). Same behaviour.
The text was updated successfully, but these errors were encountered: