Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Exceptions raised when using ActionController::Live cause server crash #9604

Merged
merged 1 commit into from

8 participants

@sgrif
Collaborator

Any exceptions that occur in a controller using ActionController::Live would cause the server to either hang with an open socket indefinitely, or immediately crash (depending on whether the server was launched with rails s or directly).

Easiest way to duplicate the issue is to include ActionController::Live, and visit a route that has no associated view, or to call an undefined method. Issue was reproduced with Ruby 1.9.3 as well as 2.0.

Added a catch block in Live to make exception handling work similarly to streaming templates. A redirect to 500.html is issued using a script tag, the stream is closed, and the exception is logged to the terminal.

actionpack/lib/action_controller/metal/live.rb
@@ -116,6 +116,10 @@ def process(name)
begin
super(name)
+ rescue => e
+ @_response.stream.write ActionView::Base.streaming_completion_on_exception
@josevalim Owner

We definitely should not write this. It may not be the case that html is being streamed.

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

@josevalim Changed to only return the html redirect for html requests. Am I correct in assuming we want a plaintext error output otherwise?

@josevalim
Owner

@sgrif I don't think we should output anything at all. For server sent events, there is nothing we could output that would make sense. Even more, the failure could even be due to errors in the socket, so pushing anything could just make things worse. Maybe we would provide a on_error callback in the response stream that users could customize, but i don't think we should do anything out the box.

@sgrif
Collaborator

@josevalim Both of those make sense, I'll just need to make sure that an error in the on_error block doesn't cause additional problems. It seems like it might not be immediately apparent that all requests to a controller using Live are streamed, even if you're just rendering a view as normal. Do you think it would make sense to only stream when stream.write is called directly from the controller, or if stream: true is passed to render? If not, we should probable change the wording in the documentation from "all actions in that controller will be able to stream" to "all actions will stream"

@sgrif
Collaborator

@josevalim I've made the changes requested.

@sikachu
Collaborator

@sgrif would you mind rebasing this against master and force push to your branch again? It's not mergeable right now. Thanks!

@sgrif sgrif Exception handling for controllers using ActionController::Live
Any exceptions that occured at the view or controller level for a
controller using ActionController::Live would cause the server to either
hang with an open socket indefinitely, or immediately crash (depending
on whether the server was launched with rails s or directly). Changed
the behavior of exceptions to act the same as streaming templates for
html requests, and allow for an on_error callback if needed.
c01d080
@sgrif
Collaborator

@sikachu Sure. I squashed down to a single commit as well.

@geopet

Just checking in on this PR. I'm curious to know what are necessary next steps before this can be merged.

@josevalim josevalim was assigned
@tenderlove
Owner

This seems good to me. @josevalim?

@rafaelfranca rafaelfranca merged commit 63f7356 into from
@egilburg egilburg commented on the diff
actionpack/lib/action_controller/metal/live.rb
@@ -121,6 +129,16 @@ def process(name)
begin
super(name)
+ rescue => e
+ begin
+ @_response.stream.write(ActionView::Base.streaming_completion_on_exception) if request.format == :html
+ @_response.stream.call_on_error
+ rescue => exceptionception
@egilburg
egilburg added a note

is this a typo?

@sgrif Collaborator
sgrif added a note

It's a reference to the movie Inception (exception handler within an exception handler)

@rafaelfranca Owner

lol. Fixed. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 18, 2013
  1. @sgrif

    Exception handling for controllers using ActionController::Live

    sgrif authored
    Any exceptions that occured at the view or controller level for a
    controller using ActionController::Live would cause the server to either
    hang with an open socket indefinitely, or immediately crash (depending
    on whether the server was launched with rails s or directly). Changed
    the behavior of exceptions to act the same as streaming templates for
    html requests, and allow for an on_error callback if needed.
This page is out of date. Refresh to see the latest.
View
28 actionpack/lib/action_controller/metal/live.rb
@@ -56,6 +56,14 @@ def close
super
@buf.push nil
end
+
+ def on_error(&block)
+ @error_callback = block
+ end
+
+ def call_on_error
+ @error_callback.call
+ end
end
class Response < ActionDispatch::Response #:nodoc: all
@@ -121,6 +129,16 @@ def process(name)
begin
super(name)
+ rescue => e
+ begin
+ @_response.stream.write(ActionView::Base.streaming_completion_on_exception) if request.format == :html
+ @_response.stream.call_on_error
+ rescue => exceptionception
@egilburg
egilburg added a note

is this a typo?

@sgrif Collaborator
sgrif added a note

It's a reference to the movie Inception (exception handler within an exception handler)

@rafaelfranca Owner

lol. Fixed. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ log_error(exceptionception)
+ ensure
+ log_error(e)
+ @_response.stream.close
+ end
ensure
@_response.commit!
end
@@ -129,6 +147,16 @@ def process(name)
@_response.await_commit
end
+ def log_error(exception)
+ logger = ActionController::Base.logger
+ return unless logger
+
+ message = "\n#{exception.class} (#{exception.message}):\n"
+ message << exception.annoted_source_code.to_s if exception.respond_to?(:annoted_source_code)
+ message << " " << exception.backtrace.join("\n ")
+ logger.fatal("#{message}\n\n")
+ end
+
def response_body=(body)
super
response.stream.close if response
View
76 actionpack/test/controller/live_stream_test.rb
@@ -52,6 +52,29 @@ def thread_locals
def with_stale
render :text => 'stale' if stale?(:etag => "123")
end
+
+ def exception_in_view
+ render 'doesntexist'
+ end
+
+ def exception_with_callback
+ response.headers['Content-Type'] = 'text/event-stream'
+
+ response.stream.on_error do
+ response.stream.write %(data: "500 Internal Server Error"\n\n)
+ response.stream.close
+ end
+
+ raise 'An exception occurred...'
+ end
+
+ def exception_in_exception_callback
+ response.headers['Content-Type'] = 'text/event-stream'
+ response.stream.on_error do
+ raise 'We need to go deeper.'
+ end
+ response.stream.write params[:widget][:didnt_check_for_nil]
+ end
end
tests TestController
@@ -66,6 +89,21 @@ def build_response
TestResponse.new
end
+ def assert_stream_closed
+ assert response.stream.closed?, 'stream should be closed'
+ end
+
+ def capture_log_output
+ output = StringIO.new
+ old_logger, ActionController::Base.logger = ActionController::Base.logger, ActiveSupport::Logger.new(output)
+
+ begin
+ yield output
+ ensure
+ ActionController::Base.logger = old_logger
+ end
+ end
+
def test_set_response!
@controller.set_response!(@request)
assert_kind_of(Live::Response, @controller.response)
@@ -119,7 +157,43 @@ def test_live_stream_default_header
def test_render_text
get :render_text
assert_equal 'zomg', response.body
- assert response.stream.closed?, 'stream should be closed'
+ assert_stream_closed
+ end
+
+ def test_exception_handling_html
+ capture_log_output do |output|
+ get :exception_in_view
+ assert_match %r((window\.location = "/500\.html"</script></html>)$), response.body
+ assert_match 'Missing template test/doesntexist', output.rewind && output.read
+ assert_stream_closed
+ end
+ end
+
+ def test_exception_handling_plain_text
+ capture_log_output do |output|
+ get :exception_in_view, format: :json
+ assert_equal '', response.body
+ assert_match 'Missing template test/doesntexist', output.rewind && output.read
+ assert_stream_closed
+ end
+ end
+
+ def test_exception_callback
+ capture_log_output do |output|
+ get :exception_with_callback, format: 'text/event-stream'
+ assert_equal %(data: "500 Internal Server Error"\n\n), response.body
+ assert_match 'An exception occurred...', output.rewind && output.read
+ assert_stream_closed
+ end
+ end
+
+ def test_exceptions_raised_handling_exceptions
+ capture_log_output do |output|
+ get :exception_in_exception_callback, format: 'text/event-stream'
+ assert_equal '', response.body
+ assert_match 'We need to go deeper', output.rewind && output.read
+ assert_stream_closed
+ end
end
def test_stale_without_etag
Something went wrong with that request. Please try again.