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

streaming interrupted before complete with ActionController::Live in Rails 5 for production environment #25581

Closed
rosenfeld opened this Issue Jun 29, 2016 · 7 comments

Comments

Projects
None yet
5 participants
@rosenfeld
Contributor

rosenfeld commented Jun 29, 2016

Sorry, at the time being I don't have much details or a sample application to reproduce the issue but I was finally able to understand that my streamed response was interrupted after the first writes after I upgraded the application to Rails 5. Unfortunately it only happens in my production (and staging) servers, not under the development environment. I have to sign off for today but I'd love to get any input that might help me digging on what could be causing this issue in production with Rails 5.

Any ideas on what has been changed in Rails 5 that could be causing this issue? This is not related to nginx since I tried using curl directly to localhost:port and I get the same error. as soon as the controller has to wait for some more data the connection is interrupted so I only see the output of the first stream.write calls. The action finishes without any errors so I don't know what could be happening.

If I can't think of anything that could fix this tomorrow morning I'm afraid I'll have to downgrade to Rails 4 until I'm able to figure out what's happening. Please let me know if you think you can help with this issue.

Thanks.

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Jun 29, 2016

@rosenfeld

This comment has been minimized.

Show comment
Hide comment
@rosenfeld

rosenfeld Jun 30, 2016

Contributor

I have a bit more details, not sure whether or not it will help. My first attempt to upgrade to Rails 5 was with beta 4. Just before the upgrade the feature worked fine and failed just after upgrading to Rails 5 beta 4.

Contributor

rosenfeld commented Jun 30, 2016

I have a bit more details, not sure whether or not it will help. My first attempt to upgrade to Rails 5 was with beta 4. Just before the upgrade the feature worked fine and failed just after upgrading to Rails 5 beta 4.

@rosenfeld

This comment has been minimized.

Show comment
Hide comment
@rosenfeld

rosenfeld Jun 30, 2016

Contributor

Ok, I was finally able to track down what is causing the problem. The response being created is the regular one rather than the live version because make_response! is not being called from the Live module. It works if I include Live directly in the controller but this is how my controllers handle streaming:

class MyController < ApplicationController
  include StreamingSupport
end

And StreamingSupport is defined as:

require_dependency 'action_controller'

# Adds support for streaming using ActionController::Live.
#
# Also handle the unauthenticated cases where warden would throw a
# :warden symbol, but it would cause an exception because the live
# controller runs in a separate thread and only propagates exceptions.
# There is currently no way to proparate throws through the Thread
# boundary.
#
# To handle this situation, we detect an ArgumentError caused by
# an uncaught throw by warden, then we rethrow the :warden symbol
# without any parameters. This will cause the appropriate warden
# handlers to run. Note that custom actions or any other parameters
# to throw :warden are lost.
#
# https://github.com/hassox/warden/wiki/Failures
# https://bugs.ruby-lang.org/issues/6372
# https://github.com/plataformatec/devise/issues/2332
# https://github.com/rails/rails/issues/13873
module StreamingSupport
  include ActionController::Live # for it to work on Rails 5 this must be moved directly
            # to each controller including StreamingSupport

  def process(name)
    super(name)
  rescue ArgumentError => e
    if e.message == 'uncaught throw :warden'
      throw :warden
    else
      raise e
    end
  end
end

If I move that include ActionController::Live line to each controller using streaming than it seems to work fine. Is this expected?

Contributor

rosenfeld commented Jun 30, 2016

Ok, I was finally able to track down what is causing the problem. The response being created is the regular one rather than the live version because make_response! is not being called from the Live module. It works if I include Live directly in the controller but this is how my controllers handle streaming:

class MyController < ApplicationController
  include StreamingSupport
end

And StreamingSupport is defined as:

require_dependency 'action_controller'

# Adds support for streaming using ActionController::Live.
#
# Also handle the unauthenticated cases where warden would throw a
# :warden symbol, but it would cause an exception because the live
# controller runs in a separate thread and only propagates exceptions.
# There is currently no way to proparate throws through the Thread
# boundary.
#
# To handle this situation, we detect an ArgumentError caused by
# an uncaught throw by warden, then we rethrow the :warden symbol
# without any parameters. This will cause the appropriate warden
# handlers to run. Note that custom actions or any other parameters
# to throw :warden are lost.
#
# https://github.com/hassox/warden/wiki/Failures
# https://bugs.ruby-lang.org/issues/6372
# https://github.com/plataformatec/devise/issues/2332
# https://github.com/rails/rails/issues/13873
module StreamingSupport
  include ActionController::Live # for it to work on Rails 5 this must be moved directly
            # to each controller including StreamingSupport

  def process(name)
    super(name)
  rescue ArgumentError => e
    if e.message == 'uncaught throw :warden'
      throw :warden
    else
      raise e
    end
  end
end

If I move that include ActionController::Live line to each controller using streaming than it seems to work fine. Is this expected?

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Jun 30, 2016

Member

@rosenfeld if you put extend ActiveSupport::Concern in the StreamingSupport module, it should work. We changed Live to be a concern, so that should clear it up.

Member

tenderlove commented Jun 30, 2016

@rosenfeld if you put extend ActiveSupport::Concern in the StreamingSupport module, it should work. We changed Live to be a concern, so that should clear it up.

@rosenfeld

This comment has been minimized.

Show comment
Hide comment
@rosenfeld

rosenfeld Jun 30, 2016

Contributor

That makes sense. I guess another alternative would be using an "included" hook in the module so that I can include it directly in the controller.

In any case we can simply close this issue if we think I'm the only one taking this approach or we could add some note to the changelog in case someone else would be interested in knowing about this. This was tricky for me because it only happened in the production servers and I was only able to reproduce locally and debug the issue after switching to something similar to the production mode. So, if they test the behavior before going live they would be inclined to think everything is fine until someone reports the error in production.

If you are interested in such a note I can add it to the actionpack changelog and create a PR but if you think this won't affect anyone else than that's fine too, simply close this issue.

However if you prefer me to add this note to the changelog, please let me know where it should go, as this behavior has probably changed in one of the first beta releases, so I'm not sure it should go there or on the more recent notes section...

Contributor

rosenfeld commented Jun 30, 2016

That makes sense. I guess another alternative would be using an "included" hook in the module so that I can include it directly in the controller.

In any case we can simply close this issue if we think I'm the only one taking this approach or we could add some note to the changelog in case someone else would be interested in knowing about this. This was tricky for me because it only happened in the production servers and I was only able to reproduce locally and debug the issue after switching to something similar to the production mode. So, if they test the behavior before going live they would be inclined to think everything is fine until someone reports the error in production.

If you are interested in such a note I can add it to the actionpack changelog and create a PR but if you think this won't affect anyone else than that's fine too, simply close this issue.

However if you prefer me to add this note to the changelog, please let me know where it should go, as this behavior has probably changed in one of the first beta releases, so I'm not sure it should go there or on the more recent notes section...

@eileencodes

This comment has been minimized.

Show comment
Hide comment
Member

eileencodes commented Jun 30, 2016

@rosenfeld

This comment has been minimized.

Show comment
Hide comment
@rosenfeld

rosenfeld Jun 30, 2016

Contributor

Sure, will do. I'll close this issue for the time being and will create the PR later today.

Contributor

rosenfeld commented Jun 30, 2016

Sure, will do. I'll close this issue for the time being and will create the PR later today.

@rosenfeld

This comment has been minimized.

Show comment
Hide comment
@rosenfeld

rosenfeld Jun 30, 2016

Contributor

Done creating the PR's

Contributor

rosenfeld commented Jun 30, 2016

Done creating the PR's

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