[squid-dev] [PATCH] pconn_lifetime

[Amos Jeffries]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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

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

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.

 - scope implied: all client (browser) connections


Christos patch is therefore implementing the _documented_ behaviour of
client_lifetime.


Sadly the actual current implementation of client_lifetime does
several undocumented and IMO wrong things.

What asked for is that those wrong things get moved to other
directives so they can still be controlled. And that client_lifetime
configure the new logic so that people using it already will actually
start to see it working as documented.

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

> 
>> * change your new directive "pconn_lifetime" to be 
>> "client_pconn_lifetime" since it is specifically scoped to client
>>  browser connections.
> 
> pconn_lifetime is not exclusive to client browser connections. 
> Ideally, it should apply to _all_ Squid connections that are in an 
> "idle persistent connection" state. We are not there yet, but we 
> already support origin connections AFAIK.

The patch code presented does none of what you say above.
 - it *is* limited only to client connections,
 - it is *not* limited to measuring idle connections,
 - it is *not* limited to persistent connections,


Ironically, client_lifetime directives current implementation does
what you are describing. It starts measuring from the beginning of
client connections idle period (violating the documented behaviour),
and gets cleared when the connection stops being idle. For server
connections it strangely starts counting from end of the idle period
when the request is sent until reply come back.


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

Christos description (and the patch) is applied to all client
connections. Without "idle", tunnels do indeed meet the description of
a "persistent connection" and the timout I'm mentioning above is the
one being applied on the client end of the tunnel.

In fact Christos patch sets pconn_lifetime timeout on tunnel
connections to measure the parse and setup, then reverts to using
client_lifetime "maximum connection time" to cover the tunnel idle
periods *individually* (violating the documented behaviour of both).


So do we at least have a consensus on terminology:

 That "lifetime" for a connection means from its opening
accept()/connect() to its close()  ??

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


Amos
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (MingW32)

iQEcBAEBAgAGBQJULuOSAAoJELJo5wb/XPRj33oH+gJWtnnL+ygHj4BhVejy3l/V
Nq2XdU16stxwIC5mqUHUi0WfEdG2V25qI7z8KtKEaqjk94hRx2iV+xaq1kZ+dhM2
sGvdlxm4J/9xLCc+vn6EHddhzEWo+x1zp3F0GuM3MjiEkA15r0q6M0KtzVUai5Rw
WUyPlJkQik/oLqO928WZwy9Yl6uJIjWYwNkOysf0/is7DgLfKKMkzOX5M+0MWbKU
8p3ElDCmzJKmteZ1K+xA1B+U8p5yQIT61sb2xgSQTr1YbvT115K2+0Za92qM6Lmj
uUoOw+JwPKBE3ymGmuWko6l6j+N47FW4ioTmyEoL+6Lel2WQH6nrd298NjnwNFI=
=Q2bq
-----END PGP SIGNATURE-----