Hmmm from what I can see, if we have a hostname and the address is re-usable, we do stuff we shouldn't.
r1560979 is my streamlining... Thx for the idea On Jan 24, 2014, at 7:19 AM, Jim Jagielski <[email protected]> wrote: > 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> >
