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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 23, 2010, 02:08 UTC
Message-ID: <20101223020801.GB14585@burratino>
In-Reply-To: <20101222235525.7998.99816.stgit@localhost.localdomain>

Jakub Narebski wrote:

> 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).

Hmm, why not throw an object of new type Gitweb::Exception?

Show 21 quoted lines

> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1045,21 +1045,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);
> -

Hoorah!

Show 13 quoted lines

>  # dispatch
>  sub dispatch {
>  	if (!defined $action) {
> @@ -1167,7 +1152,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);
> +		}

The !ref($@) seems overzealous, which is why I am wondering if it would be possible to use bless() for a finer-grained check.

Show 11 quoted lines

>  
>  	DONE_REQUEST:
>  		$post_dispatch_hook->()
> @@ -3768,7 +3757,8 @@ EOF
>  	print "</div>\n";
>  
>  	git_footer_html();
> -	goto DONE_REQUEST
> +
> +	die {'status' => $status, 'error' => $error}
>  		unless ($opts{'-error_handler'});

Is the DONE_REQUEST label still needed?

Thanks, I am happy to see the semantics becoming less thorny. Jonathan