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.
