Skip to content
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

Restore response.stream in render_to_string with ActionController::Live #11623

Closed
wants to merge 1 commit into from

Conversation

@sorah
Copy link
Contributor

@sorah sorah commented Jul 27, 2013

Fix render_to_string in ActionController::Live to not break response.stream;
By wrapping render_to_string method to restore response.stream too.

Because render_to_string modifies response_body temporally, but
it doesn't restore response.stream.

This will work properly by this fix:

class FooController < ActionController::Base
  include ActionController::Live

  def streaming
    response.stream.write "foo\n"

    # This breaks existing response.stream before
    render_to_string(text: 'foo')

    # This will raise IOError because render_to_string re-news `response.stream`,
    # and it has closed by `ActionController::Live#response_body=`
    response.stream.write "bar\n"
    response.stream.close
  end
end

Confirmed this can reproduce at Rails 4.0.0 too.

@@ -91,6 +91,8 @@ def to_hash
end
end

attr_writer :stream

This comment has been minimized.

@sorah

sorah Jul 27, 2013
Author Contributor

I'm not sure this is good way, instance_variable_set for instead?

Fix `render_to_string` in `ActionController::Live` to not break `response.stream`;
By wrapping `render_to_string` method to restore `response.stream` too.

Because `render_to_string` modifies response_body temporally, but
it doesn't restore `response.stream`.

This will work properly by this fix:

    class FooController < ActionController::Base
      include ActionController::Live

      def streaming
        response.stream.write "foo\n"

        # This breaks existing response.stream before
        render_to_string(text: 'zomg')

        response.stream.write "bar\n"
        response.stream.close
      end
    end
@josevalim
Copy link
Contributor

@josevalim josevalim commented Sep 11, 2013

We have recently changed how render_to_string works, maybe that fixed the issue? We just need a test case to avoid future regressions then.

@davidhq
Copy link

@davidhq davidhq commented Nov 10, 2013

In 4.0.1 there was still a problem (easily fixed with http://blog.sorah.jp/2013/07/28/render_to_string-in-ac-live though)
I wanted to try in edge rails but I have other problem: activerecord-hackery/polyamorous#6 and can't even test it

@davidhq
Copy link

@davidhq davidhq commented Nov 10, 2013

UPDATE: this "easy fix" I was mentioning does not actually work... it only works for the server side so it appears to be sending data, but client-side SSE JavaScript won't be receiving events for some reason... see here: http://stackoverflow.com/questions/19890266/sse-server-sent-events-not-working

@scomma
Copy link

@scomma scomma commented Dec 19, 2013

This approach introduces a new bug, where upon the client side closing the connection, an IOError will not be raised when we attempt to write to response.stream. Consequently the thread will never finish.

@scomma
Copy link

@scomma scomma commented Dec 19, 2013

Update. render_to_body doesn't attempt to close the stream so it's safe to use that without requiring this patch.

@maclover7
Copy link
Member

@maclover7 maclover7 commented Jan 10, 2016

Please rebase.

@NARKOZ
Copy link
Contributor

@NARKOZ NARKOZ commented Nov 20, 2017

@sorah please rebase.

@rails-bot
Copy link

@rails-bot rails-bot bot commented Dec 18, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.