Re: mod_fcgid: Immediate HTTP error 503 if the max total process count is reached
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
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
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
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
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
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&name=maxnowait * The whole patch as a single file: https://github.com/famzah/mod_fcgid/compare/2.3.9...maxnowait.patch?diff=unified&name=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