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

From: Jakub Narebski <jnareb@gmail.com>
Date: Dec 25, 2010, 22:14 UTC
Message-ID: <201012252314.22541.jnareb@gmail.com>
In-Reply-To: <20101223015540.GA14585@burratino>

On Thu, 23 Dec 2010, Jonathan Nieder wrote:

Show 19 quoted lines

> Jakub Narebski wrote:
> 
> > End the request after die_error finishes, rather than exiting gitweb
> > instance
> [...]
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -1169,6 +1169,7 @@ sub run {
> >  
> >  		run_request();
> >  
> > +	DONE_REQUEST:
> >  		$post_dispatch_hook->()
> >  			if $post_dispatch_hook;
> >  		$first_request = 0;
> > @@ -3767,7 +3768,7 @@ EOF
> 
> [side note: the "@@ EOF" line above would say "@@ sub die_error {" if
> userdiff.c had perl support and gitattributes used it.]

Hmmm, I thought that git has Perl-specific diff driver (xfuncname), but I see that it doesn't. The default funcname works quite well for Perl code... with exception of here-documents (or rather their ending).

BTW. do you know how such perl support should look like?

Show 9 quoted lines

> >  	print "</div>\n";
> >  
> >  	git_footer_html();
> > -	goto DONE_GITWEB
> > +	goto DONE_REQUEST
> >  		unless ($opts{'-error_handler'});
> 
> This seems to remove the last user of the DONE_GITWEB label.  Why not
> delete the label, too?

Well, actually this patch is in this series only for the label ;-)

Anyway, I can simply drop this patch, and have next one in series (adding exception-based error handling, making die_error work like 'die') delete DONE_GITWEB label...

> When die_error is called by CGI::Carp (via handle_errors_html), it
> does not rearm the error handler afaict.  Previously that did not
> matter because die_error kills gitweb; now should it be set up
> again?

Thanks, I missed this (but after examining it turns out to be a non-issue). That will teach me to leave code outside of run() subroutine; one of reasons behind creating c2394fe (gitweb: Put all per-connection code in run() subroutine, 2010-05-07) was to clarify code flow.

A note: using set_message inside handle_errors_html was necessary because if there was a fatal error in die_error, then handle_errors_html would be called recursively - this was fixed in CGI.pm 3.45, but we cannot rely on this; we cannot rely on having new enough version of CGI::Carp that supports set_die_handler either.

But actually handle_errors_html gets called only from fatalsToBrowser, which in turn gets called from CGI::Carp::die... which ends calling CODE::die (aka realdie), which ends CGI process anyway.

That is why die_error ends with

	goto DONE_GITWEB
		unless ($opts{'-error_handler'});

i.e. it doesn't goto DONE_GITWEB nor DONE_REQUEST if called from handle_errors_html anyway.

> die_error gets called when server load is too high; I wonder whether
> it is right to go back for another request in that case.

If client (web browser) are requesting connection, we have to tell it something anyway. Note that each request might serve different client. But when the die_error(503, "The load average on the server is too high") doesn't generate load by itself, all should be all right.

Show 6 quoted lines

> 
> A broken per-request (or other) configuration could potentially leave
> a gitweb process in a broken state, and until now the state would be
> reset on the first error.  I wonder if escape valve would be needed
> --- e.g., does the CGI harness take care of starting a new gitweb
> process after every couple hundred requests or so?

'die $@ if $@' would call CORE::die, which means it would end gitweb process.

For CGI server it doesn't matter anyway, as for each request the process
is respawned anyway (together with respawning Perl interpreter), and I
think that ModPerl::Registry and FastCGI servers monitor process that it
is to serve requests, and respawn it if/when it dies.

> Aside from those (minor) worries, this patch seems like a good idea.

 
Thanks a lot for your comments.

-- 
Jakub Narebski
Poland