Let me take a look... if all it doesn't is streamline the logic then it makes sense: +1
On Jan 23, 2014, at 6:35 PM, Yann Ylavic <[email protected]> wrote: > Hi, > > 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] > <httpd-trunk-mod_proxy_determine_connection.patch>
