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

[Amos Jeffries]

On 17/03/2017 2:11 a.m., Eduard Bagdasaryan wrote:
> 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).
> 

Hmm. Okay for now, but please add a TODO or XXX about that to the function.

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

Sounds good to me.

> 
>> 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); }
> 

Sounds good. I did a few tests to see if we could also avoid the
static_cast, but it seems not in any simple way. So dont forget that in
the new function.


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

Okay, I can live with that.

Amos