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

[Jonathan Nieder <jrnieder@gmail.com>]

Jakub Narebski wrote:
> On Thu, 23 Dec 2010, Jonathan Nieder wrote:

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

I like the current order (first the brief patch to change the
semantics, then the more ambitious change to an eval {} based error
handling implementation), but it doesn't matter so much.

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

Right, I should have thought a few seconds more.  Respawning
gitweb.perl would generate _more_ load[1].

>> A broken per-request (or other) configuration could potentially leave
>> a gitweb process in a broken state,
[...]
> 'die $@ if $@' would call CORE::die, which means it would end gitweb
> process.

This is referring to a later patch?

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

Sorry, that was unclear of me.  I meant that buggy configuration could
leave a gitweb process in buggy but alive state and frequent failing
requests might be a way to notice that.  Contrived example (just to
illustrate what I mean):

	our $version .= ".custom";
	if (length $version >= 1000) {	# untested, buggy code goes here.
		@diff_opts = ("--nonsense");
	}

I think I was not right to worry about this, either.  It is better to
make such unusual and buggy configurations as noticeable as possible
so they can be fixed.

[...]
> 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.
[...]
> Thanks a lot for your comments.

Thanks for a thorough explanation.  For what it's worth, with or
without removal of the DONE_GITWEB: label,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[1] I can imagine scenarios in which exiting gitweb would help
alleviate the load, involving:

 - large memory footprint for each gitweb process forcing the system
   into swapping (e.g., from a memory leak), or
 - FastCGI-like server noticing the load and choosing to decrease the
   number of gitweb instances.

In the usual case, presumably gitweb memory footprint is small and
FastCGI-like servers limit the number of gitweb instances to a modest
fixed number.