Add support for multiple HTTP proxies

Add support for multiple HTTP proxies

If the configuration returned from the findProxy in HttpClient
contails a list of proxies the the entries in the list will be tried
one by one until a working one is found. Before only the first was
ever tried.

R=ajohnsen@google.com

BUG=dart:5468
TEST=tests/standalone/io/http_proxy_test.dart

Committed: https://code.google.com/p/dart/source/detail?r=13599

[Anders Johnsen, 2012-10-12 06:40:14]

LGTM, nice! :)

https://codereview.chromium.org/11092044/diff/1/runtime/bin/http_impl.dart
File runtime/bin/http_impl.dart (right):

https://codereview.chromium.org/11092044/diff/1/runtime/bin/http_impl.dart#ne...
runtime/bin/http_impl.dart:2112: void _establishConnection(String host,
Should we consider moving this outside of _prepareHttpClientConnection? This is
getting quite nested.

https://codereview.chromium.org/11092044/diff/1/runtime/bin/http_impl.dart#ne...
runtime/bin/http_impl.dart:2155: if (proxyIndex <
proxyConfiguration.proxies.length) {
An alternative would be to move this line to the start of the recursive
function. Then any caller (if others come one day), will have the correct
behavior if the index is invalid.

https://codereview.chromium.org/11092044/diff/1/runtime/bin/http_impl.dart#ne...
runtime/bin/http_impl.dart:2169: socket.onError = null;
Not knowing much about http proxy, is the spec saying that a bad connection is
the only time we should try other? Ie. is there a HTTP error code that would
mean we should use the next proxy? If that is that case, we should add a TODO.

[Søren Gjesse, 2012-10-12 08:15:37]

Thanks for the review.

https://codereview.chromium.org/11092044/diff/1/runtime/bin/http_impl.dart
File runtime/bin/http_impl.dart (right):

https://codereview.chromium.org/11092044/diff/1/runtime/bin/http_impl.dart#ne...
runtime/bin/http_impl.dart:2112: void _establishConnection(String host,
On 2012/10/12 06:40:14, ajohnsen wrote:
> Should we consider moving this outside of _prepareHttpClientConnection? This
is
> getting quite nested.

I was also considering that, but then the argument list would need to include
the arguments passed to _prepareHttpClientConnection above so I decided against
it.

https://codereview.chromium.org/11092044/diff/1/runtime/bin/http_impl.dart#ne...
runtime/bin/http_impl.dart:2155: if (proxyIndex <
proxyConfiguration.proxies.length) {
On 2012/10/12 06:40:14, ajohnsen wrote:
> An alternative would be to move this line to the start of the recursive
> function. Then any caller (if others come one day), will have the correct
> behavior if the index is invalid.

The problem with that approach is that the actual connection exception is not
available at the beginning of the function. Added an assert that proxy index is
valid as this in an internal invariant.

https://codereview.chromium.org/11092044/diff/1/runtime/bin/http_impl.dart#ne...
runtime/bin/http_impl.dart:2169: socket.onError = null;
On 2012/10/12 06:40:14, ajohnsen wrote:
> Not knowing much about http proxy, is the spec saying that a bad connection is
> the only time we should try other? Ie. is there a HTTP error code that would
> mean we should use the next proxy? If that is that case, we should add a TODO.

There is no "real" standard here. The page
http://findproxyforurl.com/netscape-documentation/ only talks about
"unresponsive proxy" and "proxy goes down". In that respect I think we are fine,
but there might be other rules implemented in browsers that I don't know about.