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
