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@xxxxxxxxx>
Thanks.
--
Jakub Narebski
Poland
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html