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 _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
