Permalink
Browse files

use built-in exception handling in live controllers

when an exception happens in an action before the response has been
committed, then we should re-raise the exception in the main thread.
This lets us reuse the existing exception handling.
  • Loading branch information...
1 parent a6d2c97 commit 20416f17bdbbb02603605be6e86f31245be553bc @tenderlove tenderlove committed Feb 28, 2014
Showing with 22 additions and 17 deletions.
  1. +5 −2 actionpack/lib/action_controller/metal/live.rb
  2. +17 −15 actionpack/test/controller/live_stream_test.rb
@@ -203,6 +203,7 @@ def process(name)
t1 = Thread.current
locals = t1.keys.map { |key| [key, t1[key]] }
+ error = nil
# This processes the action in a child thread. It lets us return the
# response code and headers back up the rack stack, and still process
# the body in parallel with sending data to the client
@@ -217,8 +218,9 @@ def process(name)
begin
super(name)
rescue => e
- @_response.status = 500 unless @_response.committed?
- @_response.status = 400 if e.class == ActionController::BadRequest
+ unless @_response.committed?
+ error = e
+ end
begin
@_response.stream.write(ActionView::Base.streaming_completion_on_exception) if request.format == :html
@_response.stream.call_on_error
@@ -234,6 +236,7 @@ def process(name)
}
@_response.await_commit
+ raise error if error
end
def log_error(exception)
@@ -90,6 +90,9 @@ def test_sse_with_id
end
class LiveStreamTest < ActionController::TestCase
+ class Exception < StandardError
+ end
+
class TestController < ActionController::Base
include ActionController::Live
@@ -152,11 +155,12 @@ def exception_with_callback
response.stream.close
end
+ response.stream.write "" # make sure the response is committed
raise 'An exception occurred...'
end
def exception_in_controller
- raise 'Exception in controller'
+ raise Exception, 'Exception in controller'
end
def bad_request_error
@@ -168,6 +172,7 @@ def exception_in_exception_callback
response.stream.on_error do
raise 'We need to go deeper.'
end
+ response.stream.write ''
response.stream.write params[:widget][:didnt_check_for_nil]
end
end
@@ -246,24 +251,19 @@ def test_render_text
end
def test_exception_handling_html
- capture_log_output do |output|
+ assert_raises(ActionView::MissingTemplate) do
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
+ assert_stream_closed
end
def test_exception_handling_plain_text
- capture_log_output do |output|
+ assert_raises(ActionView::MissingTemplate) do
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
+ def test_exception_callback_when_committed
capture_log_output do |output|
get :exception_with_callback, format: 'text/event-stream'
assert_equal %(data: "500 Internal Server Error"\n\n), response.body
@@ -273,16 +273,18 @@ def test_exception_callback
end
def test_exception_in_controller_before_streaming
- response = get :exception_in_controller, format: 'text/event-stream'
- assert_equal 500, response.status
+ assert_raises(ActionController::LiveStreamTest::Exception) do
+ get :exception_in_controller, format: 'text/event-stream'
+ end
end
def test_bad_request_in_controller_before_streaming
- response = get :bad_request_error, format: 'text/event-stream'
- assert_equal 400, response.status
+ assert_raises(ActionController::BadRequest) do
+ get :bad_request_error, format: 'text/event-stream'
+ end
end
- def test_exceptions_raised_handling_exceptions
+ def test_exceptions_raised_handling_exceptions_and_committed
capture_log_output do |output|
get :exception_in_exception_callback, format: 'text/event-stream'
assert_equal '', response.body

0 comments on commit 20416f1

Please sign in to comment.