Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

show exception middleware accepts multiple apps #7804

Closed
wants to merge 2 commits into from

5 participants

@schneems
Collaborator

The show exception middleware allows developers to use custom error rendering apps. As error rendering applications become more complicated the chance for errors in the error rendering applications increases. By allowing multiple exception apps to be used instead of just one, a developer can provide a fallback application. This provides a better experience for the end user and decreases the chances that the FAILSAFE_RESPONSE will be used.

By default the public exception app will be put after any applications specified using config.exceptions_app in rails. This will provide a fallback in case an error is introduced by the exceptions_app.

Errors from within exception_apps will still be shown in the logs for debugging. This was inspired by the desire to have safer dynamic error pages #7772 but with or without that PR, I believe this PR still provides valuable behavior.

ATP Railties and Action Pack. Btw, it's my birthday.

@steveklabnik
Collaborator

:cake:

@schneems
Collaborator

Thanks for the :cake: I had a fantastic :birthday:. Any comment on PR concept or implementation?

@rafaelfranca rafaelfranca commented on the diff
railties/test/application/middleware/exceptions_test.rb
((8 lines not shown))
+ end
+ RUBY
+
+ app.config.action_dispatch.show_exceptions = true
+
+ get "/foo"
+ assert_equal 404, last_response.status
+ assert_match "The page you were looking for doesn't exist.", last_response.body
+ end
+
+ test "accepts multiple apps for fallback" do
+ add_to_config <<-RUBY
+ exception_apps =
+ config.exceptions_app = [
+ lambda do |env|
+ raise "oppa gangnam style"
@rafaelfranca Owner

lol

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

I like the idea. The implementation seems fine.

@spastorino @jeremy @josevalim thoughts?

@rafaelfranca
Owner

@schneems BTW. Happy birthday

@schneems
Collaborator

@rafaelfranca, thanks!

@schneems
Collaborator

Ping. @spastorino, @jeremy, @josevalim: thoughts? Merge or close plz.

@rafaelfranca
Owner

I think @spastorino has some thoughts but I don't remember what.

...ack/lib/action_dispatch/middleware/show_exceptions.rb
@@ -38,16 +40,38 @@ def call(env)
private
+ def name_for_app(app)
+ if app.is_a? ActionDispatch::PublicExceptions
+ "static error pages from public dir"
+ elsif app.is_a? ActionDispatch::Routing::RouteSet
+ "action specified in routes"
+ else
+ app.class.name
+ end
+ end
@spastorino Owner

why this?

@schneems Collaborator
schneems added a note

Usability and clarity.

Rendering exception status: 500 with ActionDispatch::Routing::RouteSet

Was not intuitively obvious to some people I showed this to. This seemed to be better:

Rendering exception status: 500 with action specified in routes

The two most common cases would be the Rails routes and the PublicExceptions app, so I added this helper to clean up the names a bit. Would you prefer if I kept the original class name in the description?

Rendering exception status: 500 with action specified in routes (ActionDispatch::Routing::RouteSet)
@spastorino Owner

yes, add the names too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ack/lib/action_dispatch/middleware/show_exceptions.rb
((14 lines not shown))
def render_exception(env, exception)
wrapper = ExceptionWrapper.new(env, exception)
status = wrapper.status_code
env["action_dispatch.exception"] = wrapper.exception
env["PATH_INFO"] = "/#{status}"
- response = @exceptions_app.call(env)
- response[1]['X-Cascade'] == 'pass' ? pass_response(status) : response
- rescue Exception => failsafe_error
- $stderr.puts "Error during failsafe response: #{failsafe_error}\n #{failsafe_error.backtrace * "\n "}"
- FAILSAFE_RESPONSE
+ response = nil
+
+ # iterate over each exception app, store for first valid response
+ @exception_apps.each do |exception_app|
+ begin
+ $stderr.puts "Rendering exception status: #{status} with #{name_for_app(exception_app)}"
@spastorino Owner

why are you using stderr?

@schneems Collaborator
schneems added a note

Not entirely sure, don't think I had a good reason. I can put to stdout instead...i'll make that change soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ack/lib/action_dispatch/middleware/show_exceptions.rb
((14 lines not shown))
def render_exception(env, exception)
wrapper = ExceptionWrapper.new(env, exception)
status = wrapper.status_code
env["action_dispatch.exception"] = wrapper.exception
env["PATH_INFO"] = "/#{status}"
- response = @exceptions_app.call(env)
- response[1]['X-Cascade'] == 'pass' ? pass_response(status) : response
- rescue Exception => failsafe_error
- $stderr.puts "Error during failsafe response: #{failsafe_error}\n #{failsafe_error.backtrace * "\n "}"
- FAILSAFE_RESPONSE
+ response = nil
+
+ # iterate over each exception app, store for first valid response
+ @exception_apps.each do |exception_app|
+ begin
+ $stderr.puts "Rendering exception status: #{status} with #{name_for_app(exception_app)}"
+ response ||= exception_app.call(env)
@spastorino Owner

why are you iterating through all the apps? if the first one here returns something the rest are useless, no?

@schneems Collaborator
schneems added a note

The others never get called if response gets set, I played with some other iterator methods and this way was the cleanest. I'm open to suggestion.

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

Changed stderr to stdout since it wasn't an actual error

@jeremy
Owner

Makes sense to want failsafe error handling of error handlers also. How about doing it the Rack way -- wrapping the Rack app with a failsafe handler app -- rather than passing multiple Rack apps and trying each in turn?

@spastorino
Owner

Yeah agree with @jeremy on this one

@schneems schneems show exception middleware accepts multiple apps
The show exception middleware allows developers to use custom error rendering apps. As error rendering applications become more complicated the chance for errors in the error rendering applications increases. By allowing multiple exception apps to be used instead of just one a developer can provide a fallback application. This provides a better experience for the end user and decreases the chances that the `FAILSAFE_RESPONSE` will be used.

By default the public exception app will be put after any applications specified using `config.exceptions_app` in rails. This will provide a fallback in case an error is introduced by the exceptions_app.

Errors from within exception_apps will still be shown in the logs for debugging.
7610886
@schneems schneems commented on the diff
...ack/lib/action_dispatch/middleware/show_exceptions.rb
@@ -38,16 +40,38 @@ def call(env)
private
+ def name_for_app(app)
+ if app.is_a? ActionDispatch::PublicExceptions
+ "static error pages from public dir (#{app.class.name})"
+ elsif app.is_a? ActionDispatch::Routing::RouteSet
+ "action specified in routes (#{app.class.name})"
+ else
+ "middleware (#{app.class.name})"
+ end
+ end
+
@schneems Collaborator
schneems added a note

Updated this method

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

@jeremy @spastorino One of ShowExceptions middleware purpose is to wrap it in a failsafe response, so even if all the exceptions apps have exceptions we still return something. I want to be able to provide multiple apps for sequential fallback. I don't entirely follow the suggestion, can you you give me some pseudo code, or explain the idea some more?

@jeremy
Owner

@schneems you can wrap your exceptions app with a failsafe middleware and use that as your config.exceptions_app. For example:

config.exceptions_app = MyExceptionHandler.new(ActionDispatch::PublicExceptions.new(Rails.public_path))

or

config.exceptions_app = MyExceptionHandler.new(config.exceptions_app)
@schneems
Collaborator

I see what you mean now, the ShowExceptions app already wraps anything we pass to config.exceptions_app, wrapping an app to pass to an app that wraps it seems a bit redundant. Even if I was to write such a middleware it would be very very similar to the Show Exceptions middleware. If we wanted it to handle multiple fallbacks, that middleware would need a loop as well. Adding an extra piece of middleware wouldn't decrease complexity, or necessarily make ownership clearer it would just be moving existing code around.

Ideally anyone already using dynamic error pages outlined here http://blog.plataformatec.com.br/2012/01/my-five-favorite-hidden-features-in-rails-3-2/ like :

config.exceptions_app = self.routes

Would be able to take advantage of this fallback behavior without having to change their interface, without having to specify an additional middleware app.

@steveklabnik
Collaborator

Hey @schneems are you still working on this?

@schneems
Collaborator

Yeah, I'm still not convinced that a wrapper middleware is the way to go, but ill try an implementation and maybe that will change my mind. Ill try to get this done either before thanksgiving or right after. Please don't close just yet.

@schneems schneems referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@schneems schneems referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@schneems schneems referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@schneems schneems referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@schneems schneems referenced this pull request from a commit in schneems/rails
@schneems schneems Enable Exception Fallback/Failsafe Behavior
Right now we have a default error handling app `ActionDispatch::PublicExceptions` that renders the static 404 & 500 pages. If you don't like it you can override this behavior by specifying an exceptions app in the config of your application using `config.exceptions_app`. Right now if you do so, and you have an exception in your exceptions app, your users will not see your 404 or 500 pages, but instead a FAILSAFE_RESPONSE that simply renders some text.

This change gives the ability to have MULTIPLE fallback apps. So anyone can write exception handling apps, and if those apps fails, it will continue to try to use the public app. This design also allows the developer to specify additional apps if they choose.

There are two key things in this PR, first that this fallback behavior to the public exceptions app is automatic, and second that by default you can specify multiple apps.

This is an alternative implementation of an existing PR #7804 made at the behest of @jeremy. I believe this is a cleaner implementation.

ATP railties and actionpack cc/ @rafael @rafaelfranca @spastorino @steveklabnik
5c5b023
@schneems
Collaborator

closing this in favor of #8425

@schneems schneems closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 7, 2012
  1. @schneems
Commits on Oct 9, 2012
  1. @schneems

    show exception middleware accepts multiple apps

    schneems authored
    The show exception middleware allows developers to use custom error rendering apps. As error rendering applications become more complicated the chance for errors in the error rendering applications increases. By allowing multiple exception apps to be used instead of just one a developer can provide a fallback application. This provides a better experience for the end user and decreases the chances that the `FAILSAFE_RESPONSE` will be used.
    
    By default the public exception app will be put after any applications specified using `config.exceptions_app` in rails. This will provide a fallback in case an error is introduced by the exceptions_app.
    
    Errors from within exception_apps will still be shown in the logs for debugging.
This page is out of date. Refresh to see the latest.
View
2  actionpack/lib/action_dispatch/middleware/public_exceptions.rb
@@ -1,4 +1,6 @@
module ActionDispatch
+ # This middleware takes exceptions from the application and renders
+ # the corresponding static error page from the public directory.
class PublicExceptions
attr_accessor :public_path
View
42 actionpack/lib/action_dispatch/middleware/show_exceptions.rb
@@ -4,6 +4,8 @@
module ActionDispatch
# This middleware rescues any exception returned by the application
# and calls an exceptions app that will wrap it in a format for the end user.
+ # Multiple exception apps can be specified, in case there is an error in one
+ # of the apps, the first app with a valid response will be used.
#
# The exceptions app should be passed as parameter on initialization
# of ShowExceptions. Everytime there is an exception, ShowExceptions will
@@ -12,8 +14,8 @@ module ActionDispatch
#
# If the application returns a "X-Cascade" pass response, this middleware
# will send an empty response as result with the correct status code.
- # If any exception happens inside the exceptions app, this middleware
- # catches the exceptions and returns a FAILSAFE_RESPONSE.
+ # If an exception happens inside the all the exceptions apps, this middleware
+ # catches the exception and returns a FAILSAFE_RESPONSE.
class ShowExceptions
FAILSAFE_RESPONSE = [500, { 'Content-Type' => 'text/plain' },
["500 Internal Server Error\n" <<
@@ -21,9 +23,9 @@ class ShowExceptions
"application's log file and/or the web server's log file to find out what " <<
"went wrong."]]
- def initialize(app, exceptions_app)
+ def initialize(app, *exception_apps)
@app = app
- @exceptions_app = exceptions_app
+ @exception_apps = exception_apps.flatten.compact.uniq
end
def call(env)
@@ -38,16 +40,38 @@ def call(env)
private
+ def name_for_app(app)
+ if app.is_a? ActionDispatch::PublicExceptions
+ "static error pages from public dir (#{app.class.name})"
+ elsif app.is_a? ActionDispatch::Routing::RouteSet
+ "action specified in routes (#{app.class.name})"
+ else
+ "middleware (#{app.class.name})"
+ end
+ end
+
@schneems Collaborator
schneems added a note

Updated this method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
def render_exception(env, exception)
wrapper = ExceptionWrapper.new(env, exception)
status = wrapper.status_code
env["action_dispatch.exception"] = wrapper.exception
env["PATH_INFO"] = "/#{status}"
- response = @exceptions_app.call(env)
- response[1]['X-Cascade'] == 'pass' ? pass_response(status) : response
- rescue Exception => failsafe_error
- $stderr.puts "Error during failsafe response: #{failsafe_error}\n #{failsafe_error.backtrace * "\n "}"
- FAILSAFE_RESPONSE
+ response = nil
+
+ # iterate over each exception app, store for first valid response
+ @exception_apps.each do |exception_app|
+ begin
+ $stdout.puts "Rendering exception status: #{status} with #{name_for_app(exception_app)}"
+ response ||= exception_app.call(env)
+ rescue Exception => rescue_error
+ $stderr.puts "Error rendering exception #{status} with #{name_for_app(exception_app)}: #{rescue_error}\n #{rescue_error.backtrace * "\n "}"
+ end
+ end
+
+ if response
+ response[1]['X-Cascade'] == 'pass' ? pass_response(status) : response
+ else
+ FAILSAFE_RESPONSE
+ end
end
def pass_response(status)
View
2  railties/lib/rails/application.rb
@@ -327,7 +327,7 @@ def default_middleware_stack #:nodoc:
middleware.use ::Rack::MethodOverride
middleware.use ::ActionDispatch::RequestId
middleware.use ::Rails::Rack::Logger, config.log_tags # must come after Rack::MethodOverride to properly log overridden methods
- middleware.use ::ActionDispatch::ShowExceptions, config.exceptions_app || ActionDispatch::PublicExceptions.new(Rails.public_path)
+ middleware.use ::ActionDispatch::ShowExceptions, config.exceptions_app, ActionDispatch::PublicExceptions.new(Rails.public_path)
middleware.use ::ActionDispatch::DebugExceptions, app
middleware.use ::ActionDispatch::RemoteIp, config.action_dispatch.ip_spoofing_check, config.action_dispatch.trusted_proxies
View
34 railties/test/application/middleware/exceptions_test.rb
@@ -60,6 +60,40 @@ def index
assert_equal "YOU FAILED BRO", last_response.body
end
+ test "falls back on public app if custom exception app raises error" do
+ add_to_config <<-RUBY
+ config.exceptions_app = lambda do |env|
+ raise "hover boards don't work on water"
+ end
+ RUBY
+
+ app.config.action_dispatch.show_exceptions = true
+
+ get "/foo"
+ assert_equal 404, last_response.status
+ assert_match "The page you were looking for doesn't exist.", last_response.body
+ end
+
+ test "accepts multiple apps for fallback" do
+ add_to_config <<-RUBY
+ exception_apps =
+ config.exceptions_app = [
+ lambda do |env|
+ raise "oppa gangnam style"
@rafaelfranca Owner

lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end,
+ lambda do |env|
+ [404, { "Content-Type" => "text/plain" }, ["YOU FAILED BRO"]]
+ end
+ ]
+ RUBY
+
+ app.config.action_dispatch.show_exceptions = true
+
+ get "/foo"
+ assert_equal 404, last_response.status
+ assert_equal "YOU FAILED BRO", last_response.body
+ end
+
test "unspecified route when action_dispatch.show_exceptions is not set raises an exception" do
app.config.action_dispatch.show_exceptions = false
Something went wrong with that request. Please try again.