H
i,
I'm currently working on a patch to be able to configure the lifetime (TTL)
of mod_proxy's DNS lookups (I know about graceful restarts, but that's not
always an option).
I'll propose it when it'll work (al least for me), should it be of any
interest.
The modified code is mainly in ap_proxy_determine_connection (that's why I
may look picky about your UDS' modifications there Jim ;)
No offense, ap_proxy_determine_connection() is quite hard to follow
regarding the init/reuse of connection's hostname/uds_path/address (when
[and where] to do [single] DNS lookups) depending on worker's params and/or
forward/reverse/UDS...
My point is not (at all) to blame anyone (of course, keep the good work!),
I guess the reason is that it has been adapted progressively with new
features/fixes, as always.
I propose the following/attached patch, which rearranges the code in this
way :
- UDS branch first where all relative (specific) inits/reuses are done, or
else
- hostname+port init/reuse (respectively if NULL or not, since they are
always cleared on cleanup if not reusable), and then
- address (single)lookup/reuse depending solely on worker's params
(is_address_reusable/disablereuse).
All the (possible) paths look identical to the previous ones, but these
different actions are not mixed together as before, hence IMHO the code is
clearer (and better fits my needs, although it's not necessarily your
concern).
Maybe I should propose this when
my patch is
fully
booked (and so is UDS), but since the code there is still hot, it looks
like a good reason to refactor it a bit.
I just take my chance...
Regards,
Yann.
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1560806)
+++ modules/proxy/proxy_util.c (working copy)
@@ -2218,11 +2218,23 @@ ap_proxy_determine_connection(apr_pool_t *p, reque
uri->scheme, uds_path);
}
+ /*
+ * In UDS cases, some structs are NULL. Protect from de-refs
+ * and provide info for logging at the same time.
+ */
+ if (!conn->addr) {
+ apr_sockaddr_t *sa;
+ apr_sockaddr_info_get(&sa, NULL, APR_UNSPEC, 0, 0, conn->pool);
+ conn->addr = sa;
+ }
+ conn->hostname = "httpd-UDS";
+ conn->port = 0;
}
- if (!(uds_path) &&
- (!conn->hostname || !worker->s->is_address_reusable ||
- worker->s->disablereuse)) {
- if (proxyname) {
+ else {
+ if (conn->hostname) {
+ /* reuse hostname and port */
+ }
+ else if (proxyname) {
conn->hostname = apr_pstrdup(conn->pool, proxyname);
conn->port = proxyport;
/*
@@ -2258,9 +2270,7 @@ ap_proxy_determine_connection(apr_pool_t *p, reque
conn->hostname = apr_pstrdup(conn->pool, uri->hostname);
conn->port = uri->port;
}
- socket_cleanup(conn);
- if (!(*worker->s->uds_path) &&
- (!worker->s->is_address_reusable || worker->s->disablereuse)) {
+ if (!worker->s->is_address_reusable || worker->s->disablereuse) {
/*
* Only do a lookup if we should not reuse the backend address.
* Otherwise we will look it up once for the worker.
@@ -2269,29 +2279,18 @@ ap_proxy_determine_connection(apr_pool_t *p, reque
conn->hostname, APR_UNSPEC,
conn->port, 0,
conn->pool);
+ /* Ask to close the socket below */
+ conn->close = 1;
}
- }
- else {
/*
- * In UDS cases, some structs are NULL. Protect from de-refs
- * and provide info for logging at the same time.
- */
- apr_sockaddr_t *sa;
- apr_sockaddr_info_get(&sa, NULL, APR_UNSPEC, 0, 0, conn->pool);
- conn->hostname = "httpd-UDS";
- conn->addr = sa;
- }
- if (!(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.
*/
- if (!worker->cp->addr) {
+ else if (!worker->cp->addr) {
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;
}
-
/*
* Worker can have the single constant backend adress.
* The single DNS lookup is used once per worker.
[EOS]
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1560806)
+++ modules/proxy/proxy_util.c (working copy)
@@ -2218,11 +2218,23 @@ ap_proxy_determine_connection(apr_pool_t *p, reque
uri->scheme, uds_path);
}
+ /*
+ * In UDS cases, some structs are NULL. Protect from de-refs
+ * and provide info for logging at the same time.
+ */
+ if (!conn->addr) {
+ apr_sockaddr_t *sa;
+ apr_sockaddr_info_get(&sa, NULL, APR_UNSPEC, 0, 0, conn->pool);
+ conn->addr = sa;
+ }
+ conn->hostname = "httpd-UDS";
+ conn->port = 0;
}
- if (!(uds_path) &&
- (!conn->hostname || !worker->s->is_address_reusable ||
- worker->s->disablereuse)) {
- if (proxyname) {
+ else {
+ if (conn->hostname) {
+ /* reuse hostname and port */
+ }
+ else if (proxyname) {
conn->hostname = apr_pstrdup(conn->pool, proxyname);
conn->port = proxyport;
/*
@@ -2258,9 +2270,7 @@ ap_proxy_determine_connection(apr_pool_t *p, reque
conn->hostname = apr_pstrdup(conn->pool, uri->hostname);
conn->port = uri->port;
}
- socket_cleanup(conn);
- if (!(*worker->s->uds_path) &&
- (!worker->s->is_address_reusable || worker->s->disablereuse)) {
+ if (!worker->s->is_address_reusable || worker->s->disablereuse) {
/*
* Only do a lookup if we should not reuse the backend address.
* Otherwise we will look it up once for the worker.
@@ -2269,29 +2279,18 @@ ap_proxy_determine_connection(apr_pool_t *p, reque
conn->hostname, APR_UNSPEC,
conn->port, 0,
conn->pool);
+ /* Ask to close the socket below */
+ conn->close = 1;
}
- }
- else {
/*
- * In UDS cases, some structs are NULL. Protect from de-refs
- * and provide info for logging at the same time.
- */
- apr_sockaddr_t *sa;
- apr_sockaddr_info_get(&sa, NULL, APR_UNSPEC, 0, 0, conn->pool);
- conn->hostname = "httpd-UDS";
- conn->addr = sa;
- }
- if (!(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.
*/
- if (!worker->cp->addr) {
+ else if (!worker->cp->addr) {
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;
}
-
/*
* Worker can have the single constant backend adress.
* The single DNS lookup is used once per worker.
