[squid-dev] [PATCH] pconn_lifetime

[Alex Rousskov]

On 10/03/2014 11:57 AM, Amos Jeffries wrote:
> On 4/10/2014 3:52 a.m., Alex Rousskov wrote:
>> On 10/01/2014 11:24 PM, Amos Jeffries wrote:
> 
>>> We already have client_lifetime directive which is documented
>>> as doing exactly what this new directive does. 
>>> (<http://www.squid-cache.org/Doc/config/client_lifetime/>).
> 
>> No, client_lifetime is not documented (and is not meant) to do
>> what we want pconn_lifetime to do. Client_lifetime is supposed to
>> kill client connections exceeding the configured lifetime,
>> _regardless_ of the client (and client connection) state. We do
>> not want that for pconn_lifetime which only affects _idle_
>> connections.
> 
> Lets have a look at that documentation then...
> 
> http://www.squid-cache.org/Doc/config/client_lifetime/ : " The
> maximum amount of time a client (browser) is allowed to remain
> connected to the cache process. "
> 
> - scope: "client (browser)"

Yes.


> And Christos patch description: " the desired maximum lifetime of a
> persistent connection. "

Yes. No "side" limitation here.


> Having read through the patch. What it does is *only* set a timer 
> running when the client connection is accepted, and limits (most) 
> timeouts applied later on that connection to be no more than the 
> configured total.

The patch should limit the _total_ pconn lifetime (but enforce that
limit only when Squid thinks the persistent connection is [going to
be] idle). Also, it should apply the same logic to origin server
connections that we pool. AFAICT, the patch does all of the above. If
it does not, there is a bug in the patch. More on that further below.

Please note that I use the term "idle from Squid point of view" as in
"may be closed without causing much harm". That is, it is an opposite
of "in the middle of handling some transaction". We could split hairs
arguing about some corner cases, but I hope we do not have to do that.


>> Moreover, pconn_lifetime is meant to apply to all connections,
>> not just the connections from a client.
> 
> Then Christos patch is seriously incomplete, because it only
> affects connections arriving from clients. The lifetime is never
> set on any of the other types of connection that make up "all".

AFAICT, the patch affects both HTTP user and origin connections. The
origin connections are affected by pconn.cc changes. Perhaps I
misinterpreted that change or you missed it?

I am not 100% sure, but I suspect those pconn.cc changes affect ICAP
connection as well.

As we discussed, the patch does not add new code to pool idle HTTP
user connections -- that big change is outside this patch scope, but
when/if that code is added, the same approach can be applied there
trivially.


>>> * update tunnel.cc client connection timeouts to use 
>>> conn->timeLeft(). - maybe with some new tunnel-specific
>>> lifetime config item. But I think your new config lifetime is
>>> appropriate for now at least.
> 
>> There is no concept of "idle persistent connection" for TCP
>> tunnels so we should not apply pconn_lifetime to them right now.
> 
> You added "idle" to that description. The code in question *does
> not* limit itself to idle connections, nor persistent connections.

I did not add that. The proposed feature should apply to "idle"
connections only, as documented: "When set, Squid will close a
now-idle persistent connection that ..."


> So do we at least have a consensus on terminology:
> 
> That "lifetime" for a connection means from its opening 
> accept()/connect() to its close()  ??

Yes, as documented in the patch: 'Connection lifetime is the time
period from the connection acceptance or opening time until "now"'.


> That the bits named FOO_timeout means some duration of that
> lifetime spent doing a "FOO" ??

Timeout and lifetime are very different things so I would not mix them
up like that, but yes, agents may do FOO during their lifetime (often
many times!).


Hope this clarifies,

Alex.