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

[Jakub Narebski <jnareb@gmail.com>]

On Thu, 23 Dec 2010, Jonathan Nieder wrote:
> 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?

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

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