Re: cvs commit: httpd-2.0/server/mpm/winnt child.c
Jeff Trawick wrote: possible heresy The first parameter to select() on Windows is actually ignored. We should zap the logic that does the Unix-style computation of listenmaxfd and just pass in INVALID_SOCKET or 0, and add a comment that the value is ignored. /possible heresy No heresy, MSDN doc clearly says the first parm is ignored so it doesn't matter what we set it to. For clarification I can add a comment and set it to zero. Thanks, Allan
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
Bill Stoddard wrote: Do you know of any cases that actually require mpm_state to be updated in ap_signal_parent()? Setting winnt_mpm_state to AP_MPMQ_STOPPING in child main should be sufficient unless I am missing something. the code in service.c which shuts down the server for other reasons? AFAICT, not all code in child main that leads to termination will call ap_signal_parent(), and not all calls to ap_signal_parent() come through child_main but... where will module code be called (pool cleanups, other child maintenance routines)? the desire here is for any such code to see AP_MPMQ_STOPPING... based on this, maybe it can be set in fewer places or some common place (btw, thanks for having a look at this!)
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
Jeff Trawick wrote:
Bill Stoddard wrote:
Do you know of any cases that actually require mpm_state to be updated
in ap_signal_parent()? Setting winnt_mpm_state to AP_MPMQ_STOPPING in
child main should be sufficient unless I am missing something.
the code in service.c which shuts down the server for other reasons?
AFAICT, not all code in child main that leads to termination will call
ap_signal_parent(), and not all calls to ap_signal_parent() come through
child_main
but... where will module code be called (pool cleanups, other child
maintenance routines)? the desire here is for any such code to see
AP_MPMQ_STOPPING... based on this, maybe it can be set in fewer places
or some common place
(btw, thanks for having a look at this!)
Humm no simple answers. Thinking out loud.
All code that runs in the child process will enter/exit via child main which is called on the ap_mpm_run hook.
So setting state to STARTING at process initialization time is good. Setting state to RUNNING then STOPPING
in child_main is good. This covers all cases in the child process. So child processes do not require the
changes to ap_signal_parent(). Agree?
The module architecture is still a bit crufty on Windows because the parent process also calls all the module
init/cleanup hooks (becase Windows does not support Unix fork() semantics). Most modules should simply get out
of the way if they detect they are being called in the parent process (and not being run in ONE_PROCESS mode)
but there can be exceptions. Modules may need to maintain shared resources (e.g. shared memory segment) across
graceful restarts. Or someday when we enable multiple active child processes, modules may need to share state
between child processes.
Checkout this patch. I -know- state AP_MPMQ_RESTARTING does not exist in ap_mpm.h (so this patch will not
compile) but consider it for purposes of discussion. The parent process always goes through master_main() and
master_main() can distinguish between stopping and graceful restarting and this would be useful information
for a Windows module in the parent process for purposes discussed above.
Index: mpm_winnt.c
===
RCS file: /home/cvs/httpd-2.0/server/mpm/winnt/mpm_winnt.c,v
retrieving revision 1.305
diff -u -r1.305 mpm_winnt.c
--- mpm_winnt.c 9 Jan 2004 20:41:06 - 1.305
+++ mpm_winnt.c 14 Jan 2004 14:18:44 -
@@ -307,7 +307,6 @@
switch(type) {
case SIGNAL_PARENT_SHUTDOWN:
{
- winnt_mpm_state = AP_MPMQ_STOPPING;
SetEvent(shutdown_event);
break;
}
@@ -315,7 +314,6 @@
case SIGNAL_PARENT_RESTART:
case SIGNAL_PARENT_RESTART_GRACEFUL:
{
- winnt_mpm_state = AP_MPMQ_STOPPING;
is_graceful = 1;
SetEvent(restart_event);
break;
@@ -327,7 +325,6 @@
switch(type) {
case SIGNAL_PARENT_SHUTDOWN:
{
- winnt_mpm_state = AP_MPMQ_STOPPING;
signal_name = signal_shutdown_name;
break;
}
@@ -335,7 +332,6 @@
case SIGNAL_PARENT_RESTART:
case SIGNAL_PARENT_RESTART_GRACEFUL:
{
- winnt_mpm_state = AP_MPMQ_STOPPING;
signal_name = signal_restart_name;
is_graceful = 1;
break;
@@ -934,6 +930,7 @@
ap_scoreboard_image-parent[0].pid = child_pid;
/* Wait for shutdown or restart events or for child death */
+winnt_mpm_state = AP_MPMQ_RUNNING;
rv = WaitForMultipleObjects(NUM_WAIT_HANDLES, (HANDLE *) event_handles, FALSE,
INFINITE);
cld = rv - WAIT_OBJECT_0;
if (rv == WAIT_FAILED) {
@@ -1013,6 +1010,7 @@
if (shutdown_pending)
{
int timeout = 3; /* Timeout is milliseconds */
+winnt_mpm_state = AP_MPMQ_STOPPING;
/* This shutdown is only marginally graceful. We will give the
* child a bit of time to exit gracefully. If the time expires,
@@ -1044,7 +1042,7 @@
}
return 0; /* Tell the caller we do not want to restart */
}
-
+winnt_mpm_state = AP_MPMQ_RESTARTING;
return 1; /* Tell the caller we want a restart */
}
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
Bill Stoddard wrote: Checkout this patch. I -know- state AP_MPMQ_RESTARTING does not exist in ap_mpm.h (so this patch will not compile) but consider it for purposes of discussion. The parent process always goes through master_main() and master_main() can distinguish between stopping and graceful restarting and this would be useful information for a Windows module in the parent process for purposes discussed above. a comment on just the AP_MPMQ_RESTARTING idea: Unix code can probably distinguish between restarting or not too, but the purpose of simply reporting that we're stopping is that most code only cares whether or not this MPM process is exiting, and if we only report the more granular information, we've created extra work for most modules and made it more error prone to check. The more granular information should be reported by a separate query call which can distinguish between stopping because the entire server is shutting down completely stopping, but just this process (idle or hit MaxRequestsPerChild) stopping gracefully during restart procedure stopping ungracefully during restart procedure
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
Do you know of any cases that actually require mpm_state to be updated in ap_signal_parent()? Setting
winnt_mpm_state to AP_MPMQ_STOPPING in child main should be sufficient unless I am missing something.
Bill
[EMAIL PROTECTED] wrote:
trawick 2003/12/16 18:16:44
Modified:server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
Log:
add support for querying MPM state to ap_mpm_query() for the WinNT MPM
Revision ChangesPath
1.21 +3 -1 httpd-2.0/server/mpm/winnt/child.c
...
Index: mpm_winnt.c
===
RCS file: /home/cvs/httpd-2.0/server/mpm/winnt/mpm_winnt.c,v
retrieving revision 1.302
retrieving revision 1.303
diff -u -r1.302 -r1.303
--- mpm_winnt.c 15 Dec 2003 23:19:14 - 1.302
+++ mpm_winnt.c 17 Dec 2003 02:16:44 - 1.303
@@ -107,6 +107,7 @@
static int thread_limit = DEFAULT_THREAD_LIMIT;
static int first_thread_limit = 0;
static int changed_limit_at_restart;
+int winnt_mpm_state = AP_MPMQ_STARTING;
/* ap_my_generation are used by the scoreboard code */
ap_generation_t volatile ap_my_generation=0;
@@ -312,6 +313,7 @@
switch(type) {
case SIGNAL_PARENT_SHUTDOWN:
{
+ winnt_mpm_state = AP_MPMQ_STOPPING;
Not needed.
SetEvent(shutdown_event);
break;
}
@@ -319,6 +321,7 @@
case SIGNAL_PARENT_RESTART:
case SIGNAL_PARENT_RESTART_GRACEFUL:
{
+ winnt_mpm_state = AP_MPMQ_STOPPING;
Ditto
is_graceful = 1;
SetEvent(restart_event);
break;
@@ -330,6 +333,7 @@
switch(type) {
case SIGNAL_PARENT_SHUTDOWN:
{
+ winnt_mpm_state = AP_MPMQ_STOPPING;
Ditto
signal_name = signal_shutdown_name;
break;
}
@@ -337,6 +341,7 @@
case SIGNAL_PARENT_RESTART:
case SIGNAL_PARENT_RESTART_GRACEFUL:
{
+ winnt_mpm_state = AP_MPMQ_STOPPING;
Ditto
signal_name = signal_restart_name;
is_graceful = 1;
break;
@@ -1097,6 +1102,9 @@
return APR_SUCCESS;
case AP_MPMQ_MAX_DAEMONS:
*result = 0;
+return APR_SUCCESS;
+case AP_MPMQ_MPM_STATE:
+*result = winnt_mpm_state;
return APR_SUCCESS;
}
return APR_ENOTIMPL;
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
[EMAIL PROTECTED] wrote:
stoddard2003/12/15 15:19:14
Modified:server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
Log:
Win32: Rename WindowsSocketsWorkaround directive to Win32DisableAcceptEx.
Clean up code paths.
Index: mpm_winnt.c
-static const char *set_sockets_workaround (cmd_parms *cmd, void *dummy, char *arg)
+static const char *set_disable_acceptex(cmd_parms *cmd, void *dummy, char *arg)
{
const char *err = ap_check_cmd_context(cmd, GLOBAL_ONLY);
if (err != NULL) {
return err;
}
-
-windows_sockets_workaround = 0;
-if (!strcasecmp(arg, on)) {
-windows_sockets_workaround = 1;
-}
-else if (strcasecmp(arg, off)) {
+if (osver.dwPlatformId == VER_PLATFORM_WIN32_WINDOWS) {
ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
- WARNING: setting WindowsSocketsWorkaround to off);
+ Ignoring Win32EnableAcceptEx configuration directive.
+ The directive is not valid on Windows 9x);
I think you mean Ignoring Win32DisableAcceptEx configuration there.
I don't think we need that message though.
+ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
+ Disabled use AcceptEx WinSock2 API);
I suggest Disabled use of AcceptEx...
I think it would be better if it is written to the error log once the
server is initialized, rather than have it appear temporarily on the
screen. Somebody looking at the error log to try to understand some
misbehavior would want to know this info.
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm.h
Cliff Woolley wrote:
On Thu, 20 Nov 2003 [EMAIL PROTECTED] wrote:
stoddard2003/11/20 11:44:19
Modified:.CHANGES
server/mpm/winnt child.c mpm.h
Log:
Win32: Make Win32 MPM transaction pools honor MaxMemFree
/* Create the tranaction pool */
-if ((rv = apr_pool_create(context-ptrans, pchild)) != APR_SUCCESS)
{
+apr_allocator_create(allocator);
+apr_allocator_max_free_set(allocator, ap_max_mem_free);
+rv = apr_pool_create_ex(context-ptrans, NULL, NULL, allocator);
+if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK,APLOG_WARNING, rv, ap_server_conf,
mpm_get_completion_context: Failed to create the
transaction pool.);
CloseHandle(context-Overlapped.hEvent);
return NULL;
}
-apr_pool_tag(context-ptrans, ptrans);
+apr_allocator_owner_set(allocator, context-ptrans);
+apr_pool_tag(context-ptrans, transaction);
This seems reasonable. +1 (untested). One question, though. Why is
ptrans no longer a child pool of pchild?
--Cliff
Good intentions and all I was going to look into that. This was a straight port from worker.
Bill
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm.h
On Wed, 3 Dec 2003, Bill Stoddard wrote: Good intentions and all I was going to look into that. This was a straight port from worker. Really? Humph. Guess we should look into that, yeah. :-)
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm.h
On Thu, 20 Nov 2003 [EMAIL PROTECTED] wrote:
stoddard2003/11/20 11:44:19
Modified:.CHANGES
server/mpm/winnt child.c mpm.h
Log:
Win32: Make Win32 MPM transaction pools honor MaxMemFree
/* Create the tranaction pool */
-if ((rv = apr_pool_create(context-ptrans, pchild)) !=
APR_SUCCESS) {
+apr_allocator_create(allocator);
+apr_allocator_max_free_set(allocator, ap_max_mem_free);
+rv = apr_pool_create_ex(context-ptrans, NULL, NULL, allocator);
+if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK,APLOG_WARNING, rv, ap_server_conf,
mpm_get_completion_context: Failed to create
the transaction pool.);
CloseHandle(context-Overlapped.hEvent);
return NULL;
}
-apr_pool_tag(context-ptrans, ptrans);
+apr_allocator_owner_set(allocator, context-ptrans);
+apr_pool_tag(context-ptrans, transaction);
This seems reasonable. +1 (untested). One question, though. Why is
ptrans no longer a child pool of pchild?
--Cliff
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm.h
On Thu, 20 Nov 2003 [EMAIL PROTECTED] wrote: stoddard2003/11/20 11:44:19 Modified:.CHANGES server/mpm/winnt child.c mpm.h Log: Win32: Make Win32 MPM transaction pools honor MaxMemFree There's a very small doc change needed here too. (Just add mpm_winnt to the modulelist for that directive.) Joshua.
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
[EMAIL PROTECTED] wrote:
ake 2003/03/04 14:15:52
Modified:.CHANGES
server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
Log:
Added the WindowsSocketsWorkaroud directive for Windows NT/2000/XP
to work around problems with certain VPN and Firewall products that
have buggy AcceptEx implementations
Revision ChangesPath
1.1103+5 -0 httpd-2.0/CHANGES
snip
static const command_rec winnt_cmds[] = {
LISTEN_COMMANDS,
@@ -224,6 +243,9 @@
Number of threads each child creates ),
AP_INIT_TAKE1(ThreadLimit, set_thread_limit, NULL, RSRC_CONF,
Maximum worker threads in a server for this run of Apache),
+AP_INIT_TAKE1(WindowsSocketsWorkaround, set_sockets_workaround, NULL, RSRC_CONF,
+ Set \on\ to work around buggy Winsock provider implementations of certain VPN or Firewall software),
+
{ NULL }
};
Rather than WindowsSocketsWorkaround, why not WinUseWinsock1 or ??. It
would be better I think if the directive somehow indicated exactly what
it was doing (causing the winnt mpm to use the select/accept winsock1
calls rather than AcceptEx, a winsock2 call).
Bill
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
At 12:57 PM 3/13/2003, Bill Stoddard wrote:
[EMAIL PROTECTED] wrote:
ake 2003/03/04 14:15:52
Modified:.CHANGES
server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
Log:
Added the WindowsSocketsWorkaroud directive for Windows NT/2000/XP
to work around problems with certain VPN and Firewall products that
have buggy AcceptEx implementations
Revision ChangesPath
1.1103+5 -0 httpd-2.0/CHANGES
snip
static const command_rec winnt_cmds[] = {
LISTEN_COMMANDS,
@@ -224,6 +243,9 @@
Number of threads each child creates ),
AP_INIT_TAKE1(ThreadLimit, set_thread_limit, NULL, RSRC_CONF,
Maximum worker threads in a server for this run of Apache),
+AP_INIT_TAKE1(WindowsSocketsWorkaround, set_sockets_workaround, NULL, RSRC_CONF,
+ Set \on\ to work around buggy Winsock provider implementations of certain
VPN or Firewall software),
+
{ NULL }
};
Rather than WindowsSocketsWorkaround, why not WinUseWinsock1 or ??. It would be
better I think if the directive somehow indicated exactly what it was doing (causing
the winnt mpm to use the select/accept winsock1 calls rather than AcceptEx, a
winsock2 call).
That would be a misnomer - since our handle inheritance requires winsock2.
What about WindowsUseAcceptEx on|off? That's really descriptive of what
the workaround does. Or, we could call it WindowsFastSockets on|off,
which is the effect of the workaround.
I was also looking at this entire patch - it seems silly to retest for both the
version of windows and this flag throughout the code. Why not simply
initialize it in the register_hooks() call based on the OS version? Then let
the directive override its default value. We should prevent Win9x users
from enabling the flag, however :-)
Bill
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
William A. Rowe, Jr. wrote:
At 12:57 PM 3/13/2003, Bill Stoddard wrote:
[EMAIL PROTECTED] wrote:
ake 2003/03/04 14:15:52
Modified:.CHANGES
server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
Log:
Added the WindowsSocketsWorkaroud directive for Windows NT/2000/XP
to work around problems with certain VPN and Firewall products that
have buggy AcceptEx implementations
Revision ChangesPath
1.1103+5 -0 httpd-2.0/CHANGES
snip
static const command_rec winnt_cmds[] = {
LISTEN_COMMANDS,
@@ -224,6 +243,9 @@
Number of threads each child creates ),
AP_INIT_TAKE1(ThreadLimit, set_thread_limit, NULL, RSRC_CONF,
Maximum worker threads in a server for this run of Apache),
+AP_INIT_TAKE1(WindowsSocketsWorkaround, set_sockets_workaround, NULL, RSRC_CONF,
+ Set \on\ to work around buggy Winsock provider implementations of certain VPN or Firewall software),
+
{ NULL }
};
Rather than WindowsSocketsWorkaround, why not WinUseWinsock1 or ??. It would be better I think if the directive somehow indicated exactly what it was doing (causing the winnt mpm to use the select/accept winsock1 calls rather than AcceptEx, a winsock2 call).
That would be a misnomer - since our handle inheritance requires winsock2.
What about WindowsUseAcceptEx on|off? That's really descriptive of what
the workaround does. Or, we could call it WindowsFastSockets on|off,
which is the effect of the workaround.
I was also looking at this entire patch - it seems silly to retest for both the
version of windows and this flag throughout the code. Why not simply
initialize it in the register_hooks() call based on the OS version? Then let
the directive override its default value.
That might be cool. Post a patch and I'll review it.
We should prevent Win9x users
from enabling the flag, however :-)
Bill
WindowsUseAcceptEx is much better I think. Since it is on by default on
systems which support it. Another (better?) suggestion:
WindowsDisableAcceptEx (with no arguments)?
Bill
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
Rather than WindowsSocketsWorkaround, why not WinUseWinsock1 or ??. It would be better I think if the directive somehow indicated exactly what it was doing (causing the winnt mpm to use the select/accept winsock1 calls rather than AcceptEx, a winsock2 call). We're still usng winsock2 with this directive, it's our use of some of MS winsock extension calls (mswsock.dll) that are biting us. In fact I wrestled with the same question myself before deciding that otherBills suggestion was probably the best. If we were to be more to the point maybe something like WindowsSocketsDontUseAcceptEx, but apart from the length how many webmasters are likely to know what AcceptEx is? I'm open to renaming if we can come up with something more suitable, Allan
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
* Allan Edwards wrote: +AP_INIT_TAKE1(WindowsSocketsWorkaround, set_sockets_workaround, NULL, RSRC_CONF, + Set \on\ to work around buggy Winsock provider implementations of certain VPN or Firewall software), + hey, no need to double code. AP_INIT_FLAG exists ;-) Well I guess that's just a matter of personal preference, probably. My opinion in this case isn't strong either. Actually I wanted to exclude an oversight of the INIT_FLAG macro :) nd -- Wenn nur Ingenieure mit Diplom programmieren würden, hätten wir wahrscheinlich weniger schlechte Software. Wir hätten allerdings auch weniger gute Software. -- Felix von Leitner in dasr
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
probably. My opinion in this case isn't strong either. Actually I wanted to exclude an oversight of the INIT_FLAG macro :) OK, point taken, thanks for the feedback Andre! Allan
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
* [EMAIL PROTECTED] wrote: +AP_INIT_TAKE1(WindowsSocketsWorkaround, set_sockets_workaround, NULL, RSRC_CONF, + Set \on\ to work around buggy Winsock provider implementations of certain VPN or Firewall software), + hey, no need to double code. AP_INIT_FLAG exists ;-) nd -- print Just Another Perl Hacker; # André Malo, http://pub.perlig.de/ #
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
+AP_INIT_TAKE1(WindowsSocketsWorkaround, set_sockets_workaround, NULL, RSRC_CONF, + Set \on\ to work around buggy Winsock provider implementations of certain VPN or Firewall software), + hey, no need to double code. AP_INIT_FLAG exists ;-) Well I guess that's just a matter of personal preference, I don't have strong feelings either way... took the cue from OtherBill. You might argue there is precedence for his style with Keepalive on|off. Any others think on|off is better dispensed with? Allan
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c
[EMAIL PROTECTED] wrote:
wrowe 2002/10/13 20:13:20
Modified:server/mpm/winnt child.c
Log:
Handle WSA_IO_PENDING as well.
My VisualC++ 6 (with MS Platform SDK May 2002) chokes on lines 528 and
529.
There seem to be two )s missing, as well as a boolean operator (XX)
to connect the two terms:
528: else if ((rv != APR_FROM_OS_ERROR(ERROR_IO_PENDING)) XX
529: (rv != APR_FROM_OS_ERROR(WSA_IO_PENDING))) {
--
Sebastian Bergmann
http://sebastian-bergmann.de/ http://phpOpenTracker.de/
Did I help you? Consider a gift: http://wishlist.sebastian-bergmann.de/
RE: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
Bill,
I am chewing on my tounge now to not be nasty. Where are you going with
this? Perhaps I missed it but was this change discussed on list? And the
commit log message describing the change is quite poor.
Bill
-Original Message-
From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]]
Sent: Monday, July 29, 2002 1:13 AM
To: [EMAIL PROTECTED]
Subject: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c
mpm_winnt.h
wrowe 2002/07/28 22:12:50
Modified:server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
Log:
pconf global factors out nicely. The one other pconf appears to be
eqivilant to pchild.
Revision ChangesPath
1.2 +3 -4 httpd-2.0/server/mpm/winnt/child.c
Index: child.c
===
RCS file: /home/cvs/httpd-2.0/server/mpm/winnt/child.c,v
retrieving revision 1.1
retrieving revision 1.2
diff -u -r1.1 -r1.2
--- child.c 29 Jul 2002 05:06:20 - 1.1
+++ child.c 29 Jul 2002 05:12:50 - 1.2
@@ -83,7 +83,6 @@
/* shared with mpm_winnt.c */
extern DWORD my_pid;
-extern apr_pool_t *pconf;
/* used by parent to signal the child to start and exit */
/* shared with mpm_winnt.c, but should be private to child.c */
@@ -97,7 +96,7 @@
/* Queue for managing the passing of COMP_CONTEXTs between
* the accept and worker threads.
*/
-static apr_pool_t *pchild = NULL;
+static apr_pool_t *pchild;
static int shutdown_in_progress = 0;
static int workers_may_exit = 0;
static unsigned int g_blocked_threads = 0;
@@ -417,7 +416,7 @@
if (context == NULL) {
/* allocate the completion context and the transaction pool */
-context = apr_pcalloc(pconf, sizeof(COMP_CONTEXT));
+context = apr_pcalloc(pchild, sizeof(COMP_CONTEXT));
apr_pool_create(context-ptrans, pchild);
apr_pool_tag(context-ptrans, ptrans);
context-ba = apr_bucket_alloc_create(pchild);
@@ -757,7 +756,7 @@
}
-void child_main()
+void child_main(apr_pool_t *pconf)
{
apr_status_t status;
apr_hash_t *ht;
1.290 +103 -1124 httpd-2.0/server/mpm/winnt/mpm_winnt.c
Index: mpm_winnt.c
===
RCS file: /home/cvs/httpd-2.0/server/mpm/winnt/mpm_winnt.c,v
retrieving revision 1.289
retrieving revision 1.290
diff -u -r1.289 -r1.290
--- mpm_winnt.c 27 Jul 2002 18:13:59 - 1.289
+++ mpm_winnt.c 29 Jul 2002 05:12:50 - 1.290
@@ -56,6 +56,8 @@
* University of Illinois, Urbana-Champaign.
*/
+#ifdef WIN32
+
#define CORE_PRIVATE
#include httpd.h
#include http_main.h
@@ -108,19 +110,10 @@
*/
extern apr_shm_t *ap_scoreboard_shm;
server_rec *ap_server_conf;
-typedef HANDLE thread;
/* Definitions of WINNT MPM specific config globals */
-apr_pool_t *pconf;
-static apr_pool_t *pchild = NULL;
-static int workers_may_exit = 0;
-static int shutdown_in_progress = 0;
-static unsigned int g_blocked_threads = 0;
-
static HANDLE shutdown_event; /* used to signal the
parent to shutdown */
static HANDLE restart_event; /* used to signal the
parent to restart */
-static HANDLE exit_event; /* used by parent to signal
the child to exit */
-static HANDLE max_requests_per_child_event;
static char ap_coredump_dir[MAX_STRING_LEN];
@@ -129,23 +122,31 @@
OSVERSIONINFO osver; /* VER_PLATFORM_WIN32_NT */
-apr_proc_mutex_t *start_mutex;
-static DWORD my_pid;
static DWORD parent_pid;
+DWORD my_pid;
int ap_threads_per_child = 0;
/* ap_my_generation are used by the scoreboard code */
ap_generation_t volatile ap_my_generation=0;
-/* Queue for managing the passing of COMP_CONTEXTs between
- * the accept and worker threads.
+
+/* shared by service.c as global, although
+ * perhaps it should be private.
+ */
+apr_pool_t *pconf;
+
+
+/* definitions from child.c */
+void child_main(apr_pool_t *pconf);
+
+/* used by parent to signal the child to start and exit
+ * NOTE: these are not sophisticated enough for multiple children
+ * so they ultimately should not be shared with child.c
*/
-static apr_thread_mutex_t *qlock;
-static PCOMP_CONTEXT qhead = NULL;
-static PCOMP_CONTEXT qtail = NULL;
-static int num_completion_contexts = 0;
-static HANDLE ThreadDispatchIOCP = NULL;
+extern apr_proc_mutex_t *start_mutex;
+extern HANDLE exit_event;
+
/* Stub functions until this MPM supports the connection status API */
@@ -205,206 +206,6 @@
};
-AP_DECLARE(void) mpm_recycle_completion_context(PCOMP_CONTEXT context)
-{
-/* Recycle the completion context.
- * - clear the ptrans pool
- * - put the context on the queue to be consumed
RE: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
Seperating out the routines that run only in the child (and putting them in child.c) is not a bad thing but this patch is difficult to review for several reasons: 1. The commit log did not mention the biggest change. Easy to intuit looking at the code, but it should have at least been mentioned in the commit log. 2. You broke the rule to not make more than one conceptual change to the code within a single commit. You moved code (to child.c) and made a function change (which pools were being used for what) in one commit. These changes should have been made as two seperate commits (or perhaps 3 or 4) to make review a bit less onorous. Bill Bill, I am chewing on my tounge now to not be nasty. Where are you going with this? Perhaps I missed it but was this change discussed on list? And the commit log message describing the change is quite poor. Bill -Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]] Sent: Monday, July 29, 2002 1:13 AM To: [EMAIL PROTECTED] Subject: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h wrowe 2002/07/28 22:12:50 Modified:server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h Log: pconf global factors out nicely. The one other pconf appears to be eqivilant to pchild. Revision ChangesPath 1.2 +3 -4 httpd-2.0/server/mpm/winnt/child.c
RE: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
At 08:15 AM 7/29/2002, you wrote: Seperating out the routines that run only in the child (and putting them in child.c) is not a bad thing but this patch is difficult to review for several reasons: 1. The commit log did not mention the biggest change. Easy to intuit looking at the code, but it should have at least been mentioned in the commit log. The changes to fold out the make_listeners_noninherited should really have been entirely precommitted, not part of the split. I didn't foresee the problems until I was quite a ways through, and unfortunately this box has only one tree. Bigger problem, though. This body of this change should have occurred with the original child.c checkin. That's my bad for missing the fact that it hadn't. 2. You broke the rule to not make more than one conceptual change to the code within a single commit. You moved code (to child.c) and made a function change (which pools were being used for what) in one commit. These changes should have been made as two seperate commits (or perhaps 3 or 4) to make review a bit less onorous. This patch was supposed to be -only- the change to pass [instead of stash] the pconf for pchild. I note that pchild is the direct relative of pconf, so in the Win95 queue code, I presumed that pchild is just as safe for that application. The *intended* patch for mpm_winnt.c simply changed child_main() to pass the pconf. Please read the child.c patch on it's own, and forgive that mpm_winnt changes were all slated (except for the child_main() pconf arg) for the child.c checkin. I am chewing on my tounge now to not be nasty. Where are you going with this? Perhaps I missed it but was this change discussed on list? And the commit log message describing the change is quite poor. The prior commit message is the one you should be interested in. mpm_winnt.c is now just about as difficult to follow as the old src/main.c. Our multiuse of globals makes my head spin. Globals are also quite dangerous with pool scopeing, it's hard to tell just when they will expire. I reached the conclusion to attack this split when I ran into the issue of factoring out the redundant max_requests_per_child_event ... which is truly a special case of exit_event. Such a special case that it should fall into a single enum value in the same grouping as is_graceful. But we are inconsistent about how we toggle the various flags prior to signaling the exit_event, and then wait until we are in the exit_event to toggle is_graceful. This is all quite hard to follow. The real PITA was that I recognized that the parent doesn't know about the child giving up the ghost until it's dead. This is a trivial change; we simply need to keep watching the exit event handle in the parent, as well, and make it a manual reset event. If the child's exit event is signalled, the parent will know we have a child heading out of existence, and we can preemptively begin a new child process while the old one cycles down. wrowe 2002/07/28 22:12:50 Modified:server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h Log: pconf global factors out nicely. The one other pconf appears to be eqivilant to pchild. The -huge- problem was that this was not a balanced commit. My fault that I didn't notice that mpm_winnt change itself was not within the following commit; I've added the following comment to mpm_winnt.c's and .h's logs. Modified:.libhttpd.dsp os/win32 ap_regkey.c os.h util_win32.c Added: server/mpm/winnt child.c Log: Refactor out the child behavior from mpm_winnt. This is the first step in making a legible multiprocess windows mpm, or at least structuring the code to always begin a new child as an old one is going to die soon, rather than waiting for it's final dying breath. The only code that had to be affected [due to the split and general structure of the code] was merging the set_listeners_noninherited() code directly into the get_listeners_from_parent code, and also into the apr socket.c code for winnt. For the most part the code splits rather nicely.
