[squid-dev] [PATCH] Honor peer timeouts when forwarding CONNECTs

[Amos Jeffries]

On 13/03/2017 10:50 a.m., Eduard Bagdasaryan wrote:
> Hello,
> 
> This patch fixes two bugs with tunneling CONNECT requests (or equivalent
> traffic) through a cache_peer:
> 
> 1. Not detecting dead cache_peers due to missing code to count peer
>    connect failures. SSL-level failures were detected (for "tls"
>    cache_peers) but TCP/IP connect(2) failures were not (for all peers).
> 
> 2. Origin server connect_timeout used instead of peer_connect_timeout or
>    a peer-specific connect-timeout=N (where configured).
> 
> The regular forwarding code path does not have the above bugs. This
> change reduces code duplication across the two code paths (that
> duplication probably caused these bugs in the first place), but a lot
> more work is needed in that direction.
> 
> This patch applies to v5 r15094. I assume v4 should be fixed as well:
> the same patch applies to v4 r14999.
> 

Yay. Thank you.

FYI: From the changes in tunnel.cc I suspect that this also fixes some
bugs in TOS/MARK on tunnels. The two code paths now calling
startConnecting() used to update one or other detail but not both - the
new method updates both always.
  I have not dug into the implications of that.



in src/neighbours.cc:

* peerConnectTimeout() should be a member of the CachePeer class yes?
 - the parameter is mandatory non-nil and it essentially produces a
property of that class state.
 - with the connect_timeout member a private this method would be its
accessor.
 - when this is done the tunnel.cc inclusion of neighbours.h can be
removed again.


in src/CachePeer.*:

* aftre teh above peerConnectTimeout() change we no longer need this
member to be public, so it can become provate and retain its exisign name.
 - the dump_peer_options() access to the member could check the
peerConnectTimeout() replacement accessor [see neighbours.cc below]
output was != the global Config value skip dispay if they match.
 - adding a setter as well would be necessary for the parse logic. (for
now, later a parser function would be better - maybe mark that as TODO).


in src/tunnel.cc:

* "start" is an action name and we use it (almost?) exclusively for Job
initiation. By comparison "started" means/implies more clearly a state
or time point. The Tunnel member stores a time point.
 - IMO both are bad, but "started" is better than "start". Better still
would be a name that actually describes *what* has been start(ed).


in src/PeerPoolMgr.cc:

* in PeerPoolMgr::handleOpenedConnection() I see a line doing max(1,
(peerTimeout - timeUsed)); just like the code in FwdState.cc
FwdState::connectDone().
 BUT, this PoolMgr is (wrongly) using int instead of time_t. Perhapse
that should be changed and something de-duplicate that max(1, x) usage?


in src/comm/Connection.cc:

* I dont see why EnoughTimeToReForward() should be in the Comm::
namespace. It is about messageing layer timing. So is more appropriate
to be in FwdState:: or maybe SquidTime.h/time.cc.
 - if you pick time.cc that would also mean the fairly generic time_t
static functions it calls would move to time.cc where they seem more
appropriate.


Cheers
Amos