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

From: Jakub Narebski <jnareb@gmail.com>
Date: Dec 25, 2010, 23:17 UTC
Message-ID: <201012260017.56306.jnareb@gmail.com>
In-Reply-To: <20101223020801.GB14585@burratino>

On Thu, 23 Dec 2010, Jonathan Nieder wrote:

Show 8 quoted lines

> Jakub Narebski wrote:
> 
> > Gitweb assumes here that exceptions thrown by Perl would be simple
> > strings; die_error() throws hash reference (if not for minimal
> > external dependencies, it would be probable object of Class::Exception
> > or Throwable class thrown).
> 
> Hmm, why not throw an object of new type Gitweb::Exception?

First, 'gitweb: Prepare for splitting gitweb' commit is only later in series... ;-) but that of course is not a serious issue.

Second, more important is that I'd rather gitweb doesn't go "reinvent the wheel" route. I'd rather (re)use Exception::Class (like e.g. SVN::Web does it) if we go the OO exception handling route.

But if we are going to use Exception::Class, then we can also use Try::Tiny, I think.

Show 23 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!

Yeah, that is very nice.

Show 10 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($@)) {

Ooops, it should be 'if ($@ ...)', not 'if (defined $@ ...)'.

Show 6 quoted lines

> > +			# 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.

You meant Scalar::Util::blessed here, isn't it? Fortunately Scalar::Util is core Perl module.

By 'overzealous' do you mean here possibility of catching what we shouldn't, i.e. non-gitweb error (not thrown by die_error)? We can narrow it to "ref($@) eq 'HASH'", but I don't think it would be ever necessary: Perl throws string exceptions.

Show 13 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?

No it isn't.

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

Now I should check if this doesn't affect gitweb performance too badly. IIRC I have chosen 'goto DONE_GITWEB' because I didn't know about ModPerl::Registry redefining 'exit' (why it was done), and because of some microbenchmark showing that it performs better than die/eval (why this specific solution)...

But I think that the performance hit would be negligible in practice; making gitweb more maintainable is I think worth the cost.

-- 
Jakub Narebski
Poland