Fix an issue where destinations are compared against themselves

[tiramisuflavor]

The two calls to Random.Next may return the same index, so the comparison happens against the same destination. This behavior compromises the worst case protection this algorithm has because it might only use the worst destination, not offering opportunity of a better one. The fewer destinations we have, the more prominent this issue becomes.

[tiramisuflavor]

@microsoft-github-policy-service agree

[MihaZupan]

This seems like a reasonable change to make, thanks!

Can you please also add a test that checks this specific concern?
E.g.:

• you have 2 destinations, one with 1000 requests and the other with 0
• if you do 100 calls to PickDestination, they should all give you the destination with less load

The failing test is https://github.com/microsoft/reverse-proxy/blob/4700674a85b0fe9b0fd16be1de4034f626ceb1b9/test/ReverseProxy.Tests/LoadBalancing/LoadBalancingPoliciesTests.cs#L75

[MihaZupan]

Thanks

[Tratcher]

This fix suffers from the same issue as the original, a high collision rate in small sets. I think it'd be better to do a deterministic shift in those cases.

int secondIndex = random.Next(destinationCount);
if (secondIndex == firstIndex)
{
    secondIndex = (irstIndex + 1) % destinationCount;
}

I'll send a PR.

[Tratcher]

@MihaZupan convinced me that the +1 approach leads to non-trivial imbalance.

Even in the smallest case of two destinations where collisions will be common, the loop will only need to run a few times on average to break the tie. That's probably adequate.