[squid-dev] [PATCH] pconn_lifetime

[Amos Jeffries]

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

On 2/10/2014 5:23 a.m., Tsantilas Christos wrote:
> Ping....
> 
> So which is the decision? Can the patch go to trunk? Should the
> patch replaced with on other solution?
> 

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

However, what that directive *actually* does is very different from
the documented behaviour - it is also used for controlling
between-request idle timeout (breaking client_idle_pconn_timeout by
making it 1 week!) and also server request timout (pretending to be a
timeout for duration between sending request and starting to read reply).


I think extending it to a better solution for the above bugs and
scoping correctly.


1) In the existing client_side_reply.cc keep-alive conditions there
exists a line:

 else if (http->getConn() && http->getConn()->port->listenConn == NULL)

your patch retains it as:
  if (http->getConn()->port->listenConn == NULL)

 - this check is *supposed* to be the check for when, as you put it
"environments with long-lived connections where Squid configuration
... change during a single connection lifetime."
 - that is buggy. A check of !Comm::IsConnOpen() is what will
determine a live/dead listener socket.


2) There is more work to do to integrate it properly with the existing
pconn timeout and lifetime directives.

I request that we include these in this patch:

 * change ConnStateData::clientParseRequests() to use
conn->timeLeft(Config.Timeout.clientIdlePconn) when setting timeout
between requests.
 - this is another bug in the existing code that is retained by your
patch and needs fixing.

 * change your new directive "pconn_lifetime" to be
"client_pconn_lifetime" since it is specifically scoped to client
browser connections.
 - keeping or updating the client_lifetime documentation.

 * add some new directive (or find existing one that supposed to be
applied) for the server waiting-for-reply timout period.
 - use that instead of Config.Timeout.lifetime in src/http.cc

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

NP: at this point Config.Timeout.lifetime should not be used anywhere
in Squid and can be dropped.

 * check the SSL code where Config.Timeout.lifetime is mentioned in
comment to see if the timeout there is still relevant. Probably is but
needs checking and the comment updating.



Optional / future work:

A) this depends on having a matching/symmetrical server_pconn_lifetime
directive for the server connections.

Since there would be separate lifetimes for each 'side', I think we
should followup with a patch making a Comm::Connection::startTime_ be
initialized by Comm::TcpAcceptor() and Comm::ConnOpener() with the
relevant lifetime.

Then timeLeft() method does not need dependency on SquidConfig.h and
connection lifetimes will actually be predictable across a
reconfigure. At present a reconfigure changing lifetime will change
some connections but not others depending on whether they actually
receive a request between the reconfigure and one of the two old/new
lifetimes.

B) after the above we can also de-duplicate the timeLeft(X) calls to
one inside commSetConnTimeout().

Amos

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

iQEcBAEBAgAGBQJULOGPAAoJELJo5wb/XPRjTnsIAJtxQgW1OOGo+xfcgnEKGjlm
lTSJNOFClJCD/06FBV4ky6A2d0lqYOsHj4n3I5shh12UA3knHUNcWRCX4+Utnh41
O+ixrKKLLdjDoriyzV3p9vN9n7xlEvlTWjyPJ/qpSkZi95QbOKhkl1rkt/V+l67R
VTNHGzHXat6FISPfkNiscLkSG+fXGxXbZcXMn+TO+3wJtcFJZrb41qJGYuGYUZyN
MFKKEhyfvuvd+pIlEJuq9NO55oh+4k7jn6z9ySAsDZ44XuI59enjVlggAJxgMi9b
OqbNjxYL21d6pd1Mjr35+M8oNTG1Q8LtvRN+Bpm6mNclUbWeZAMuGrUp1mSZje0=
=G9Kt
-----END PGP SIGNATURE-----