Ryan, I've CC'd you on this just to let you see the patch. If you don't
want me to involve you in this, please accept my apologies and let me
know and I won't CC you in any further patches.
I spent some time debugging the perchild MPM since it wasn't working for
me, nor anyone else it seems. I've found a few problems:
Problem 1:
In worker_thread, there is a variable called csd that is used to get
the new socket from lr->accept_func(). If that variable is NULL, then
the memory for the new socket is allocated in the per-transaction pool.
Unfortunately, the code neglected to reset the variable to NULL after
servicing a request. The result is that the first request for each
thread worked fine, but subsequent request may have the memory
overwritten and resulting in an invalid FD.
Solution:
Set it to NULL before calling lr->accept_func(). Implemented in the patch
below.
Problem 2:
pass_request fills out an iovec with the headers and the body of the
request it wants to pass to another process. It unfortunately uses the
wrong variable for the length causing it to always be 0.
Solution:
Use the correct variable name "l". len is never set to anything so I removed
that and used 0 in the one reference to it. Implemented in the patch below.
Problem 3:
receive_from_other_child assumes that the iovec is the same on read as
it is on write. This isn't true and readmsg() follows readv() semantics.
iovec is a scatter/gather list and as a result, the 2 send buffers are
merged into one received buffer with the second always being untouched.
It also trusted the lengths in iov.iov_len which will be the size of the
original buffer, not the size of the data actually received.
Solution:
Merge the 2 buffer's into 1 and find the null terminators for the 2 strings.
Implemented in the patch below.
Problem 4:
Occasionally, when a request needs to be passed to another process, it will
hang until another connection is accepted. perchild uses the same process
accept mutex that the other MPM's seem to need. This results in
serialization of all accept() calls as is the intent of the mutex. The
problem is that another process might have acquired the mutex from the
process that actually needs the mutex to hit accept() and see the passed
request. This isn't normally a problem because all processes accept
connections on all listening ports. The problem in this case is the fact
not all processes accept connections on the per process unix domain socket
to pass connections around, for obvious reasons.
Solution:
I don't have one :) I'm not sure what the best way of solving this is. I
don't fully understand the semantics of the process accept mutex yet. Any
suggestions?
Anyway, this patch has been tested lightly and makes things work MUCH better
for the perchild MPM now, atleast for me :)
If this patch is acceptable by the Apache development team, can it be
applied?
JE
Index: perchild.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/server/mpm/experimental/perchild/perchild.c,v
retrieving revision 1.135
diff -u -r1.135 perchild.c
--- perchild.c 11 Oct 2002 15:41:52 -0000 1.135
+++ perchild.c 18 Oct 2002 01:31:22 -0000
@@ -679,9 +679,9 @@
{
struct msghdr msg;
struct cmsghdr *cmsg;
- char headers[HUGE_STRING_LEN];
- char request_body[HUGE_STRING_LEN];
- struct iovec iov[2];
+ char buffer[HUGE_STRING_LEN * 2], *headers, *body;
+ int headerslen, bodylen;
+ struct iovec iov;
int ret, dp;
apr_os_sock_t sd;
apr_bucket_alloc_t *alloc = apr_bucket_alloc_create(ptrans);
@@ -690,15 +690,13 @@
apr_os_sock_get(&sd, lr->sd);
- iov[0].iov_base = headers;
- iov[0].iov_len = HUGE_STRING_LEN;
- iov[1].iov_base = request_body;
- iov[1].iov_len = HUGE_STRING_LEN;
+ iov.iov_base = buffer;
+ iov.iov_len = sizeof(buffer);
msg.msg_name = NULL;
msg.msg_namelen = 0;
- msg.msg_iov = iov;
- msg.msg_iovlen = 2;
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
cmsg = apr_palloc(ptrans, sizeof(*cmsg) + sizeof(sd));
cmsg->cmsg_len = sizeof(*cmsg) + sizeof(sd);
@@ -715,14 +713,25 @@
APR_BRIGADE_INSERT_HEAD(bb, bucket);
bucket = apr_bucket_socket_create(*csd, alloc);
APR_BRIGADE_INSERT_HEAD(bb, bucket);
- bucket = apr_bucket_heap_create(iov[1].iov_base,
- iov[1].iov_len, NULL, alloc);
+
+ body = strchr(iov.iov_base, 0);
+ if (!body) {
+ return 1;
+ }
+
+ body++;
+ bodylen = strlen(body);
+
+ headers = iov.iov_base;
+ headerslen = body - headers;
+
+ bucket = apr_bucket_heap_create(body, bodylen, NULL, alloc);
APR_BRIGADE_INSERT_HEAD(bb, bucket);
- bucket = apr_bucket_heap_create(iov[0].iov_base,
- iov[0].iov_len, NULL, alloc);
+ bucket = apr_bucket_heap_create(headers, headerslen, NULL, alloc);
APR_BRIGADE_INSERT_HEAD(bb, bucket);
apr_pool_userdata_set(bb, "PERCHILD_SOCKETS", NULL, ptrans);
+
return 0;
}
@@ -730,7 +739,7 @@
static void *worker_thread(apr_thread_t *thd, void *arg)
{
- void *csd = NULL;
+ void *csd;
apr_pool_t *tpool; /* Pool for this thread */
apr_pool_t *ptrans; /* Pool for per-transaction stuff */
volatile int thread_just_started = 1;
@@ -832,6 +841,7 @@
}
got_fd:
if (!workers_may_exit) {
+ csd = NULL;
rv = lr->accept_func(&csd, lr, ptrans);
if (rv == APR_EGENERAL) {
/* E[NM]FILE, ENOMEM, etc */
@@ -1635,7 +1645,6 @@
apr_bucket_brigade *bb = apr_brigade_create(r->pool, c->bucket_alloc);
apr_bucket_brigade *sockbb;
char request_body[HUGE_STRING_LEN] = "\0";
- apr_off_t len = 0;
apr_size_t l = 0;
perchild_header h;
apr_bucket *sockbuck;
@@ -1647,7 +1656,7 @@
"passing request to another child. Vhost: %s, child %d %d",
apr_table_get(r->headers_in, "Host"), child_num, sconf->output);
ap_get_brigade(r->connection->input_filters, bb, AP_MODE_EXHAUSTIVE,
APR_NONBLOCK_READ,
- len);
+ 0);
for (sockbuck = APR_BRIGADE_FIRST(bb); sockbuck != APR_BRIGADE_SENTINEL(bb);
sockbuck = APR_BUCKET_NEXT(sockbuck)) {
@@ -1678,7 +1687,7 @@
iov[0].iov_base = h.headers;
iov[0].iov_len = strlen(h.headers) + 1;
iov[1].iov_base = request_body;
- iov[1].iov_len = len + 1;
+ iov[1].iov_len = l + 1;
msg.msg_name = NULL;
msg.msg_namelen = 0;
