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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 26, 2010, 09:50 UTC
Message-ID: <20101226095054.GB21588@burratino>
In-Reply-To: <201012252314.22541.jnareb@gmail.com>

Jakub Narebski wrote:

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

Show 8 quoted lines

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

Show 5 quoted lines

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

[...]

Show 11 quoted lines

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