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

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