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

[Ivan Zahariev]

Hi,

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


On 20.9.2016 г. 16:33 ч., 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]

On Mon, Sep 19, 2016 at 4:30 AM, Ivan Zahariev  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.

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

[Ivan Zahariev]

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 г. 11:57 ч., 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 г. 13:45 ч., 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]

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 г. 13:45 ч., 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

[Nick Kew]

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.

-- 
Nick Kew

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

[Ivan Zahariev]

Hello,

I got no feedback. Am I posting this suggestion at the right mailing list?

Best regards.
--Ivan

On 19.5.2016 г. 10:40 ч., Ivan Zahariev wrote:

Hi all,

I'd like to propose a new configuration setting for "mod_fcgid". The 
source code changes to review follow:


  * The whole patch compared to version 2.3.9:

https://github.com/famzah/mod_fcgid/compare/2.3.9...maxnowait?diff=split=maxnowait
  * The whole patch as a single file:

https://github.com/famzah/mod_fcgid/compare/2.3.9...maxnowait.patch?diff=unified=maxnowait
  * Every single commit compared to version 2.3.9:
https://github.com/famzah/mod_fcgid/commits/maxnowait
  * There should be no merge conflicts with the current "trunk"
version 2.3.10.


The motivation is that the current behavior to queue up new pending 
requests differs from the RLimitNPROC directive behavior. When there 
is a spike in the web hits, lots of Apache children get busy just 
waiting for up to 30+ seconds to get an idle FastCGI process. Thus we 
"waste" Apache children doing nothing while they could serve static 
content. This also puts unneeded memory pressure. Additionally, users 
today won't wait for a page to load, if it takes more than a few 
seconds, and we'd rather notify them that we are currently overloaded 
by sending them a 503 HTTP response immediately.


Here is the documentation for the new directive: 
http://www.famzah.net/temp/FcgidMaxProcessesUsedNoWait.docs.txt


Let me know what you think.

Best regards.
--Ivan