You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2014/01/22 23:40:42 UTC
Re: svn commit: r1560367 - in /httpd/httpd/trunk: docs/log-message-tags/next-number
modules/mappers/mod_rewrite.c modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h
modules/proxy/proxy_util.c
On Wed, Jan 22, 2014 at 3:54 PM, <ji...@apache.org> wrote:
> Author: jim
> Date: Wed Jan 22 14:54:21 2014
> New Revision: 1560367
>
> URL: http://svn.apache.org/r1560367
> Log:
> make mod_rewrite and mod_proxy UDS work together...
>
> [...]
>
> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1560367&r1=1560366&r2=1560367&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Wed Jan 22 14:54:21 2014
> @@ -1927,11 +1927,14 @@ PROXY_DECLARE(int) ap_proxy_pre_request(
> }
> }
> else if (r->proxyreq == PROXYREQ_REVERSE) {
> + char *ptr;
> + const char *ptr2;
> if (conf->reverse) {
> ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
> "*: found reverse proxy worker for %s",
> *url);
> *balancer = NULL;
> *worker = conf->reverse;
> + *(*worker)->s->uds_path = '\0';
> access_status = OK;
> /*
> * The reverse worker does not keep connections alive, so
> @@ -1939,6 +1942,30 @@ PROXY_DECLARE(int) ap_proxy_pre_request(
> * regarding the Connection header in the request.
> */
> apr_table_setn(r->subprocess_env, "proxy-nokeepalive",
> "1");
> + /*
> + * In the case of the generic reverse proxy, we need to
> see if we
> + * were passed a UDS url (eg: from mod_proxy) and adjust
> uds_path
> + * as required.
> + */
> + if (apr_table_get(r->notes, "rewrite-proxy") &&
> + (ptr2 = ap_strstr_c(r->filename, "unix:")) &&
> + (ptr = ap_strchr(r->filename, '|'))) {
> + apr_uri_t urisock;
> + apr_status_t rv;
> + *ptr = '\0';
> + rv = apr_uri_parse(r->pool, ptr2, &urisock);
> + if (rv == APR_SUCCESS) {
> + char *sockpath = ap_runtime_dir_relative(r->pool,
> urisock.path);;
> + if (PROXY_STRNCPY((*worker)->s->uds_path,
> sockpath) != APR_SUCCESS) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
> APLOGNO(02597)
> + "worker uds path (%s) too long", sockpath);
> + }
> + r->filename = ptr+1; /* so we get the scheme
> for the uds */
> + }
> + else {
> + *ptr = '|';
> + }
> + }
> }
> }
> }
>
When multiple requests (simultaneously) hit the "reverse" worker, isn't
there an issue here regarding worker->s->uds_path?
If so, maybe you could use another r->notes instead to remember the UDS
path until a proxy_conn_rec is available (ie. conn->uds_path), like in the
following (and attached) patch.
Note about this patch: since no request_rec is available in
ap_proxy_acquire_connection(), conn->uds_path's (modified) initialisation
has been moved to ap_proxy_determine_connection(), which is also called in
all proxy modules except mod_proxy_ftp (though it could be too).
Regards,
Yann.
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1560505)
+++ modules/proxy/proxy_util.c (working copy)
@@ -1934,7 +1934,6 @@ PROXY_DECLARE(int) ap_proxy_pre_request(proxy_work
"*: found reverse proxy worker for %s",
*url);
*balancer = NULL;
*worker = conf->reverse;
- *(*worker)->s->uds_path = '\0';
access_status = OK;
/*
* The reverse worker does not keep connections alive, so
@@ -1956,10 +1955,12 @@ PROXY_DECLARE(int) ap_proxy_pre_request(proxy_work
rv = apr_uri_parse(r->pool, ptr2, &urisock);
if (rv == APR_SUCCESS) {
char *sockpath = ap_runtime_dir_relative(r->pool,
urisock.path);;
- if (PROXY_STRNCPY((*worker)->s->uds_path,
sockpath) != APR_SUCCESS) {
+ /* XXX: is that check (still) necessary? */
+ if (strlen(sockpath) >=
sizeof((*worker)->s->uds_path)) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
APLOGNO(02597)
"worker uds path (%s) too long", sockpath);
}
+ apr_table_setn(r->notes, "proxy-uds-path",
sockpath);
r->filename = ptr+1; /* so we get the scheme
for the uds */
}
else {
@@ -2125,28 +2126,6 @@ PROXY_DECLARE(int) ap_proxy_acquire_connection(con
(*conn)->close = 0;
(*conn)->inreslist = 0;
- if (*worker->s->uds_path) {
- if ((*conn)->uds_path == NULL) {
- /* use (*conn)->pool instead of worker->cp->pool to match
lifetime */
- (*conn)->uds_path = apr_pstrdup((*conn)->pool,
worker->s->uds_path);
- }
- if ((*conn)->uds_path) {
- ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02545)
- "%s: has determined UDS as %s",
- proxy_function, (*conn)->uds_path);
- }
- else {
- /* should never happen */
- ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02546)
- "%s: cannot determine UDS (%s)",
- proxy_function, worker->s->uds_path);
-
- }
- }
- else {
- (*conn)->uds_path = NULL;
- }
-
return OK;
}
@@ -2177,6 +2156,7 @@ ap_proxy_determine_connection(apr_pool_t *p, reque
int server_port;
apr_status_t err = APR_SUCCESS;
apr_status_t uerr = APR_SUCCESS;
+ const char *uds_path;
/*
* Break up the URL to determine the host to connect to
@@ -2192,6 +2172,17 @@ ap_proxy_determine_connection(apr_pool_t *p, reque
uri->port = ap_proxy_port_of_scheme(uri->scheme);
}
+ uds_path = worker->s->uds_path;
+ if (*uds_path || (uds_path = apr_table_get(r->notes,
"proxy-uds-path"))) {
+ /* use conn->pool instead of worker->cp->pool to match lifetime */
+ conn->uds_path = apr_pstrdup(conn->pool, uds_path);
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02545)
+ "UDS path determined as %s", conn->uds_path);
+ }
+ else {
+ conn->uds_path = NULL;
+ }
+
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00944)
"connecting %s to %s:%d", *url, uri->hostname, uri->port);
@@ -2217,7 +2208,7 @@ ap_proxy_determine_connection(apr_pool_t *p, reque
* to check host and port on the conn and be careful about
* spilling the cached addr from the worker.
*/
- if (!(*worker->s->uds_path) &&
+ if (!(conn->uds_path) &&
(!conn->hostname || !worker->s->is_address_reusable ||
worker->s->disablereuse)) {
if (proxyname) {
@@ -2257,7 +2248,7 @@ ap_proxy_determine_connection(apr_pool_t *p, reque
conn->port = uri->port;
}
socket_cleanup(conn);
- if (!(*worker->s->uds_path) &&
+ if (!(conn->uds_path) &&
(!worker->s->is_address_reusable || worker->s->disablereuse)) {
/*
* Only do a lookup if we should not reuse the backend address.
@@ -2269,7 +2260,7 @@ ap_proxy_determine_connection(apr_pool_t *p, reque
conn->pool);
}
}
- if (!(*worker->s->uds_path) && worker->s->is_address_reusable &&
!worker->s->disablereuse) {
+ if (!(conn->uds_path) && worker->s->is_address_reusable &&
!worker->s->disablereuse) {
/*
* Looking up the backend address for the worker only makes sense
if
* we can reuse the address.
[EOS]
Re: svn commit: r1560367 - in /httpd/httpd/trunk: docs/log-message-tags/next-number modules/mappers/mod_rewrite.c modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h modules/proxy/proxy_util.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jan 22, 2014, at 5:40 PM, Yann Ylavic <yl...@gmail.com> wrote:
> When multiple requests (simultaneously) hit the "reverse" worker, isn't there an issue here regarding worker->s->uds_path?
>
Yes...
FWIW, I tend to commit things in trunk as works-in-progress;
once it works for me and is stable in my testing, I commit.
That doesn't mean it's "fully baked" nor should it;