You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2010/11/16 07:56:57 UTC
Re: svn commit: r1035504 - in /httpd/httpd/trunk: include/ap_mmn.h
modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
On 11/16/2010 01:23 AM, minfrin@apache.org wrote:
> Author: minfrin
> Date: Tue Nov 16 00:23:37 2010
> New Revision: 1035504
>
> URL: http://svn.apache.org/viewvc?rev=1035504&view=rev
> Log:
> Fix pool lifetime issues when the proxy backend connection is terminated
> early by forcing a setaside on transient buckets placed in the brigade
> by mod_ssl. This has the effect of extending the lifetime of buckets until
> the end of the request. This is a variation on the original fix for this
> problem, which added transient buckets to be setaside later in the process.
>
> Modified:
> httpd/httpd/trunk/include/ap_mmn.h
> httpd/httpd/trunk/modules/proxy/mod_proxy.h
> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
> httpd/httpd/trunk/modules/proxy/proxy_util.c
>
> Modified: httpd/httpd/trunk/include/ap_mmn.h
>
> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1035504&r1=1035503&r2=1035504&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Tue Nov 16 00:23:37 2010
> @@ -2643,19 +2643,17 @@ PROXY_DECLARE(int) ap_proxy_connection_c
> apr_sockaddr_t *backend_addr = conn->addr;
> int rc;
> apr_interval_time_t current_timeout;
> - apr_bucket_alloc_t *bucket_alloc;
>
> if (conn->connection) {
> return OK;
> }
>
> - bucket_alloc = apr_bucket_alloc_create(conn->scpool);
> /*
> * The socket is now open, create a new backend server connection
> */
> conn->connection = ap_run_create_connection(conn->scpool, s, conn->sock,
> 0, NULL,
> - bucket_alloc);
> + c->bucket_alloc);
-1 Veto. This does not work. The socket bucket of the backend connection is created from
this bucket allocator. Hence the reuse of connections from the backend will be broken
as c->bucket_alloc will be gone when the backend connection and hence the socket bucket
is reused.
The only way that *might* work is to change the bucket allocator *after* the connection is used for
the first request (as the socket bucket is created on the fly the first time
ap_core_input_filter is used). But this does not resolve the lifetime issue for
the first connection.
See also
http://mail-archives.apache.org/mod_mbox/httpd-dev/201011.mbox/%3c99EA83DCDE961346AFA9B5EC33FEC08B051D3F0B@VF-MBX11.internal.vodafone.com%3e
Regards
Rüdiger
Re: svn commit: r1035504 - in /httpd/httpd/trunk: include/ap_mmn.h modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 16 Nov 2010, at 12:17 PM, Plüm, Rüdiger, VF-Group wrote:
> Sorry for having been in grumpy mode this morning, but I saw this code
> which is what I pointed out before to be not working :-).
As you've pointed out, the code is definitely wrong, I suspect you
choked on your coffee and for that I apologise :)
> That looks like a plan. *Before* we sent the final brigade (which
> had its
> buckets transformed to the transient buckets of c->bucket_alloc)
> containing the EOS bucket do a setaside on each bucket in this brigade
> (maybe via ap_save_brigade).
That is what the new patch did, the mistake I made was to think that
the forced setaside could replace the wrapping in transient buckets,
when in reality it needed to be done in addition to the wrapping in
transient buckets.
This is the part of the original patch that does it:
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1035578)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -1902,7 +1902,6 @@
/* Switch the allocator lifetime of the buckets */
ap_proxy_buckets_lifetime_transform(r, bb,
pass_bb);
- apr_brigade_cleanup(bb);
/* found the last brigade? */
if
(APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(pass_bb))) {
@@ -1910,6 +1909,23 @@
/* signal that we must leave */
finish = TRUE;
+ /* the brigade may contain transient buckets
that contain
+ * data that lives only as long as the
backend connection.
+ * Force a setaside so these transient
buckets become heap
+ * buckets that live as long as the request.
+ */
+ for (e = APR_BRIGADE_FIRST(pass_bb); e
+ != APR_BRIGADE_SENTINEL(pass_bb); e
+ = APR_BUCKET_NEXT(e)) {
+ apr_bucket_setaside(e, r->pool);
+ }
+
+ /* finally it is safe to clean up the brigade
from the
+ * connection pool, as we have forced a
setaside on all
+ * buckets.
+ */
+ apr_brigade_cleanup(bb);
+
/* make sure we release the backend
connection as soon
* as we know we are done, so that the
backend isn't
* left waiting for a slow client to
eventually
@@ -1930,6 +1946,7 @@
/* make sure we always clean up after ourselves */
apr_brigade_cleanup(pass_bb);
+ apr_brigade_cleanup(bb);
} while (!finish);
}
Regards,
Graham
--
RE: svn commit: r1035504 - in /httpd/httpd/trunk: include/ap_mmn.h modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Graham Leggett
> Sent: Dienstag, 16. November 2010 10:53
> To: dev@httpd.apache.org
> =====================================================================
> >> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> >> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Tue Nov 16
> >> 00:23:37 2010
> >> @@ -2643,19 +2643,17 @@ PROXY_DECLARE(int) ap_proxy_connection_c
> >> apr_sockaddr_t *backend_addr = conn->addr;
> >> int rc;
> >> apr_interval_time_t current_timeout;
> >> - apr_bucket_alloc_t *bucket_alloc;
> >>
> >> if (conn->connection) {
> >> return OK;
> >> }
> >>
> >> - bucket_alloc = apr_bucket_alloc_create(conn->scpool);
> >> /*
> >> * The socket is now open, create a new backend server
> connection
> >> */
> >> conn->connection = ap_run_create_connection(conn->scpool, s,
> >> conn->sock,
> >> 0, NULL,
> >> - bucket_alloc);
> >> + c->bucket_alloc);
> >
> > -1 Veto. This does not work.
>
> Just to clear up any possible perception of otherwise, I have spent
> hours and hours trying to pick apart this code and try to understand
> exactly what both mod_proxy and mod_ssl are doing, and why
> mod_proxy_http is behaving so dramatically differently to
> mod_proxy_ftp and mod_proxy_connect, in order to fix a very real
> problem in a very real set of datacentres. I would appreciate it if
> you could help me get to the bottom of any issues I may not be
> understanding so that this can be fixed once and for all. You don't
> need to veto anything to get my attention, you can just say "this
> won't work and this is why". :)
Sorry for having been in grumpy mode this morning, but I saw this code
which is what I pointed out before to be not working :-).
>
> > The socket bucket of the backend connection is created from
> > this bucket allocator. Hence the reuse of connections from the
> > backend will be broken
> > as c->bucket_alloc will be gone when the backend connection and
> > hence the socket bucket
> > is reused.
>
> Ok, I originally understood that the conn_rec was recreated
> each time,
> but this isn't the case.
>
> This is making more sense.
>
> What it seems we need to do is keep wrapping buckets in transient
> buckets as we were doing before, and then, for the final EOS case,
> force the setaside to guarantee that that last set of buckets have
> been physically moved to the frontend connection, and the
> backend can
> be safely released and reused.
That looks like a plan. *Before* we sent the final brigade (which had its
buckets transformed to the transient buckets of c->bucket_alloc)
containing the EOS bucket do a setaside on each bucket in this brigade
(maybe via ap_save_brigade). Maybe something along the following lines
(completely untested and only typed on a scratchpad):
/* Switch the allocator lifetime of the buckets */
ap_proxy_buckets_lifetime_transform(r, bb, pass_bb);
/* found the last brigade? */
if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(pass_bb))) {
apr_bucket_brigade *saved_bb;
/* signal that we must leave */
finish = TRUE;
saved_bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
ap_save_brigade(NULL, &saved_bb, &pass_bb, r->pool);
pass_bb = saved_bb;
apr_brigade_cleanup(bb);
/* make sure we release the backend connection as soon
* as we know we are done, so that the backend isn't
* left waiting for a slow client to eventually
* acknowledge the data.
*/
ap_proxy_release_connection(backend->worker->scheme,
backend, r->server);
}
/* try send what we read */
if (ap_pass_brigade(r->output_filters, pass_bb) != APR_SUCCESS
|| c->aborted) {
/* Ack! Phbtt! Die! User aborted! */
backend->close = 1; /* this causes socket close below */
finish = TRUE;
apr_brigade_cleanup(bb);
}
/* make sure we always clean up after ourselves */
if (!finish) {
apr_brigade_cleanup(bb);
}
apr_brigade_cleanup(pass_bb);
Regards
Rüdiger
Re: svn commit: r1035504 - in /httpd/httpd/trunk: include/ap_mmn.h modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 16 Nov 2010, at 8:56 AM, Ruediger Pluem wrote:
>> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1035504&r1=1035503&r2=1035504&view=diff
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =====================================================================
>> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
>> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Tue Nov 16
>> 00:23:37 2010
>> @@ -2643,19 +2643,17 @@ PROXY_DECLARE(int) ap_proxy_connection_c
>> apr_sockaddr_t *backend_addr = conn->addr;
>> int rc;
>> apr_interval_time_t current_timeout;
>> - apr_bucket_alloc_t *bucket_alloc;
>>
>> if (conn->connection) {
>> return OK;
>> }
>>
>> - bucket_alloc = apr_bucket_alloc_create(conn->scpool);
>> /*
>> * The socket is now open, create a new backend server connection
>> */
>> conn->connection = ap_run_create_connection(conn->scpool, s,
>> conn->sock,
>> 0, NULL,
>> - bucket_alloc);
>> + c->bucket_alloc);
>
> -1 Veto. This does not work.
Just to clear up any possible perception of otherwise, I have spent
hours and hours trying to pick apart this code and try to understand
exactly what both mod_proxy and mod_ssl are doing, and why
mod_proxy_http is behaving so dramatically differently to
mod_proxy_ftp and mod_proxy_connect, in order to fix a very real
problem in a very real set of datacentres. I would appreciate it if
you could help me get to the bottom of any issues I may not be
understanding so that this can be fixed once and for all. You don't
need to veto anything to get my attention, you can just say "this
won't work and this is why". :)
> The socket bucket of the backend connection is created from
> this bucket allocator. Hence the reuse of connections from the
> backend will be broken
> as c->bucket_alloc will be gone when the backend connection and
> hence the socket bucket
> is reused.
Ok, I originally understood that the conn_rec was recreated each time,
but this isn't the case.
This is making more sense.
What it seems we need to do is keep wrapping buckets in transient
buckets as we were doing before, and then, for the final EOS case,
force the setaside to guarantee that that last set of buckets have
been physically moved to the frontend connection, and the backend can
be safely released and reused.
Regards,
Graham
--