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

[Eduard Bagdasaryan]

Adjusted the patch accordingly.


Eduard.


On 22.03.2017 15:58, Amos Jeffries wrote:
> 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: SQUID-279-honor-peer-timeouts-connect-fwd-t2.patch
Type: text/x-patch
Size: 60199 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20170326/a1d7d8fd/attachment-0001.bin>