You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Stefan Fritsch <sf...@sfritsch.de> on 2012/01/01 21:39:08 UTC
Re: 2.3.15 RewriteRule P
On Wednesday 16 November 2011, Steffen wrote:
> What I noticed, it is connecting to a port by a formerly used
> proxied connection (port 7080 instead of 81);
>
> Summary log:
>
> [proxy:debug] [pid 8680:tid 2668] proxy_util.c(2140): proxy: HTTP:
> has acquired connection for (*)
> [proxy:debug] [pid 8680:tid 2668] proxy_util.c(2193): proxy:
> connecting http://127.0.0.1:81/sysadmin to 127.0.0.1:81
> [proxy:debug] [pid 8680:tid 2668] proxy_util.c(2319): proxy:
> connected /sysadmin to 127.0.0.1:7080
After a cursory glance at the code, I have a suspicion about the
reason. It seems to me that this check in proxy_util.c
/*
* Make sure that we pick the the correct and valid worker.
* If a single keepalive connection triggers different workers,
* then we have a problem (we don't select the correct one).
* Do an expensive check in this case, where we compare the
* the hostnames associated between the two.
*
* TODO: Handle this much better...
*/
if (!conn->hostname || !worker->s->is_address_reusable ||
worker->s->disablereuse ||
(r->connection->keepalives &&
(r->proxyreq == PROXYREQ_PROXY || r->proxyreq ==
PROXYREQ_REVERSE) &&
(strcasecmp(conn->hostname, uri->hostname) != 0) ) ) {
should also compare conn->port and uri->port, i.e. the
(strcasecmp(conn->hostname, uri->hostname) != 0)
should be
(strcasecmp(conn->hostname, uri->hostname) != 0 ||
conn->port != uri->port)
Can anyone more familiar with the code verify this? Steffen, maybe you
can try the change and see if it helps?
Cheers,
Stefan
Re: 2.3.15 RewriteRule P
Posted by Eric Covener <co...@gmail.com>.
On Wed, Jan 4, 2012 at 3:42 PM, Rüdiger Plüm <r....@gmx.de> wrote:
>
>
> Eric Covener wrote:
>>> The different handling of conn->port and conn->hostname doesn't look
>>> right to me. Can the r->proxyreq check actually be false at this point or is
>>> it simply redundant? And shouldn't the strcasecmp(conn->hostname,
>>> uri->hostname) check be done regardless of r->connection->keepalives?
>>
>> I did not understand the r->keepalives "optimization", maybe comparing
>> hostnames could just come last.
>>
>
> I don't think that this "optimization" is valid, because this can IMHO lead to
> a situation where a non keepalive frontend connection reuses the wrong backend
> connection. Keepalives between frontend and backend connections are decoupled.
>
> So it should be simply removed.
Just to close the loop on this aspect -- AFAICT the reason we have
this is a vestige of 2.0.x where the backend proxy_conn_rec was
cached in the frotnend per-connection config. There you could easily
hit /app1 and /app2 over keepalive and this check would kick in. But
this is n/a in 2.2 and later (the strcmp is not needed at all as
opposed to being needed on initial req).
>
>
> Regards
>
> Rüdiger
--
Eric Covener
covener@gmail.com
Re: 2.3.15 RewriteRule P
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jan 4, 2012, at 9:25 PM, Eric Covener wrote:
>
> Is keeping the generic worker reusable important?
>
If possible, then yeah. But if adds a lot of complexity to the
code, or reduces the efficiency of the named workers, then
I'd punt it for now...
Re: 2.3.15 RewriteRule P
Posted by Eric Covener <co...@gmail.com>.
On Wed, Jan 4, 2012 at 3:42 PM, Rüdiger Plüm <r....@gmx.de> wrote:
>
>
> Eric Covener wrote:
>>> The different handling of conn->port and conn->hostname doesn't look
>>> right to me. Can the r->proxyreq check actually be false at this point or is
>>> it simply redundant? And shouldn't the strcasecmp(conn->hostname,
>>> uri->hostname) check be done regardless of r->connection->keepalives?
>>
>> I did not understand the r->keepalives "optimization", maybe comparing
>> hostnames could just come last.
>>
>
> I don't think that this "optimization" is valid, because this can IMHO lead to
> a situation where a non keepalive frontend connection reuses the wrong backend
> connection. Keepalives between frontend and backend connections are decoupled.
>
> So it should be simply removed.
>
Some more fishy stuff here at least with a reusable-addr generic worker.
IIUC The generic worker can never safely copy the
proxy_worker->cp->addr down into a a proxy_conn_rec, because the
proxy_worker only stores the addr and not the hostname that went into
it. Both the URI and the ProxyRemote could be changing under the
covers.
We would need to store the host and port in the worker too, then make
conn->addr and conn->hostname allocated out of a new subpool for the
pair, since right now these things that can flip based on URI or
ProxyRemote are allocated out of the same thing as all of
proxy_conn_rec.
Is keeping the generic worker reusable important?
Re: 2.3.15 RewriteRule P
Posted by Rüdiger Plüm <r....@gmx.de>.
Eric Covener wrote:
>> The different handling of conn->port and conn->hostname doesn't look
>> right to me. Can the r->proxyreq check actually be false at this point or is
>> it simply redundant? And shouldn't the strcasecmp(conn->hostname,
>> uri->hostname) check be done regardless of r->connection->keepalives?
>
> I did not understand the r->keepalives "optimization", maybe comparing
> hostnames could just come last.
>
I don't think that this "optimization" is valid, because this can IMHO lead to
a situation where a non keepalive frontend connection reuses the wrong backend
connection. Keepalives between frontend and backend connections are decoupled.
So it should be simply removed.
Regards
Rüdiger
Re: 2.3.15 RewriteRule P
Posted by Eric Covener <co...@gmail.com>.
> The different handling of conn->port and conn->hostname doesn't look
> right to me. Can the r->proxyreq check actually be false at this point or is
> it simply redundant? And shouldn't the strcasecmp(conn->hostname,
> uri->hostname) check be done regardless of r->connection->keepalives?
I did not understand the r->keepalives "optimization", maybe comparing
hostnames could just come last.
Re: 2.3.15 RewriteRule P
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Sun, 1 Jan 2012, Eric Covener wrote:
>> Can anyone more familiar with the code verify this? Steffen, maybe you
>> can try the change and see if it helps?
>
> I think there are a few additional wrinkles -- I couldn't repro after
> this but no confident about what's right with the addr handling:
>
> http://people.apache.org/~covener/patches/trunk-proxy_toggle_ports.diff
> Index: modules/proxy/proxy_util.c
> ===================================================================
> --- modules/proxy/proxy_util.c (revision 1226300)
> +++ modules/proxy/proxy_util.c (working copy)
> @@ -2029,7 +2029,7 @@
> * TODO: Handle this much better...
> */
> if (!conn->hostname || !worker->s->is_address_reusable ||
> - worker->s->disablereuse ||
> + worker->s->disablereuse || conn->port != uri->port ||
> (r->connection->keepalives &&
> (r->proxyreq == PROXYREQ_PROXY || r->proxyreq == PROXYREQ_REVERSE) &&
> (strcasecmp(conn->hostname, uri->hostname) != 0) ) ) {
The different handling of conn->port and conn->hostname doesn't look
right to me. Can the r->proxyreq check actually be false at this point or is
it simply redundant? And shouldn't the strcasecmp(conn->hostname,
uri->hostname) check be done regardless of r->connection->keepalives?
> @@ -2076,6 +2076,8 @@
> conn->pool);
> }
> else if (!worker->cp->addr) {
> + conn->port = uri->port;
> + conn->hostname = apr_pstrdup(conn->pool, uri->hostname);
> if ((err = PROXY_THREAD_LOCK(worker)) != APR_SUCCESS) {
> ap_log_rerror(APLOG_MARK, APLOG_ERR, err, r, APLOGNO(00945) "lock");
> return HTTP_INTERNAL_SERVER_ERROR;
> @@ -2097,7 +2099,9 @@
> }
> }
> else {
> - conn->addr = worker->cp->addr;
> + if (!conn->addr) {
> + conn->addr = worker->cp->addr;
> + }
> }
> /* Close a possible existing socket if we are told to do so */
> if (conn->close) {
> <virtualhost *:80>
> RewriteEngine on
> RewriteRule ^/a http://localhost:81/a [P]
> RewriteRule ^/b http://localhost:82/b [P]
> </virtualhost>
> <virtualhost *:81>
> AliasMatch ^/a$ /home/covener/SRC/httpd-trunk/built/icons/a.gif
> </virtualhost>
> <virtualhost *:82>
> AliasMatch ^/b$ /home/covener/SRC/httpd-trunk/built/icons/a.gif
> </virtualhost>
>
> Before any patch, as simple as a/b fails. The worker->cp->addr copy
> issue fails with something like a/b/b.
Re: 2.3.15 RewriteRule P
Posted by Eric Covener <co...@gmail.com>.
> Can anyone more familiar with the code verify this? Steffen, maybe you
> can try the change and see if it helps?
I think there are a few additional wrinkles -- I couldn't repro after
this but no confident about what's right with the addr handling:
http://people.apache.org/~covener/patches/trunk-proxy_toggle_ports.diff
<virtualhost *:80>
RewriteEngine on
RewriteRule ^/a http://localhost:81/a [P]
RewriteRule ^/b http://localhost:82/b [P]
</virtualhost>
<virtualhost *:81>
AliasMatch ^/a$ /home/covener/SRC/httpd-trunk/built/icons/a.gif
</virtualhost>
<virtualhost *:82>
AliasMatch ^/b$ /home/covener/SRC/httpd-trunk/built/icons/a.gif
</virtualhost>
Before any patch, as simple as a/b fails. The worker->cp->addr copy
issue fails with something like a/b/b.
Re: 2.3.15 RewriteRule P
Posted by Ruediger Pluem <rp...@apache.org>.
Stefan Fritsch wrote:
> On Wednesday 16 November 2011, Steffen wrote:
>> What I noticed, it is connecting to a port by a formerly used
>> proxied connection (port 7080 instead of 81);
>>
>> Summary log:
>>
>> [proxy:debug] [pid 8680:tid 2668] proxy_util.c(2140): proxy: HTTP:
>> has acquired connection for (*)
>> [proxy:debug] [pid 8680:tid 2668] proxy_util.c(2193): proxy:
>> connecting http://127.0.0.1:81/sysadmin to 127.0.0.1:81
>> [proxy:debug] [pid 8680:tid 2668] proxy_util.c(2319): proxy:
>> connected /sysadmin to 127.0.0.1:7080
>
>
> After a cursory glance at the code, I have a suspicion about the
> reason. It seems to me that this check in proxy_util.c
>
> /*
> * Make sure that we pick the the correct and valid worker.
> * If a single keepalive connection triggers different workers,
> * then we have a problem (we don't select the correct one).
> * Do an expensive check in this case, where we compare the
> * the hostnames associated between the two.
> *
> * TODO: Handle this much better...
> */
> if (!conn->hostname || !worker->s->is_address_reusable ||
> worker->s->disablereuse ||
> (r->connection->keepalives &&
> (r->proxyreq == PROXYREQ_PROXY || r->proxyreq ==
> PROXYREQ_REVERSE) &&
> (strcasecmp(conn->hostname, uri->hostname) != 0) ) ) {
>
>
>
> should also compare conn->port and uri->port, i.e. the
>
>
> (strcasecmp(conn->hostname, uri->hostname) != 0)
>
>
> should be
>
>
> (strcasecmp(conn->hostname, uri->hostname) != 0 ||
> conn->port != uri->port)
>
>
> Can anyone more familiar with the code verify this? Steffen, maybe you
> can try the change and see if it helps?
It should. There was a change in behaviour. Previously worker->s->is_address_reusable
was always 0 for the worker used with this configuration. This is no longer true.
Hence we see this issue now.
Regards
Rüdiger