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

[Eduard Bagdasaryan]

On 16.03.2017 10:15, Amos Jeffries wrote:

 >in src/neighbours.cc:
 >
 > * peerConnectTimeout() should be a member of the CachePeer class yes?

The initial patch version did as you suggest, but then we decided
to use separate method instead, to avoid 'heavy' dependency on
SquidConfig.h (which periodically causes linkage problems, IIRC).


 > 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).

I do not mind renaming it, e.g., into 'creationTime'.


 > 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?

I am not against changing int to time_t there. As for 'max', how about
creating inside neighbors.cc a helper method:
time_t PositiveTimeout(const time_t timeout) { return max(1, timeout); }


 > 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.

Again, I am not against moving this method and ForwardTimeout() into
FwdState.cc (keeping related '*Forward' methods there makes some sense).



Eduard.