[squid-dev] [PATCH] Do not revive unconditionally dead peers after DNS refresh

[Alex Rousskov]

On 04/27/2017 02:39 PM, Eduard Bagdasaryan wrote:
> +    // always start probing in order to effectively detect
> +    // dead or revived peers
> +    (void)peerProbeConnect(p);

I think we can simplify that comment while making it more precise:

    peerProbeConnect(p); // detect any died or revived peers ASAP

The (void) cast was correct, but is no longer needed after v5 r15133.

The above are minor polishing touches that could be done during commit,
but I am writing this message because I think we may be able to cover
one more corner case without significant changes...

Imagine a situation where the peers are already being probed during
peerDNSConfigure(). Patches Squid will do nothing nothing in this case.
However, the current probes may be using old/wrong IP addresses (or
other peer configuration parameters) and, hence, may produce wrong
result. This lack of a fresh probe may delay the discovery of a died or
revived peer and might even cause an HTTP transaction failure (in the
"the peer has recently died" case).

On the other hand, we still do not want to create too many concurrent or
too frequent probes so we want to continue to honor the current
peerProbeConnect() concurrency and rate limits.

Please see if the following small change allows Squid to handle the
above case better:

1. Add a boolean CachePeer::reprobe field, defaulted to false. It can be
described as "whether to do another TCP probe after the current TCP probes".

2. peerProbeConnect() should clear the reprobe field after passing the
two if-statement guards and before entering the loop (even if there are
no IPs to probe). The right timing here would eliminate any implicit
reprobing loops while providing the desired functionality.

3. After closing the connection, peerProbeConnectDone() should call
peerProbeConnect() if reprobe is true.

4. Add a boolean reprobeIfBusy parameter to peerProbeConnect() with
false as the default value. peerDNSConfigure() will call
peerProbeConnect() with a true parameter value. peerProbeConnect() will
start by setting reprobe to (reprobe || reprobeIfBusy).

There may be more initialization/reporting/cloning steps needed, just
like for any other CachePeer data member, but the above reflects the
core logic. Adjust as needed, of course.


Thank you,

Alex.