Re: [PREVIEW] Retry requests that failed due to a pconn race, take 1

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
Received on Tue Feb 14 2012 - 21:45:09 MST