On Fri, Jan 24, 2014 at 2:07 PM, Jim Jagielski <j...@jagunet.com> wrote:
> Hmmm from what I can see, if we have a hostname and the address > is re-usable, we do stuff we shouldn't. > The patch does nothing in the case you mention (like the original code), but I guess you meant "we have a hostnane and the address is *not* reusable". In which case the patch does nothing either, whereas the original code overwrittes the hostname, close the socket and then issues a new DNS lookup. That's indeed more than applying the De Morgan's law, but maybe I can explain why this is still a streamlining of the whole connections/adresses reuse logic. First let me stress that currently, !is_address_reusable is strictly equivalent to disablereuse, they imply the same paths everywhere. Whenever one or the other is in action (!is_address_reusable || disablereuse) the connection *and* address are never reused, and when both are not in action (is_address_reusable && !disablereuse) the connection *and* address are always reused. Disabling socket reuse only will still issue a DNS lookup for each request. Disabling address reuse only will still close the socket when done. Hence there is no distinction between connection and address reuse, the 2 flags could be merged into a single one, although it would make sense to keep both and make them do what they mean : 1. disablereuse: close the socket on release; ignore the flag on acquire (just compare conn->sock with NULL) 2. !is_address_reusable: resolve address on acquire (unconditionnaly), and if the socket is reused compare with its address (sockaddr_equal) and close it on mismatch; ignore the flag on release (just compare conn->hostname/addr with NULL) 3. the current behaviour is implied by 1. and 2. when is_address_reusable == !disablereuse Any thoughts? As you can see, my proposed patch is influenced by my current work on addresses' lifetime (DNS lookups), for which I feel quite unconfortable with the current/mixed "reuse" implementation. Back to the patch then... It indeed assumes that when the hostname is not NULL, the connection (address/socket) is reusable (otherwise it would have been cleared/zeroed on release). Hence it will never issue a DNS lookup nor socket close in that case (not cleared => reusable). Thus, the single/per-request DNS lookup is done only when hostname is NULL, and solely depends on worker's reuse parameters. I don't think it breaks anything, unless one (not httpd AFAICT) forces conn->hostname to something which is not to be taken into account... > r1560979 is my streamlining... > Yes, nice De Morgan's law. Do you plan to address my comment regarding conn->hostname's leak on http://mail-archives.apache.org/mod_mbox/httpd-dev/201401.mbox/%3ccakq1svpt_-+cndcgh95pt4adf30kn6+wjal3w15qorewapr...@mail.gmail.com%3E? > > Thx for the idea > Yw. I'll come back with the patch described above (distinct socket/address reuse) if it sounds applicable. Comments on this are welcome... Regards, Yann.
