Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

re-raise error if error occurs before committing in streaming #14090

Merged
merged 1 commit into from

3 participants

@Fortisque

Let errors get handled in the default way if an error occurs before
we start streaming. This reverts #14000 but offers a better fix.

@matthewd matthewd referenced this pull request from a commit
@tenderlove tenderlove 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.
a7b059e
@tenderlove
Owner

Can you rebase this patch?

@Fortisque

Okay I rebased it, I think beside the variable name changes my patch just had these updates to the tests.

@matthewd
Collaborator

@Fortisque you had an 'else' that we want

@Fortisque

Oh I see, I put the else back

@matthewd
Collaborator

Now the if/else bodies are the wrong way around :smile:

@matthewd
Collaborator

:+1:

/ping @tenderlove

@tenderlove tenderlove merged commit ba3ad25 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 15, 2014
  1. @Fortisque

    re-raise error if error occurs before committing in streaming

    Fortisque authored
    update the tests, using an if-else
This page is out of date. Refresh to see the latest.
View
21 actionpack/lib/action_controller/metal/live.rb
@@ -228,18 +228,19 @@ def process(name)
begin
super(name)
rescue => e
- unless @_response.committed?
+ if @_response.committed?
+ begin
+ @_response.stream.write(ActionView::Base.streaming_completion_on_exception) if request.format == :html
+ @_response.stream.call_on_error
+ rescue => exception
+ log_error(exception)
+ ensure
+ log_error(e)
+ @_response.stream.close
+ end
+ else
error = e
end
- begin
- @_response.stream.write(ActionView::Base.streaming_completion_on_exception) if request.format == :html
- @_response.stream.call_on_error
- rescue => exception
- log_error(exception)
- ensure
- log_error(e)
- @_response.stream.close
- end
ensure
@_response.commit!
end
View
19 actionpack/test/controller/live_stream_test.rb
@@ -153,6 +153,11 @@ def exception_in_view
render 'doesntexist'
end
+ def exception_in_view_after_commit
+ response.stream.write ""
+ render 'doesntexist'
+ end
+
def exception_with_callback
response.headers['Content-Type'] = 'text/event-stream'
@@ -269,6 +274,13 @@ def test_exception_handling_html
assert_raises(ActionView::MissingTemplate) do
get :exception_in_view
end
+
+ capture_log_output do |output|
+ get :exception_in_view_after_commit
+ 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 response.body
assert_stream_closed
end
@@ -277,6 +289,13 @@ def test_exception_handling_plain_text
assert_raises(ActionView::MissingTemplate) do
get :exception_in_view, format: :json
end
+
+ capture_log_output do |output|
+ get :exception_in_view_after_commit, 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_when_committed
Something went wrong with that request. Please try again.