Make Socket::connect try all addresses given.

Make Socket::connect try all addresses given.

BUG=
R=sgjesse@google.com

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

[Søren Gjesse, 2014-04-30 13:11:55]

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

https://codereview.chromium.org/259353003/diff/1/runtime/bin/socket_patch.dar...
runtime/bin/socket_patch.dart:485: if (host is List) {
We don't want to support a List here.

https://codereview.chromium.org/259353003/diff/1/runtime/bin/socket_patch.dar...
runtime/bin/socket_patch.dart:510: completer.completeError(error);
This will just return the error from the last one which failed, but I guess
there is nothing better we can do.

https://codereview.chromium.org/259353003/diff/1/sdk/lib/io/socket.dart
File sdk/lib/io/socket.dart (right):

https://codereview.chromium.org/259353003/diff/1/sdk/lib/io/socket.dart#newco...
sdk/lib/io/socket.dart:390: * [host] can either be a [String], an
[InternetAddress] or a [List] of
we don't want to support a list of InternetAddress here. If a DNS-lookup happens
it will internally do this, but it should not be something the user can specify
directly. If one wants to do a DNS-lookup manually and want to try all or some
of the addresses that must be done with manual calls to connect.

[Ivan Posva, 2014-04-30 16:28:06]

DBC -ip

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

https://codereview.chromium.org/259353003/diff/1/runtime/bin/socket_patch.dar...
runtime/bin/socket_patch.dart:510: completer.completeError(error);
On 2014/04/30 13:11:56, Søren Gjesse wrote:
> This will just return the error from the last one which failed, but I guess
> there is nothing better we can do.

You could collect them all in a List and have a multi error object that you set
in this case.

https://codereview.chromium.org/259353003/diff/1/sdk/lib/io/socket.dart
File sdk/lib/io/socket.dart (right):

https://codereview.chromium.org/259353003/diff/1/sdk/lib/io/socket.dart#newco...
sdk/lib/io/socket.dart:394: * was not possibly to any of the
[InternetAddress]es, the error from the last
possibly -> possible

Or you might reword it a bit to say that you return the last error unless a
successful connection was made.

https://codereview.chromium.org/259353003/diff/1/sdk/lib/io/socket.dart#newco...
sdk/lib/io/socket.dart:488: * was not possibly to any of the
[InternetAddress]es, the error from the last
ditto

[Anders Johnsen, 2014-05-01 06:31:22]

PTAL

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

https://codereview.chromium.org/259353003/diff/1/runtime/bin/socket_patch.dar...
runtime/bin/socket_patch.dart:485: if (host is List) {
On 2014/04/30 13:11:56, Søren Gjesse wrote:
> We don't want to support a List here.

Done.

https://codereview.chromium.org/259353003/diff/1/runtime/bin/socket_patch.dar...
runtime/bin/socket_patch.dart:510: completer.completeError(error);
On 2014/04/30 16:28:06, Ivan Posva wrote:
> On 2014/04/30 13:11:56, Søren Gjesse wrote:
> > This will just return the error from the last one which failed, but I guess
> > there is nothing better we can do.
> 
> You could collect them all in a List and have a multi error object that you
set
> in this case.

It's just hard to fit in our current model, without breaking it. Thinking about
it, we should actually return the first error. That's the one you would 'expect'
to get.

https://codereview.chromium.org/259353003/diff/1/sdk/lib/io/socket.dart
File sdk/lib/io/socket.dart (right):

https://codereview.chromium.org/259353003/diff/1/sdk/lib/io/socket.dart#newco...
sdk/lib/io/socket.dart:390: * [host] can either be a [String], an
[InternetAddress] or a [List] of
On 2014/04/30 13:11:56, Søren Gjesse wrote:
> we don't want to support a list of InternetAddress here. If a DNS-lookup
happens
> it will internally do this, but it should not be something the user can
specify
> directly. If one wants to do a DNS-lookup manually and want to try all or some
> of the addresses that must be done with manual calls to connect.

Done.

https://codereview.chromium.org/259353003/diff/1/sdk/lib/io/socket.dart#newco...
sdk/lib/io/socket.dart:394: * was not possibly to any of the
[InternetAddress]es, the error from the last
On 2014/04/30 16:28:06, Ivan Posva wrote:
> possibly -> possible
> 
> Or you might reword it a bit to say that you return the last error unless a
> successful connection was made.

Done.

https://codereview.chromium.org/259353003/diff/1/sdk/lib/io/socket.dart#newco...
sdk/lib/io/socket.dart:488: * was not possibly to any of the
[InternetAddress]es, the error from the last
On 2014/04/30 16:28:06, Ivan Posva wrote:
> ditto

Done.

[Søren Gjesse, 2014-05-01 07:23:34]

lgtm

https://codereview.chromium.org/259353003/diff/20001/runtime/bin/socket_patch...
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/259353003/diff/20001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:520: socket.close();
Add a comment that this keeps the first error.

[Anders Johnsen, 2014-05-01 08:42:38]

https://codereview.chromium.org/259353003/diff/20001/runtime/bin/socket_patch...
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/259353003/diff/20001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:520: socket.close();
On 2014/05/01 07:23:34, Søren Gjesse wrote:
> Add a comment that this keeps the first error.

Done.

[Anders Johnsen, 2014-05-01 08:43:04]

Committed patchset #2 manually as r35619 (presubmit successful).

[codefu, 2014-05-01 18:52:02]

https://codereview.chromium.org/259353003/diff/20001/runtime/bin/socket_patch...
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/259353003/diff/20001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:521: run(error != null ? error : e);
If addresses.length > 1 and the first address connects... if later in time the
socket has an error won't this try to reconnect on later addresses (and possibly
throw an error since completer is already done)?

suggested: bool retry = true; before run(), then retry = false just before
comleter.complete(socket).  in error if(retry) run(...);

[Anders Johnsen, 2014-05-01 19:27:58]

https://codereview.chromium.org/259353003/diff/20001/runtime/bin/socket_patch...
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/259353003/diff/20001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:521: run(error != null ? error : e);
On 2014/05/01 18:52:02, codefu wrote:
> If addresses.length > 1 and the first address connects... if later in time the
> socket has an error won't this try to reconnect on later addresses (and
possibly
> throw an error since completer is already done)?
> 
> suggested: bool retry = true; before run(), then retry = false just before
> comleter.complete(socket).  in error if(retry) run(...);

No, this error handler is only called when connecting fails. As soon as the
connection is made, we disable listening (see above) and later register another
error handler.