Re: mod_fcgid: Immediate HTTP error 503 if the max total process count is reached

[Ivan Zahariev <fa...@famzah.net>]

Hello devs,

It's been four months since I originally proposed this new feature in 
"mod_fcgid". During this time I've tested and deployed it on hundreds of 
busy production servers. It works as expected. If enabled, web visitors 
get an immediate response when FastCGI is overloaded, and no RAM is 
over-utilized.

You can find all the information and a patch in the enhancement request 
at Bugzilla: https://bz.apache.org/bugzilla/show_bug.cgi?id=59656

Can we get this merged into "mod_fcgid"?

Best regards.
--Ivan


On 2.6.2016 \u0433. 11:57 \u0447., Ivan Zahariev wrote:
> Hi Nick,
>
> Thanks for the info.
>
> I've followed your instructions and submitted an enhancement request: 
> https://bz.apache.org/bugzilla/show_bug.cgi?id=59656
>
> Cheers.
> --Ivan
>
>
> On 31.5.2016 \u0433. 13:45 \u0447., Nick Kew wrote:
>> On Tue, 2016-05-31 at 11:15 +0300, Ivan Zahariev wrote:
>>> Hello,
>>>
>>> I got no feedback. Am I posting this suggestion at the right mailing
>>> list?
>> Sorry, I see your original post marked for attention in my mail
>> folder, but languishing hitherto unattended.  Just now opened your
>> link in a browser to take a look.  There could be others who
>> have done something similar.
>>
>> As a general reply to this question, yes, this is the best
>> available mailinglist.  The other place to post it would be
>> as an enhancement request in bugzilla (issues.apache.org).
>> The keyword "PatchAvailable" there may help by marking it as
>> low-hanging fruit.
>>
>

Re: mod_fcgid: Immediate HTTP error 503 if the max total process count is reached

[Ivan Zahariev <fa...@famzah.net>]

Hi,

Thanks for the review. You can find my comments below in the reply.


On 20.9.2016 \u0433. 16:33 \u0447., Eric Covener wrote:
> Unfortunately there are not many reviewers for mod_fcgid.
> I tried to take as a relative laymen and had a few comments/questions:
>
> * a few C99 // comments were added and should be zapped
Fixed.

> * the assert looks funny -- no assert.   maybe ap_debug_assert() so it
> blows up in maintainer builds only?
It's a paranoid check. If it ever blows the whistle, this will be in 
some very hard to reproduce case, and I don't expect that we catch this 
on a maintainer build. I made this an APLOG_CRIT message because a 
mismatch in the counts between proctable_count_all_nonfree_nodes() and 
"g_total_process" could make your FastCGI requests fail erroneously with 
an immediate 503 HTTP error. A System Administrator who could encounter 
this bug will appreciate the immediate APLOG_CRIT message in the 
"error_log".

I don't insist this to remain like this and I will change it as you say, 
in order to comply with the usual development practices. Or we can give 
this function check a better name.

> * The new flag is merged, but cannot be unset?
> ** Probably need to make it look like an AP_INIT_FLAG
Right. I've converted this to an AP_INIT_FLAG which makes more sense.

I don't understand what you mean by "it cannot be unset". I'm merging it 
in the same way as other mod_fcgid config options. Please read below for 
more information regarding this matter.

> * I couldn't follow the concept in the comments around
> max_process_count moving or being per-request. Is it maybe out of
> date? I think the current impl takes an existing global only and
> merges it with vhost server confs but it will always be a copy.

The original implementation allows you to set "max_process_count" in the 
GLOBAL_ONLY context. This is enforced by ap_check_cmd_context() in the 
AP_INIT_TAKE1() handler, because RSRC_CONF would otherwise allow us to 
define this config option also "per vhost".

I've added the new variable "max_process_used_no_wait_enable" in the 
same way.

The original implementation works well without further magic, because it 
really uses "max_process_count" in code which works with the 
"main_server" (global) context. Now I need to access the value of 
"max_process_count" inside a "request_rec" (per VirtualHost) variable. 
If one or more mod_fcgid directives are defined not only within the main 
server config, but also as additional settings in a VirtualHost context, 
then we need to merge "max_process_count", too. If we don't merge, the 
value of "max_process_count" returned by the "request_rec" variable will 
be the default init value (zero, in our case).

Long story short -- you are right, this is a global only variable and it 
merges with vhost confs always as a copy of the global value. This is 
just a behind-the-scenes action and doesn't affect Apache 
administrators, since they can't define this variable in a vhost context 
anyway. I tried to summarize this as the comment ["global only" but we 
use its main_server value in a "request_rec"] but maybe someone can 
propose a better text.

I've committed the source code changes and you can review them again. 
When we're satisfied with the changes, I'll test the final version on 
our server fleet.

Best regards.
--Ivan

Re: mod_fcgid: Immediate HTTP error 503 if the max total process count is reached

[Eric Covener <co...@gmail.com>]

On Mon, Sep 19, 2016 at 4:30 AM, Ivan Zahariev <fa...@famzah.net> wrote:
> Hello devs,
>
> It's been four months since I originally proposed this new feature in
> "mod_fcgid". During this time I've tested and deployed it on hundreds of
> busy production servers. It works as expected. If enabled, web visitors get
> an immediate response when FastCGI is overloaded, and no RAM is
> over-utilized.
>
> You can find all the information and a patch in the enhancement request at
> Bugzilla: https://bz.apache.org/bugzilla/show_bug.cgi?id=59656
>
> Can we get this merged into "mod_fcgid"?
>
> Best regards.
> --Ivan

Unfortunately there are not many reviewers for mod_fcgid.

I tried to take as a relative laymen and had a few comments/questions:

* a few C99 // comments were added and should be zapped
* the assert looks funny -- no assert.   maybe ap_debug_assert() so it
blows up in maintainer builds only?
* The new flag is merged, but cannot be unset?
** Probably need to make it look like an AP_INIT_FLAG
* I couldn't follow the concept in the comments around
max_process_count moving or being per-request. Is it maybe out of
date? I think the current impl takes an existing global only and
merges it with vhost server confs but it will always be a copy.