Re: server-side pconn races no longer retried after r10057.1.8

On 09/20/2011 05:57 PM, Amos Jeffries wrote:
>
> Basing it on the error type/code/something would be better.
>
> There are three sets of cases feeding into this function (maybe more if
> other races exist).
> a) server error responses 5xx/403. new path required before
> connectStart().
> b) TCP connection failures. new path required before connectStart().
> c) this pconn race. same path can be passed back to connectStart() _once_.

Agreed, except the "can be" is actually "should or even must be" in (c).
It is wrong to serve an error to the user in this case.

> Cycles of repetition trying the same path on any of the (a) and (b)
> cases is a very bad idea for performance.
>
> At this point it should have (local.GetPort() > 0) from a previously
> open connection (c or a) [NP: fd will be -1 already]. So checking for
> the zero-sized error AND a present local port we could unset the local
> port, and call connectStart() without the shift().

A positive local.GetPort() check only tells us whether we had used a
connection, not whether we had reused a persistent connection, right?

How about adding an explicit flag.avoidPconn[NextTime] or similar and
setting it as sketched below?

FwdState::connectStart():
    ...
    fwdPconnPool->pop(..., !flag.avoidPconn && checkRetriable());
    flag.avoidPconn = we are reusing pconn;

And any time we change destination:

    serverDestinations.shift();
    flag.avoidPconn = false;

As the above sketch illustrates, the same flag can then be used in
FwdState::connectStart() to _avoid_ using a pconn if we are retrying the
same destination.

I do not like that we would have to remember to reset this flag every
time we shift the serverDestinations array. Perhaps that logic should be
moved into a dedicated FwdState method.

Thank you,

Alex.
Received on Wed Sep 21 2011 - 04:57:24 MDT