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

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?

Thank you,

Alex.

> In other words, we need a flag that helps us detect failed connections
> that went through the following sequence of "states":
>
> * idle
> * eof
>
> The new wasIdle flag keeps the first state, and Server kids clear that
> flag if they receive something from the server because it means the eof
> will not be reached immediately after the idle state. I do not see how
> we can avoid Server kid modifications, but it would be great if we can,
> of course.
>
> I do not like the wasIdle name because, technically, the connection was
> not idle after we started writing the request to the server, but I
> failed to find a better name. Suggestions welcomed!