Re: [PATCH 1/6] GITWEB - Load Checking

[Jakub Narebski <jnareb@gmail.com>]

On Fri, 11 Dec 2009, J.H. wrote:
> <snip>
>>> adds $maxload configuration variable.  Default is a load of 300,
>>> which for most cases should never be hit.
>> 
>> Your patch doesn't allow for *turning off* this feature.  Reasonable
>> solution would be to use 'undef' or negative number to turn off this
>> check (this feature).
> 
> Well there's the opposite argument that setting the number arbitrarily 
> high, 4096 for instance would also in essence negate this (though I'll 
> admit I've reached and exceeded those numbers before)
> 
> That said I agree, being able to turn this off needs to be added and 
> will be shortly.

Simplest solution would be to used 'undef' (undefined value) for 
"turned off", i.e.:

  if (defined $maxload && get_loadavg() > $maxload) {

>>> Please note this makes the assumption that /proc/loadavg exists
>>> as there is no good way to read load averages on a great number of
>>> platforms [READ: Windows], or that it's reasonably accurate.
>> 
>> What about MacOS X, or FreeBSD, or OpenSolaris?
> 
> Will comment on this further down

I think it would be better to write in commit message that because finding
load average is OS dependent, there is provided (sample) solution which
uses /proc/loadavg and works (at least) on Linux.  And that for platforms
which do not have /proc/loadavg the feature is simply turned off (by the
way of using load=0 if load cannot be determined).
 
>> You should mention that it is intended that if gitweb cannot read load
>> average (for example /proc/loadavg does not exist), then the feature
>> is turned off, i.e. the check always succeeds.  Which is reasonable.
> 
> That's fine.

See above proposal.  This information should be present in commit message,
and perhaps maybe even as one-line comment above opening /proc/loadavg.

>>> +# loadavg throttle
>>> +sub get_loadavg() {
>>> +    my $load;
>>> +    my @loads;
>>> +
>>> +    open($load, '<', '/proc/loadavg') or return 0;
>> 
>> Why not use one of existing CPAN modules: Sys::Info::Device::CPU,
>> BSD::getloadavg, Sys::CpuLoad?
> 
> Here's the fundamental problem:
> 
> Sys:Info:Device:CPU
> 	Windows:
> 		Using this method under Windows is not recommended
> 		since, the WMI interface will possibly take at least 2
> 		seconds to complete the request.
> 
> BSD::getloadavg
> 	While this more or less supports anything with a libc getloadavg
> 	(and thus might be the best one I've seen, I'll admit I didn't
> 	notice this one when I looked years ago) getting it to work on
> 	windows looks, exciting.
> 
> Sys::CpuLoad:
> 	http://cpansearch.perl.org/src/CLINTDW/Sys-CpuLoad-0.03/README
> 	Specifically:
> 		- Currently FreeBSD and OpenBSD are supported.
> 		- Wanted: HPUX 11.11 ...
> 		- Todo: Win32 support
> 
> 	So this doesn't really buy me anything but, maybe, BSD support.
> 	
> So at the end of the day, none of those really gets me a "useful" cross 
> platform load checker (though like I said BSD::getloadavg looks to be 
> the best of the ones you mentioned) and more or less Windows is going to 
> lose this as a usable feature no matter what.
> 
> I think I'd almost rather set this up so that if it can't get something 
> useful (I.E. /proc/loadavg is missing) it just skips past it as if the 
> load was 0.
> 
> I might try out the BSD::getloadavg but I want to take a look and see if 
> that's easily installed or not, if it's not it might be difficult to 
> justify that as a dependency.

After thinking about this a bit, now I don't think that it is terribly
important.  You *might* describe alternate approaches (roads not taken)
in commit message, but requiring /proc/loadavg for the feature to work
is fine for first patch (it makes patch simpler).

>>> +if (get_loadavg()> $maxload) {
>>> +    print "Content-Type: text/plain\n";
>>> +    print "Status: 503 Excessive load on server\n";
>>> +    print "\n";
>>> +    print "The load average on the server is too high\n";
>>> +    exit 0;
>> 
>> Why not use die_error subroutine?  Is it to have generate absolutely
>> minimal load, and that is why you do not use die_error(), or even
>> $cgi->header()?
>> 
>> Wouldn't a better solution be to use here-doc syntax?
>> 
>> +    print <<'EOF';
>> +Content-Type: text/plain; charset=utf-8
>> +Status: 503 Excessive load on server
>> +
>> +The load average on the server is too high
>> +EOF
>> +    exit 0;
> 
> It was intended to be the most minimal possible, mainly get in, get out. 
>
>   Also not sure the die_error existed in gitweb when this was originally 
> written.  Probably worth switching to it now since it's there either 
> way, and I don't think using it would add enough overhead to matter.

Well, if you are not worring excessively about overhead, then I think
using die_error would be the best solution, as it would preserve look
of gitweb.  It would require extending die_error by 503 response, or
rather %http_responses hash and comment above die_error.

Also I think that Status: should be before Content-Type: header (but
probably it is not required by the standard).

-- 
Jakub Narebski
Poland