[RFC PATCH 0/2] gitweb: die_error (error handling) improvements

[RFC PATCH 0/2] gitweb: die_error (error handling) improvements

[Jakub Narebski]

The following two patch series changes improve error / exception
handling in gitweb, preparing the way for gitweb output caching, but
useful even without it.

I'm sending this patch series early to gather feedback on possible
ways of improving error / exception handling in gitweb.


Shortlog:
~~~~~~~~~
Jakub Narebski (2):
      gitweb: use eval + die for error (exception) handling
      gitweb: Go to DONE_REQUEST rather than DONE_GITWEB in die_error

Diffstat:
~~~~~~~~~
 gitweb/gitweb.perl |   27 +++++++++------------------
 1 files changed, 9 insertions(+), 18 deletions(-)

-- 
Jakub Narebski
ShadeHawk on #git
Poland

[RFC PATCH 1/2] gitweb: Go to DONE_REQUEST rather than DONE_GITWEB in die_error

[Jakub Narebski]

End the request after die_error finishes, rather than exiting gitweb
instance (perhaps wrapped like in ModPerl::Registry or gitweb.psgi
case).

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch was sent to git mailing list as a standalone RFC patch some
time ago.  This version doesn't change anything from previous version.

I am keeping this patch (even though it is not strictly necessary), to
have DONE_REQUEST label, which I think can be quite useful, even if
die_error wouldn't be using it starting from the following commit.

 gitweb/gitweb.perl |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index cfa511c..af45daa 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1147,6 +1147,7 @@ sub run {
 
 		run_request();
 
+	DONE_REQUEST:
 		$post_dispatch_hook->()
 			if $post_dispatch_hook;
 
@@ -3669,7 +3670,7 @@ EOF
 	print "</div>\n";
 
 	git_footer_html();
-	goto DONE_GITWEB
+	goto DONE_REQUEST
 		unless ($opts{'-error_handler'});
 }
 

[RFC PATCH 2/2] gitweb: use eval + die for error (exception) handling

[Jakub Narebski]

In gitweb code it is assumed that calling die_error() subroutine would
end request, just like running "die" would.  Up till now it was done by
having die_error() jump to DONE_REQUEST (earlier DONE_GITWEB), or in
earlier version just 'exit' (for mod_perl via ModPerl::Registry it ends
request instead of exiting worker).

Instead of using 'goto DONE_REQUEST' for longjmp-like nonlocal jump, or
using 'exit', gitweb uses now native for Perl exception handlingin the
form of eval / die pair ("eval BLOCK" to trap exceptions, "die LIST" to
raise/throw them).

Up till now the "goto DONE_REQUEST" solution was enough, but with the
coming output caching support and it adding modular structure to gitweb,
it would be difficult to continue to keep using this solution
(e.g. interaction with capturing output).


Because gitweb now traps all exceptions occuring run_request(), the
handle_errors_html handler (set via set_message from CGI::Carp) is not
needed; gitweb can call die_error in -error_handler mode itself.  This
has the advantage that we can now set correct HTTP header (handler from
CGI::Carp::set_message was run after HTTP headers were already sent).

Gitweb assumes here that exceptions thrown by Perl would be simple
strings; die_error() throws hash reference (if not for minimal
extrenal dependencies, it would be probable object of Class::Exception
or Throwable class thrown).

Note: in newer versions of CGI::Carp there is set_die_handler(), where
handler have to set HTTP headers to the browser itself, but we cannot
rely on new enough CGI::Carp version to have been installed.  Also
set_die_handler interferes with fatalsToBrowser.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is main part of this series.

Comments?

 gitweb/gitweb.perl |   26 ++++++++------------------
 1 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index af45daa..ab85c53 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -20,7 +20,7 @@ use lib __DIR__ . '/lib';
 
 use CGI qw(:standard :escapeHTML -nosticky);
 use CGI::Util qw(unescape);
-use CGI::Carp qw(fatalsToBrowser set_message);
+use CGI::Carp qw(fatalsToBrowser);
 use Encode;
 use Fcntl ':mode';
 use File::Find qw();
@@ -1034,21 +1034,6 @@ sub configure_gitweb_features {
 	}
 }
 
-# custom error handler: 'die <message>' is Internal Server Error
-sub handle_errors_html {
-	my $msg = shift; # it is already HTML escaped
-
-	# to avoid infinite loop where error occurs in die_error,
-	# change handler to default handler, disabling handle_errors_html
-	set_message("Error occured when inside die_error:\n$msg");
-
-	# you cannot jump out of die_error when called as error handler;
-	# the subroutine set via CGI::Carp::set_message is called _after_
-	# HTTP headers are already written, so it cannot write them itself
-	die_error(undef, undef, $msg, -error_handler => 1, -no_http_header => 1);
-}
-set_message(\&handle_errors_html);
-
 # dispatch
 sub dispatch {
 	if (!defined $action) {
@@ -1145,7 +1130,11 @@ sub run {
 		$pre_dispatch_hook->()
 			if $pre_dispatch_hook;
 
-		run_request();
+		eval { run_request() };
+		if (defined $@ && !ref($@)) {
+			# some Perl error, but not one thrown by die_error
+			die_error(undef, undef, $@, -error_handler => 1);
+		}
 
 	DONE_REQUEST:
 		$post_dispatch_hook->()
@@ -3670,7 +3659,8 @@ EOF
 	print "</div>\n";
 
 	git_footer_html();
-	goto DONE_REQUEST
+
+	die {'status' => $status, 'error' => $error}
 		unless ($opts{'-error_handler'});
 }
 

Re: [RFC PATCH 0/2] gitweb: die_error (error handling) improvements

[J.H.]

On 12/12/2010 04:46 PM, Jakub Narebski wrote:
> The following two patch series changes improve error / exception
> handling in gitweb, preparing the way for gitweb output caching, but
> useful even without it.
> 
> I'm sending this patch series early to gather feedback on possible
> ways of improving error / exception handling in gitweb.

Personally, instead of another band-aid over this problem, and adding
(or further legitimizing) goto statements inside gitweb I'd much *MUCH*
rather we actually put in the work to actually clean this up.

This is the direction I'm heading in, which I mentioned in an earlier
e-mail.

There are a *LOT* of disadvantages to the eval mechanism in perl.  It's
the standard but gitweb is getting more and more complex, and eval is
simplistic.  Couple that with the complexity and uncertainty that things
like goto add to the code, I would *MUCH* rather not see this series go
in, as I think it is the wrong approach to fixing this.

- John 'Warthog9' Hawley

Re: [RFC PATCH 0/2] gitweb: die_error (error handling) improvements

[Jakub Narebski]

On Mon, 13 Dec 2010, J.H. wrote:
> On 12/12/2010 04:46 PM, Jakub Narebski wrote:

> > The following two patch series changes improve error / exception
> > handling in gitweb, preparing the way for gitweb output caching, but
> > useful even without it.
> > 
> > I'm sending this patch series early to gather feedback on possible
> > ways of improving error / exception handling in gitweb.
> 
> Personally, instead of another band-aid over this problem, and adding
> (or further legitimizing) goto statements inside gitweb I'd much *MUCH*
> rather we actually put in the work to actually clean this up.

That's not band-aid, that's using Perl exception mechanism.  Gitweb
uses die_error() like one would ordinarily use "die".
 
> This is the direction I'm heading in, which I mentioned in an earlier
> e-mail.

Well, then how do you want to handle errors?  Note that die_error calls
are sometimes nested quite deep in the call stack, so using return value
to denote errors and checking it is rather out of question: it would
significantly increase complexity of code for no gain.

Nevertheless I'll take a look how it is solved in other web applications
written in Perl, like SVN::Web or CPAN Hubble.
 
> There are a *LOT* of disadvantages to the eval mechanism in perl.  It's
> the standard but gitweb is getting more and more complex, and eval is
> simplistic.  Couple that with the complexity and uncertainty that things
> like goto add to the code, I would *MUCH* rather not see this series go
> in, as I think it is the wrong approach to fixing this.

eval / die is not like goto, but like exception mechanism in other
languages.  I'd prefer to use Try::Tiny or TryCatch, but we have this
"no extra dependencies" policy for gitweb.
-- 
Jakub Narebski
Poland

Re: [RFC PATCH 0/2] gitweb: die_error (error handling) improvements

[Jakub Narebski]

On Mon, 13 Dec 2010, Jakub Narebski wrote:

> Nevertheless I'll take a look how it is solved in other web applications
> written in Perl, like SVN::Web or CPAN Hubble.

SVN::Web uses eval / die... well, it uses SVN::Web::X->throw() instead
of "die" for error handling (SVN::Web::X is based on Exception::Class;
other class to use for exceprion handling is Throwable but it requires
Moose).  For errors early in the process it just uses "die".

-- 
Jakub Narebski
Poland