[squid-dev] [PATCH] pconn_lifetime

[Tsantilas Christos]

On 10/02/2014 08:24 AM, Amos Jeffries wrote:
> -----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).


My sense is that the client_lifetime is a timeout, which should 
triggered when the client waits for long time an HTTP or ICAP or DNS 
server to finish.
Probably should renamed to "client_wait_for_activity_timeout" and 
documentation should adapted. Also probably we need to fix it a little.

However it make sense to me a such timeout parameter. It is used in the 
case:
   "The web browser, waits 1 minute the dns server to respond, 1 minute 
helper to finish, 1 minute the ICAP server to respond, 2 minutes the 
HTTP server send first bytes of the answer, please close this connection 
I am not interested any more for the response..."

I should note that the pconn_lifetime has effect to server side 
connections too. And also try to not halt a served transaction. Only 
triggered after the transaction is finished, or while we are waiting for 
new requests.
It is a timeout used by administrators to resolve a problem without 
affecting active transactions.
There are major differences in use cases.


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

This is should fixed.

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

I think this change should not be done.
We may affect an valid existing request. If the timeLeft() is very 
short, we may have timeout while waiting data from helpers or ICAP server.

We need a client_wait_for_activity_timeout here.
This is what the "client_lifetime" now does.


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

So you are suggesting to use two configuration parameters here. One for 
client_side connections and one for server side.

We need to require a Lifetime value in Comm::Connection constructor, for 
this. It will make this patch a little big, but it can be implemented 
without huge changes.

But still I am not sure that we should use different parameters for 
server and client connections....


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