ping!
Please don't hesitate to push back and tell me if I can supply the patch or
update in some easier-to-digest form. In particular, while I have
rigorously stress-tested this change using mod_pagespeed's unit test,
system-test, and load-test framework, I don't really understand what the
testing flow is for APR. I'd be happy to add unit-tests for that if
someone points me to a change-list or patch-file that does it properly.
-Josh
On Thu, Nov 1, 2012 at 8:04 AM, Joshua Marantz jmara...@google.com wrote:
I have completed a solution to this problem, which can be a drop-in update
for the existing apr_memcache.c. It is now checked in for my module as
http://code.google.com/p/modpagespeed/source/browse/trunk/src/third_party/aprutil/apr_memcache2.c
.
It differs from the solution in
https://issues.apache.org/bugzilla/show_bug.cgi?id=51065 in that:
- It doesn't require an API change; it but it enforces the 50ms
timeout that already exists for apr_multgetp for all operations.
- It works under my load test (which I found is not true of the patch
in 51065).
For my own purposes, I will be shipping my module with apr_memcache2 so I
get the behavior I want regardless of what version of Apache is installed.
But I'd like to propose my patch for apr_memcache.c. The patch is
attached, and I've also submitted it as an alternative patch to bug 51065.
If you agree with the strategy I used to solve this problem, then please
let me know if I can help with any changes required to get this into the
main distribution,
On Mon, Oct 22, 2012 at 5:21 PM, Joshua Marantz jmara...@google.comwrote:
I've had some preliminary success with my own variant of apr_memcache.c
(creatively called apr_memcache2.c). Rather than setting the socket
timeout, I've been mimicing the timeout strategy I saw in
apr_memcache_multgetp, by adding a new helper method:
static apr_status_t wait_for_server_or_timeout(apr_pool_t* temp_pool,
apr_memcache2_conn_t*
conn) {
apr_pollset_t* pollset;
apr_status_t rv = apr_pollset_create(pollset, 1, temp_pool, 0);
if (rv == APR_SUCCESS) {
apr_pollfd_t pollfd;
pollfd.desc_type = APR_POLL_SOCKET;
pollfd.reqevents = APR_POLLIN;
pollfd.p = temp_pool;
pollfd.desc.s = conn-sock;
pollfd.client_data = NULL;
apr_pollset_add(pollset, pollfd);
apr_int32_t queries_recvd;
const apr_pollfd_t* activefds;
rv = apr_pollset_poll(pollset, MULT_GET_TIMEOUT, queries_recvd,
activefds);
if (rv == APR_SUCCESS) {
assert(queries_recvd == 1);
assert(activefds-desc.s == conn-sock);
assert(activefds-client_data == NULL);
}
}
return rv;
}
And calling that before many of the existing calls to get_server_line as:
rv = wait_for_server_or_timeout_no_pool(conn);
if (rv != APR_SUCCESS) {
ms_release_conn(ms, conn);
return rv;
}
This is just an experiment; I think I can streamline this by
pre-populating the pollfd structure as part of the apr_memcache_conn_t
(actually now apr_memcache2_conn_t).
I have two questions about this:
1. I noticed the version of apr_memcache.c that ships with Apache 2.4 is
somewhat different from the one that ships with Apache 2.2. In particular
the 2.4 version cannot be compiled against the headers that come with a 2.2
distribution. Is there any downside to taking my hacked 2.2 apr_memcache.c
and running it in Apache 2.4? Or should I maintain two hacks?
2. This seems wasteful in terms of system calls. I am making an extra
call to poll, rather than relying on the socket timeout. The socket
timeout didn't work as well as this though. Does anyone have any theories
as to why, or what could be done to the patch in
https://issues.apache.org/bugzilla/show_bug.cgi?id=51065 to work?
-Josh
On Fri, Oct 19, 2012 at 9:25 AM, Joshua Marantz jmara...@google.comwrote:
Following up: I tried doing what I suggested above: patching that change
into my own copy of apr_memcache.c It was first of all a bad idea to pull
in only part of apr_memcache.c because that file changed slightly between
2.2 and 2.4 and our module works in both.
I was successful making my own version of apr_memcache (renaming
entry-points apr_memcache2*) that I could hack. But if I changed the
socket timeout from -1 to 10 seconds, then the system behaved very poorly
under load test (though it worked fine in our unit-tests and system-tests).
In other words, I think the proposed patch that Jeff pointed to above is
not really working (as he predicted). This test was done without
SIGSTOPing the memcached; it would timeout under our load anyway and
thereafter behave poorly.
I'm going to follow up on that bugzilla entry, but for now I'm going to
pursue my own complicated mechanism of timing out the calls
