[squid-dev] [PATCH] pconn_lifetime

Amos Jeffries squid3 at treenet.co.nz
Thu Oct 2 05:24:31 UTC 2014

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.

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

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


Version: GnuPG v2.0.22 (MingW32)


More information about the squid-dev mailing list