Exceptions raised when using ActionController::Live cause server crash #9604
Conversation
| @@ -116,6 +116,10 @@ def process(name) | |||
|
|
|||
| begin | |||
| super(name) | |||
| rescue => e | |||
| @_response.stream.write ActionView::Base.streaming_completion_on_exception | |||
There was a problem hiding this comment.
We definitely should not write this. It may not be the case that html is being streamed.
|
cc @tenderlove |
|
@josevalim Changed to only return the html redirect for html requests. Am I correct in assuming we want a plaintext error output otherwise? |
|
@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. |
|
@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 |
|
@josevalim I've made the changes requested. |
|
@sgrif would you mind rebasing this against master and force push to your branch again? It's not mergeable right now. Thanks! |
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.
|
@sikachu Sure. I squashed down to a single commit as well. |
|
Just checking in on this PR. I'm curious to know what are necessary next steps before this can be merged. |
|
/cc @josevalim |
ghost
assigned 
|
This seems good to me. @josevalim? |
Exceptions raised when using ActionController::Live cause server crash
| begin | ||
| @_response.stream.write(ActionView::Base.streaming_completion_on_exception) if request.format == :html | ||
| @_response.stream.call_on_error | ||
| rescue => exceptionception |
There was a problem hiding this comment.
It's a reference to the movie Inception (exception handler within an exception handler)
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.