[squid-dev] [PATCH] Honor peer timeouts when forwarding CONNECTs
squid3 at treenet.co.nz
Thu Mar 16 07:15:57 UTC 2017
On 13/03/2017 10:50 a.m., Eduard Bagdasaryan wrote:
> 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.
* 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
- when this is done the tunnel.cc inclusion of neighbours.h can be
* 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).
* "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 PeerPoolMgr::handleOpenedConnection() I see a line doing max(1,
(peerTimeout - timeUsed)); just like the code in FwdState.cc
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?
* 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
More information about the squid-dev