On 15.02.2012 07:40, Alex Rousskov wrote:
Hello,
The attached patch fixes the two patch bugs identified by Amos
earlier. However, I think one of the bugs should not be actually
fixed.
The discussion is below.
On 02/14/2012 09:24 AM, Alex Rousskov wrote:
On 02/14/2012 02:09 AM, Amos Jeffries wrote:
* You set it on IdleConnList::push, but missed its twin
ConnStateData::pinConnection() in client_side.cc
Will fix.
The attached patch sets wasIdle in pinConnection(), but I am not sure
that is a good thing. Setting wasIdle for pinned connections means
that
forward.cc is going to "retry" pconn races encountered on pinned
connections. This means that FwdState is not going to shift the
destinations array and will try to connect to the same PINNED peer
again.
However, AFAIK, current pinned connections cannot be re-opened and
re-pinned (bump-ssl-server-first code will do that, but we are
discussing general trunk changes only for now). I think if we set
wasIdle for pinned connections, then after the
ConnStateData::clientPinnedConnectionClosed() handler unpins the
failed
pinned connection from the request, the forward.cc code will hit an
assertion here:
if (serverDestinations[0]->peerType == PINNED) {
ConnStateData *pinned_connection =
request->pinnedConnection();
assert(pinned_connection);
Going forward, I see several options:
1. Do not set wasIdle in pinConnection(). Yes, the pinned connection
does become idle, but we cannot retry pinned pconn races so there is
little point in detecting and then doing extra work to ignore them.
2. Set wasIdle in pinConnection() (already done in the attached
patch).
In forward.cc, add code that will avoid retrying pinned pconn races.
This avoids the lie in pinConnection() but makes the forward.cc code
a
little more complex.
3. Set wasIdle in pinConnection() (already done in the attached
patch).
Leave forward.cc as is because my assertion analysis above is wrong
and
current code can retry pinned pconn races correctly.
I am not an expert on current pinned connection use. Which option is
the
best?
I think (2). Turning that assert into:
serverConn = (pinned_connection ?
pinned_connection->validatePinnedConnection(request,
serverDestinations[0]->getPeer()) : NULL);
Amos
