In Socket:connect, start connecting to next address, after 250 (25 for loopback).

In Socket:connect, start connecting to next address, after 250 (25 for loopback).

BUG=https://code.google.com/p/dart/issues/detail?id=19871
R=whesse@google.com

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

[Bill Hesse, 2014-07-29 10:57:07]

lgtm

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

https://codereview.chromium.org/429543002/diff/1/runtime/bin/socket_patch.dar...
runtime/bin/socket_patch.dart:403: if (list.length == 0) {
Maybe list should be "addresses" here.  Use isNotEmpty?

https://codereview.chromium.org/429543002/diff/1/runtime/bin/socket_patch.dar...
runtime/bin/socket_patch.dart:411: assert(addresses.isNotEmpty);
This assert is covered by the check in the above function.

https://codereview.chromium.org/429543002/diff/1/runtime/bin/socket_patch.dar...
runtime/bin/socket_patch.dart:442: connecting.add(connectingSocket);
If you used a Map, rather than a Set, then this would just be connecting[socket]
= timer, and the connectingSocket class would disappear.  I don't think the
other code would get more complicated - abort could still be encapsulated as a
closure, binding connecting.

But you can keep it a set.

[Anders Johnsen, 2014-07-29 11:23:27]

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

https://codereview.chromium.org/429543002/diff/1/runtime/bin/socket_patch.dar...
runtime/bin/socket_patch.dart:403: if (list.length == 0) {
On 2014/07/29 10:57:06, Bill Hesse wrote:
> Maybe list should be "addresses" here.  Use isNotEmpty?

Done.

https://codereview.chromium.org/429543002/diff/1/runtime/bin/socket_patch.dar...
runtime/bin/socket_patch.dart:411: assert(addresses.isNotEmpty);
On 2014/07/29 10:57:06, Bill Hesse wrote:
> This assert is covered by the check in the above function.

Done.

https://codereview.chromium.org/429543002/diff/1/runtime/bin/socket_patch.dar...
runtime/bin/socket_patch.dart:442: connecting.add(connectingSocket);
On 2014/07/29 10:57:06, Bill Hesse wrote:
> If you used a Map, rather than a Set, then this would just be
connecting[socket]
> = timer, and the connectingSocket class would disappear.  I don't think the
> other code would get more complicated - abort could still be encapsulated as a
> closure, binding connecting.
> 
> But you can keep it a set.

Acknowledged.

[Anders Johnsen, 2014-07-29 11:27:06]

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

[Søren Gjesse, 2014-08-04 09:21:37]

dbc

Also the dartdoc comment for connect is not fully correct any more.

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

https://codereview.chromium.org/429543002/diff/20001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:421: // Set up timer for when we should retry the
next address (if any).
Long line.

https://codereview.chromium.org/429543002/diff/20001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:437: s.close();
Shouldn't s.destroy() be enough?

[Anders Johnsen, 2014-08-05 09:14:55]

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

https://codereview.chromium.org/429543002/diff/20001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:421: // Set up timer for when we should retry the
next address (if any).
On 2014/08/04 09:21:37, Søren Gjesse wrote:
> Long line.

Done.

https://codereview.chromium.org/429543002/diff/20001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:437: s.close();
On 2014/08/04 09:21:37, Søren Gjesse wrote:
> Shouldn't s.destroy() be enough?

There is no destroy on _NativeSocket. We need to clean it up nicely.