Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
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...
commit a7b059ec7f25c16beeacf2c545d2be593ed0388b 1 parent 30d21df
Aaron Patterson tenderlove authored
7 actionpack/lib/action_controller/metal/live.rb
View
@@ -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
Matthew Draper Collaborator
matthewd added a note

Do we still want to do this stuff if we're going to raise a proper exception?

See also #14090

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@_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)
32 actionpack/test/controller/live_stream_test.rb
View
@@ -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
Matthew Draper

Do we still want to do this stuff if we're going to raise a proper exception?

See also #14090

Please sign in to comment.
Something went wrong with that request. Please try again.