Fix flaky timeouts of socket_ipv6_test.

Fix flaky timeouts of socket_ipv6_test.

It seems there is no way of guaranteeing that there is always an error in those two cases, because it depends on other processes running on the system.

Related to #28046.

R=kustermann@google.com

Committed: https://github.com/dart-lang/sdk/commit/8cb4b5adfb97a8aa564a3ba21a547fccb35e6df3

[Florian Schneider, 2016-12-13 19:22:22]

Description was changed from

==========
Fix flaky timeouts of socket_ipv6_test.

It seems there is no way of guaranteeing that there is always an error,
because this test also depens on other processes running on the system.

Related to #28046.
==========

to

==========
Fix flaky timeouts of socket_ipv6_test.

It seems there is no way of guaranteeing that there is always an error in those
two cases, because it depends on other processes running on the system.

Related to #28046.
==========

[Florian Schneider, 2016-12-13 19:24:25]

fschneider@google.com changed reviewers:
+ kustermann@google.com

[Florian Schneider, 2016-12-13 19:24:26]

Not sure this is the right fix - But it seems we can't expect an error always
here. The futures complete successfully with a small probability and the main
isolate blocks because asyncEnd in not executed.

[kustermann, 2016-12-13 19:36:45]

https://codereview.chromium.org/2566273007/diff/1/tests/standalone/io/socket_...
File tests/standalone/io/socket_ipv6_test.dart (right):

https://codereview.chromium.org/2566273007/diff/1/tests/standalone/io/socket_...
tests/standalone/io/socket_ipv6_test.dart:54: Expect.equals('127.0.0.1',
server.address.host);
Maybe indent this here for readability

https://codereview.chromium.org/2566273007/diff/1/tests/standalone/io/socket_...
tests/standalone/io/socket_ipv6_test.dart:62: asyncEnd();
If you add this then the test is disabled (same below), a wrong dart:io
implementation will pass this test now.

I think retrying is better in these cases (as with the 2 test I've already
fixed).

[Florian Schneider, 2016-12-13 20:23:16]

https://codereview.chromium.org/2566273007/diff/1/tests/standalone/io/socket_...
File tests/standalone/io/socket_ipv6_test.dart (right):

https://codereview.chromium.org/2566273007/diff/1/tests/standalone/io/socket_...
tests/standalone/io/socket_ipv6_test.dart:54: Expect.equals('127.0.0.1',
server.address.host);
On 2016/12/13 19:36:45, kustermann wrote:
> Maybe indent this here for readability

Done.

https://codereview.chromium.org/2566273007/diff/1/tests/standalone/io/socket_...
tests/standalone/io/socket_ipv6_test.dart:62: asyncEnd();
On 2016/12/13 19:36:45, kustermann wrote:
> If you add this then the test is disabled (same below), a wrong dart:io
> implementation will pass this test now.
> 
> I think retrying is better in these cases (as with the 2 test I've already
> fixed).

Done.

[kustermann, 2016-12-13 21:04:30]

https://codereview.chromium.org/2566273007/diff/60001/tests/standalone/io/soc...
File tests/standalone/io/socket_ipv6_test.dart (right):

https://codereview.chromium.org/2566273007/diff/60001/tests/standalone/io/soc...
tests/standalone/io/socket_ipv6_test.dart:64: throw "Unexpected connect";
This throw will be caught by the next line and silently swallowed.

I think you could fix it by adding a test closureÆ

  .catchError((e) { ... }, test: (exception) => exception is! String)

Or what would be even better:

Socket.connect(...).then((socket) {
  socket.destroy();
  throw 'Unexpected connect';
}).catchError((e) {
}, test: (exception) => exception is! String).whenComplete(() {
  server.close();
  asyncEnd();
});

The `whenComplete()` is like a finally block.

https://codereview.chromium.org/2566273007/diff/60001/tests/standalone/io/soc...
tests/standalone/io/socket_ipv6_test.dart:134: });
same here.

https://codereview.chromium.org/2566273007/diff/60001/tests/standalone/io/soc...
tests/standalone/io/socket_ipv6_test.dart:147:
retrySync(testIPv4toIPv6_IPV6Only);  /// 07: ok
You can't use retrySync, because these functions are async (they return "void",
but they do not run synchronously).

Maybe the best way would be to just use await/async syntax, make them return a
Future, and use the existing retry method.

[Florian Schneider, 2016-12-13 22:12:33]

https://codereview.chromium.org/2566273007/diff/60001/tests/standalone/io/soc...
File tests/standalone/io/socket_ipv6_test.dart (right):

https://codereview.chromium.org/2566273007/diff/60001/tests/standalone/io/soc...
tests/standalone/io/socket_ipv6_test.dart:64: throw "Unexpected connect";
On 2016/12/13 21:04:30, kustermann wrote:
> This throw will be caught by the next line and silently swallowed.
> 
> I think you could fix it by adding a test closureÆ
> 
>   .catchError((e) { ... }, test: (exception) => exception is! String)
> 
> Or what would be even better:
> 
> Socket.connect(...).then((socket) {
>   socket.destroy();
>   throw 'Unexpected connect';
> }).catchError((e) {
> }, test: (exception) => exception is! String).whenComplete(() {
>   server.close();
>   asyncEnd();
> });
> 
> The `whenComplete()` is like a finally block.

Thanks! Does Socket.connect ever throw a String?

I'll try use onError instead. Seems more concise as well.

https://codereview.chromium.org/2566273007/diff/60001/tests/standalone/io/soc...
tests/standalone/io/socket_ipv6_test.dart:134: });
On 2016/12/13 21:04:30, kustermann wrote:
> same here.

Done. Reformatted here as well to match the other tests.

https://codereview.chromium.org/2566273007/diff/60001/tests/standalone/io/soc...
tests/standalone/io/socket_ipv6_test.dart:147:
retrySync(testIPv4toIPv6_IPV6Only);  /// 07: ok
On 2016/12/13 21:04:30, kustermann wrote:
> You can't use retrySync, because these functions are async (they return
"void",
> but they do not run synchronously).
> 
> Maybe the best way would be to just use await/async syntax, make them return a
> Future, and use the existing retry method.

Aaah. Async code. The void confusd me.

Done. (Strangely, tests were passing with retrySync, but maybe I didn't hit a
flaky failure)

[kustermann, 2016-12-13 22:22:39]

https://codereview.chromium.org/2566273007/diff/140001/tests/standalone/io/so...
File tests/standalone/io/socket_ipv6_test.dart (right):

https://codereview.chromium.org/2566273007/diff/140001/tests/standalone/io/so...
tests/standalone/io/socket_ipv6_test.dart:18:
ServerSocket.bind(serverAddr.first, 0).then((server) {
InternetAddress.lookup()
ServerSocket.bind()
Socket.connect()

They all return futures as well you have to wait for.

I think it would be much easier to read if we used async/await:

Future testIPv6ToIPv6() async {
  var serverAddr = await InternetAddress.lookup(...);
  var clientAddr = await InternetAddress.lookup(...);
  var server = await ServerSocket.bind(...);
  try {
    Expect.equals(...);
    Expect.equals(...);
    server.listen((socket) {...});
    var clientSocket = await Socket.connect(..);
    await clientSocket.close();
  } finally {
    server.close();
  }
}

https://codereview.chromium.org/2566273007/diff/140001/tests/standalone/io/so...
tests/standalone/io/socket_ipv6_test.dart:132: void main() {
Could use

main() async {
  await test...();
}

[kustermann, 2016-12-13 22:55:29]

https://codereview.chromium.org/2566273007/diff/60001/tests/standalone/io/soc...
File tests/standalone/io/socket_ipv6_test.dart (right):

https://codereview.chromium.org/2566273007/diff/60001/tests/standalone/io/soc...
tests/standalone/io/socket_ipv6_test.dart:64: throw "Unexpected connect";
> Thanks! Does Socket.connect ever throw a String?

I don't think so. It's mostly some [SocketException] I think.

[Florian Schneider, 2016-12-13 23:03:20]

https://codereview.chromium.org/2566273007/diff/140001/tests/standalone/io/so...
File tests/standalone/io/socket_ipv6_test.dart (right):

https://codereview.chromium.org/2566273007/diff/140001/tests/standalone/io/so...
tests/standalone/io/socket_ipv6_test.dart:18:
ServerSocket.bind(serverAddr.first, 0).then((server) {
On 2016/12/13 22:22:38, kustermann wrote:
> InternetAddress.lookup()
> ServerSocket.bind()
> Socket.connect()
> 
> They all return futures as well you have to wait for.
> 
> I think it would be much easier to read if we used async/await:
> 
> Future testIPv6ToIPv6() async {
>   var serverAddr = await InternetAddress.lookup(...);
>   var clientAddr = await InternetAddress.lookup(...);
>   var server = await ServerSocket.bind(...);
>   try {
>     Expect.equals(...);
>     Expect.equals(...);
>     server.listen((socket) {...});
>     var clientSocket = await Socket.connect(..);
>     await clientSocket.close();
>   } finally {
>     server.close();
>   }
> }

Ok, but async/await is not needed for correctness.

https://codereview.chromium.org/2566273007/diff/140001/tests/standalone/io/so...
tests/standalone/io/socket_ipv6_test.dart:132: void main() {
On 2016/12/13 22:22:38, kustermann wrote:
> Could use
> 
> main() async {
>   await test...();
> }

Sure, but it seems outside the scope of this CL.

[kustermann, 2016-12-13 23:10:57]

LGTM, thanks!

https://codereview.chromium.org/2566273007/diff/160001/tests/standalone/io/so...
File tests/standalone/io/socket_ipv6_test.dart (right):

https://codereview.chromium.org/2566273007/diff/160001/tests/standalone/io/so...
tests/standalone/io/socket_ipv6_test.dart:61: Socket.connect(clientAddr.first,
server.port).then((socket) {
return

[Florian Schneider, 2016-12-13 23:30:29]

https://codereview.chromium.org/2566273007/diff/160001/tests/standalone/io/so...
File tests/standalone/io/socket_ipv6_test.dart (right):

https://codereview.chromium.org/2566273007/diff/160001/tests/standalone/io/so...
tests/standalone/io/socket_ipv6_test.dart:61: Socket.connect(clientAddr.first,
server.port).then((socket) {
On 2016/12/13 23:10:57, kustermann wrote:
> return

Done.

[Florian Schneider, 2016-12-13 23:33:08]

Description was changed from

==========
Fix flaky timeouts of socket_ipv6_test.

It seems there is no way of guaranteeing that there is always an error in those
two cases, because it depends on other processes running on the system.

Related to #28046.
==========

to

==========
Fix flaky timeouts of socket_ipv6_test.

It seems there is no way of guaranteeing that there is always an error in those
two cases, because it depends on other processes running on the system.

Related to #28046.

R=kustermann@google.com

Committed:
https://github.com/dart-lang/sdk/commit/8cb4b5adfb97a8aa564a3ba21a547fccb35e6df3
==========

[Florian Schneider, 2016-12-13 23:33:10]

Committed patchset #10 (id:180001) manually as
8cb4b5adfb97a8aa564a3ba21a547fccb35e6df3 (presubmit successful).