On 21/09/11 16:56, Alex Rousskov wrote:
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_.
Er, um, s/ _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.
Yes.
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?
Yes it tells us the conn was previously successful at opening. So I
believe it should be the distinguishing difference between (a) and (b)
cases. The ZERO_SIZED error differentiates between (c) and (a|b).
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;
The problem was the last pconn, we may have N perfectly fine ones to use
on the list.
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.
Yes, a flag is possible if we need to go that way. I think we can get
the same information from local port>0 for free without any state
maintenance overheads though. And we only need to use it at this one
point where the cases are clear.
Amos
--
Please be using
Current Stable Squid 2.7.STABLE9 or 3.1.15
Beta testers wanted for 3.2.0.12
