[squid-dev] [PATCH] pconn_lifetime
squid3 at treenet.co.nz
Fri Oct 3 18:58:43 UTC 2014
-----BEGIN PGP SIGNED MESSAGE-----
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:
>>> 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).
> 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
> 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
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 ==
>> 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.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (MingW32)
-----END PGP SIGNATURE-----
More information about the squid-dev