[squid-dev] [PATCH] pconn_lifetime

[Amos Jeffries]

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

On 3/10/2014 10:21 p.m., Tsantilas Christos wrote:
> 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.

We already have a lot of different *_timeout directives being used for
activities. Each one named for the activity being watched.

What I am hearing you requesting, and seeing in your code, is a total
since-opened timeout. Covering all actions on that connection.


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

Yes, I noted that, and agree it is a good decision.

> It is a timeout used by administrators to resolve a problem
> without affecting active transactions. There are major differences
> in use cases.
> 

There is a lot of similarity here with:
* request_timeout which counts down the period between accept() and
first request bytes arriving, and
* client_idle_pconn_timeout which counts down the period between requests.

Although both those count in time relative to the current idle
activity, instead of time since connection accept().

I note that you said this pconn_lifetime did not need coding for ICAP
or server connections since those were stored in Pconn/Idle pools. The
Idle pool timeouts only count from start of idle period, just like
client_idle_pconn_timeout.
 So either request_timeout/client_idle_pconn_timeout make
pconn_lifetime needless or pconn_lifetime needs some since-connect()
equivalent on server connections (and ICAP).


I think your choice of scoping "lifetime" as since-accept() is right.


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

Your patch is already setting keepalive=false when the pipeline may
have others still waiting responses, and in fact some of those may be
fully serviced by a server and only awaiting their reply data to be
written to the client.

If you want lifetime to have no bad side effects on a client
connection, then you need the timeout handler for it to set the client
connection to half-closed (no more reading) and let the existing
transactions (all of them) complete.

If you are doing the half-closed action, then it is better to set idle
timeout using timeLeft() in ConnStateData::clientParseRequests()
instead of playing with requests keep-alive flags.


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

No it waits for a random request and sets the keepalive flag to close.
Activity on other requests is *probably* pending.

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

Optional parameter I think. It will not be used everywhere just yet.

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

See my last post to Alex.

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

iQEcBAEBAgAGBQJULvHjAAoJELJo5wb/XPRjMN0H/04IqpUy5FxNwp9XrYIYkyFh
V9iwzGyz4VeyCVFrRw57mAPtS+Wf8TIbCMhQtWjpM1jeMiaL3T3ljpr+YmTnYplK
NIOJBcBNMu2/HWpMiGlqffk9wtPhGJJEcRhsdut05AOD0QPCJYUIhR17JZO0aecL
z2mnoITfK60e5sD1B1iqRRXR+D98LqZl4/ykOVhmxv6riqZzOe4Hy3qglZpvI4f8
z+I382AbDa86iapDOM9oBttK2K8ut1+CSlk2hhEVHme3ZJTJqPvuyWkpUbUiniNb
vNWaaHDZZNxwk9EUHx94nCgLP0SSI2OLTpud0lsUTHRAd8OQ3SPuq1uYnYrXMEw=
=If6c
-----END PGP SIGNATURE-----