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

[Jakub Narebski <jnareb@gmail.com>]

On Sun, 26 Dec 2010, Jonathan Nieder wrote:
> Jakub Narebski wrote:
>> On Thu, 23 Dec 2010, Jonathan Nieder wrote:
 
>>> 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].

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

I assume that CGI / FastCGI / mod_perl (+ ModPerl::Registry) web server
would know how to regulate number of workers according to the server
load.
 
>>> 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?

I'm sorry I haven't made myself clear.

What I meant here is that gitweb includes the following code

	if (-e $GITWEB_CONFIG) {
		do $GITWEB_CONFIG;
		die $@ if $@;
	}

which means that CGI::Carp::die is called, which might call 
handle_errors_html, and which ends in CORE::die, which ends gitweb
process.  So if there is no way for broken configuration to leave
gitweb in a rboken state _at this point in series_.

Thank you for thinking about this, because it could cause problems
(could because I have not checked if it does or if it doesn't) in the
following patch, when gitweb uses eval / die for error handling.
Then it might happen when $per_request_config is false or CODE that
instead of trying to reread broken config on subsequent requests, we
will run with broken config.  It depends if "die"-ing in 
evaluate_gitweb_config would prevent setting $first_request to false.
I'd have to check that.

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

See above.

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

Which should make it in either commit message, or comments, I guess.
 
> 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>

Thanks.

-- 
Jakub Narebski
Poland