Add new hook for exceptions in hooks (#1737)

This adds a new type of hook that is called in the event of an exception
in a hook. Because of its nature it's a bit more complicated than a
normal hook, in particular because it could result in recursive calling.

Because this hook can be called in quite a variety of ways during
response handling, and because the current hook exception behavior is
quite blunt, I also wanted to make it quite flexible in terms of
controlling what happens. In particular, I wanted to be able to change
the default response in the event of an exception in a hook. The
comments in the code and the documentation should all be
self-explanatory in this regard.

Author: abeverley

lib/Dancer2/Core/App.pm

@@ -771,6 +771,7 @@ sub supported_hooks {
       core.app.before_request
       core.app.after_request
       core.app.route_exception
+      core.app.hook_exception
       core.app.before_file_render
       core.app.after_file_render
       core.error.before
@@ -790,6 +791,7 @@ sub hook_aliases {
         before_error           => 'core.error.before',
         after_error            => 'core.error.after',
         on_route_exception     => 'core.app.route_exception',
+        on_hook_exception      => 'core.app.hook_exception',
 
         before_file_render         => 'core.app.before_file_render',
         after_file_render          => 'core.app.after_file_render',
@@ -1208,7 +1210,24 @@ sub compile_hooks {
                 eval  { $EVAL_SHIM->($hook,@_); 1; }
                 or do {
                     my $err = $@ || "Zombie Error";
-                    $app->cleanup;
+                    # Don't execute the hook_exception hook if the exception
+                    # has been generated from a hook exception handler itself,
+                    # thus preventing potentially recursive code.
+                    $self->execute_hook( 'core.app.hook_exception', $app, $err )
+                        unless $position eq 'core.app.hook_exception';
+                    my $is_halted = $app->response->is_halted; # Capture before cleanup
+                    # We can't cleanup if we're in the hook for a hook
+                    # exception, as this would clear the custom response that
+                    # may have been set by the hook. However, there is no need
+                    # to do so, as the upper hook that called this hook
+                    # exception will perform the cleanup instead anyway
+                    $app->cleanup unless $position eq 'core.app.hook_exception';
+                    # Allow the hook function to halt the response, thus
+                    # retaining any response it may have set. Otherwise the
+                    # croak from this function will overwrite any content that
+                    # may have been set by the hook
+                    return if $is_halted;
+                    # Default behavior if nothing else defined
                     $app->log('error', "Exception caught in '$position' filter: $err");
                     croak "Exception caught in '$position' filter: $err";
                 };

lib/Dancer2/Manual.pod

@@ -667,6 +667,8 @@ for details about the following hooks:
 
 =item * C<on_route_exception>
 
+=item * C<on_hook_exception>
+
 =back
 
 =head2 File Rendering
@@ -925,6 +927,21 @@ L<Dancer2::Core::App> and the error as arguments.
     my ($app, $error) = @_;
   };
 
+C<on_hook_exception> is called when an exception has been caught within a hook,
+just before logging and rethrowing it higher. This hook receives a
+L<Dancer2::Core::App> and the error as arguments. If the function provided to
+on_hook_exception causes an exception itself, then this will be ignored (thus
+preventing a potentially recursive situation). However, it is still possible
+for the function to set a custom response and halt it (and optionally die),
+thus providing a method to render custom content in the event of an exception
+in a hook. This is shown in the example below:
+
+  hook on_hook_exception => sub {
+    my ($app, $error) = @_;
+    $app->response->content("Some custom content for the response");
+    $app->response->halt;
+  };
+
 =head1 SESSIONS
 
 =head2 Handling sessions

t/hooks.t

@@ -157,6 +157,60 @@ my $tests_flags = {};
     };
 }
 
+# 3 test apps for the exception hook - see comments below
+{
+    package App::HookException;
+    use Dancer2;
+
+    get '/hook_exception' => sub { return 1 };
+
+    hook after => sub {
+        die 'this is an exception in the after hook';
+    };
+
+    hook on_hook_exception => sub {
+        my ($app, $error) = @_;
+        ::is ref($app), 'Dancer2::Core::App';
+        ::like $error, qr/this is an exception in the after hook/;
+        $app->response->content("Setting response from exception hook");
+        $app->response->halt;
+    };
+}
+
+{
+    package App::HookExceptionDouble;
+    use Dancer2;
+
+    get '/hook_exception_double' => sub { return 1 };
+
+    hook after => sub {
+        die 'this is an exception in the after hook';
+    };
+
+    hook on_hook_exception => sub {
+        my ($app, $error) = @_;
+        $app->response->content("Setting response from exception hook and then dying");
+        $app->response->halt;
+        die 'this is an exception in the exception hook';
+    };
+}
+
+{
+    package App::HookExceptionRecursive;
+    use Dancer2;
+
+    get '/hook_exception_recursive' => sub { return 1 };
+
+    hook after => sub {
+        die 'this is an exception in the after hook';
+    };
+
+    hook on_hook_exception => sub {
+        my ($app, $error) = @_;
+        die 'this is an exception in the hook exception causing recursion';
+    };
+}
+
 subtest 'Request hooks' => sub {
     my $test = Plack::Test->create( App::WithSerializer->to_app );
     $test->request( GET '/' );
@@ -234,4 +288,27 @@ subtest 'route_exception' => sub {
     capture_stderr { $test->request( GET '/route_exception' ) };
 };
 
+# A basic test for a hook that is called in the event of an exception in a hook
+subtest 'hook_exception' => sub {
+    my $test = Plack::Test->create( App::HookException->to_app );
+    my $resp = $test->request( GET '/hook_exception' );
+    is( $resp->content, 'Setting response from exception hook' );
+};
+
+# A more advanced test that checks that an exception can take place within the
+# exception hook, but only if it sets the response content and performs a halt
+subtest 'hook_exception_double' => sub {
+    my $test = Plack::Test->create( App::HookExceptionDouble->to_app );
+    my $resp = $test->request( GET '/hook_exception_double');
+    is( $resp->content, 'Setting response from exception hook and then dying' );
+};
+
+# Similar to the above, but without any handling for the second exception which
+# would otherwise result in a recursive situation
+subtest 'hook_exception_recursive' => sub {
+    my $test = Plack::Test->create( App::HookExceptionRecursive->to_app );
+    my $resp = $test->request( GET '/hook_exception_recursive');
+    like( $resp->content, qr/Internal Server Error/ );
+};
+
 done_testing;