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
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
