Various fixes for sync socket implementation.

Various fixes for sync socket implementation.


Revert "Revert "Added sync_socket.h include in sync_socket_win.cc""

This reverts commit 2bd6168e1ebf35f6dbd9b2724339362c493377fc.

Revert "Revert "Added synchronous socket implementation to dart:io.""

This reverts commit a47bce03f5502dac7fb12e777e87b5aa1cfc830b.

BUG=
R=zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/6fbdd19ddb8cc5c9bb38fb1cd910c232a3999d28

[bkonyi, 2017-04-12 17:26:57]

bkonyi@google.com changed reviewers:
+ zra@google.com

[bkonyi, 2017-04-12 17:26:58]

https://codereview.chromium.org/2814773004/diff/1/runtime/bin/socket_android.cc
File runtime/bin/socket_android.cc (left):

https://codereview.chromium.org/2814773004/diff/1/runtime/bin/socket_android....
runtime/bin/socket_android.cc:10: #include <errno.h>        // NOLINT
errno was missing from last submission.

https://codereview.chromium.org/2814773004/diff/1/runtime/bin/sync_socket_and...
File runtime/bin/sync_socket_android.cc (right):

https://codereview.chromium.org/2814773004/diff/1/runtime/bin/sync_socket_and...
runtime/bin/sync_socket_android.cc:12: #include <errno.h>  // NOLINT
errno was missing from last submission.

https://codereview.chromium.org/2814773004/diff/1/runtime/bin/sync_socket_pat...
File runtime/bin/sync_socket_patch.dart (right):

https://codereview.chromium.org/2814773004/diff/1/runtime/bin/sync_socket_pat...
runtime/bin/sync_socket_patch.dart:332: static _nativeLookupRequest(host, int
type)
Making this private fixed issues with the fuzz tests failing.

https://codereview.chromium.org/2814773004/diff/1/runtime/bin/sync_socket_win.cc
File runtime/bin/sync_socket_win.cc (right):

https://codereview.chromium.org/2814773004/diff/1/runtime/bin/sync_socket_win...
runtime/bin/sync_socket_win.cc:20: #if defined(SOCKET_LOG_ERROR)
I'm not sure if this is what we want to do here, but this is what's done in some
of the *socket_fuchsia.cc files that don't support everything.

[zra, 2017-04-12 17:40:41]

https://codereview.chromium.org/2814773004/diff/1/tests/standalone/io/raw_syn...
File tests/standalone/io/raw_synchronous_socket_test.dart (right):

https://codereview.chromium.org/2814773004/diff/1/tests/standalone/io/raw_syn...
tests/standalone/io/raw_synchronous_socket_test.dart:28: void
testInvalidConnect() {
A longer explanation here would be good. Like: The connection attempt happens on
the Dart thread, and the OS timeout can be arbitrarily long. This isn't an issue
with async sockets because the lookup for a connect call happens on the IO
service thread.

https://codereview.chromium.org/2814773004/diff/1/tests/standalone/io/raw_syn...
tests/standalone/io/raw_synchronous_socket_test.dart:44: ;
stray semicolon

https://codereview.chromium.org/2814773004/diff/1/tests/standalone/standalone...
File tests/standalone/standalone.status (right):

https://codereview.chromium.org/2814773004/diff/1/tests/standalone/standalone...
tests/standalone/standalone.status:228: io/raw_synchronous_socket_test: Skip #
Unsupported
Should be Crash, and we should file an issue to implement it with the issue
number listed here.

[bkonyi, 2017-04-12 17:55:02]

Comments addressed. PTAL.

https://codereview.chromium.org/2814773004/diff/1/tests/standalone/io/raw_syn...
File tests/standalone/io/raw_synchronous_socket_test.dart (right):

https://codereview.chromium.org/2814773004/diff/1/tests/standalone/io/raw_syn...
tests/standalone/io/raw_synchronous_socket_test.dart:28: void
testInvalidConnect() {
On 2017/04/12 17:40:41, zra wrote:
> A longer explanation here would be good. Like: The connection attempt happens
on
> the Dart thread, and the OS timeout can be arbitrarily long. This isn't an
issue
> with async sockets because the lookup for a connect call happens on the IO
> service thread.

Done.

https://codereview.chromium.org/2814773004/diff/1/tests/standalone/io/raw_syn...
tests/standalone/io/raw_synchronous_socket_test.dart:44: ;
On 2017/04/12 17:40:41, zra wrote:
> stray semicolon

Done.

https://codereview.chromium.org/2814773004/diff/1/tests/standalone/standalone...
File tests/standalone/standalone.status (right):

https://codereview.chromium.org/2814773004/diff/1/tests/standalone/standalone...
tests/standalone/standalone.status:228: io/raw_synchronous_socket_test: Skip #
Unsupported
On 2017/04/12 17:40:41, zra wrote:
> Should be Crash, and we should file an issue to implement it with the issue
> number listed here.

Done.

[bkonyi, 2017-04-12 18:22:17]

bkonyi@google.com changed reviewers:
+ asiva@google.com

[bkonyi, 2017-04-12 18:22:17]

+Siva. I've addressed the comments from the original CL here:
https://codereview.chromium.org/2803543006/. PTAL when you have a chance.

[zra, 2017-04-12 19:44:22]

https://codereview.chromium.org/2814773004/diff/40001/runtime/bin/sync_socket.cc
File runtime/bin/sync_socket.cc (right):

https://codereview.chromium.org/2814773004/diff/40001/runtime/bin/sync_socket...
runtime/bin/sync_socket.cc:152: Dart_SetReturnValue(args,
DartUtils::NewDartOSError(&os_error));
I think you misunderstood asiva's comment here. You can put
Dart_TypedDataReleaseData after the if {} else {}.

https://codereview.chromium.org/2814773004/diff/40001/runtime/bin/sync_socket...
runtime/bin/sync_socket.cc:185: Dart_SetIntegerReturnValue(args, bytes_read);
This will overwrite the error return value if there is one.

[bkonyi, 2017-04-12 19:59:22]

Fixed issues brought up by Zach. PTAL.

https://codereview.chromium.org/2814773004/diff/40001/runtime/bin/sync_socket.cc
File runtime/bin/sync_socket.cc (right):

https://codereview.chromium.org/2814773004/diff/40001/runtime/bin/sync_socket...
runtime/bin/sync_socket.cc:152: Dart_SetReturnValue(args,
DartUtils::NewDartOSError(&os_error));
On 2017/04/12 19:44:21, zra wrote:
> I think you misunderstood asiva's comment here. You can put
> Dart_TypedDataReleaseData after the if {} else {}.

Ah, that makes sense. Done.

https://codereview.chromium.org/2814773004/diff/40001/runtime/bin/sync_socket...
runtime/bin/sync_socket.cc:185: Dart_SetIntegerReturnValue(args, bytes_read);
On 2017/04/12 19:44:21, zra wrote:
> This will overwrite the error return value if there is one.

Whoops... that wasn't my intention. Fixed.

[bkonyi, 2017-04-12 20:13:52]

https://codereview.chromium.org/2814773004/diff/60001/runtime/bin/sync_socket.cc
File runtime/bin/sync_socket.cc (right):

https://codereview.chromium.org/2814773004/diff/60001/runtime/bin/sync_socket...
runtime/bin/sync_socket.cc:148: Dart_TypedDataReleaseData(buffer_obj);
Forgot to remove this here... Removed in next patch.

[zra, 2017-04-12 20:26:28]

https://codereview.chromium.org/2814773004/diff/60001/runtime/bin/sync_socket.cc
File runtime/bin/sync_socket.cc (right):

https://codereview.chromium.org/2814773004/diff/60001/runtime/bin/sync_socket...
runtime/bin/sync_socket.cc:148: Dart_TypedDataReleaseData(buffer_obj);
you can remove this one.

https://codereview.chromium.org/2814773004/diff/60001/runtime/bin/sync_socket...
runtime/bin/sync_socket.cc:184: return;
Instead of having the error case in between two non-error cases, how about:

if (bytes_read < 0) {
  // set error return
  return;
}

if (bytes_read > 0) {
  // set list
}

// set return

[bkonyi, 2017-04-12 20:37:02]

https://codereview.chromium.org/2814773004/diff/60001/runtime/bin/sync_socket.cc
File runtime/bin/sync_socket.cc (right):

https://codereview.chromium.org/2814773004/diff/60001/runtime/bin/sync_socket...
runtime/bin/sync_socket.cc:148: Dart_TypedDataReleaseData(buffer_obj);
On 2017/04/12 20:26:28, zra wrote:
> you can remove this one.

Done.

https://codereview.chromium.org/2814773004/diff/60001/runtime/bin/sync_socket...
runtime/bin/sync_socket.cc:184: return;
On 2017/04/12 20:26:28, zra wrote:
> Instead of having the error case in between two non-error cases, how about:
> 
> if (bytes_read < 0) {
>   // set error return
>   return;
> }
> 
> if (bytes_read > 0) {
>   // set list
> }
> 
> // set return

Done.

[zra, 2017-04-12 20:41:10]

lgtm

[bkonyi, 2017-04-12 21:20:56]

Description was changed from

==========
Various fixes for sync socket implementation.


Revert "Revert "Added sync_socket.h include in sync_socket_win.cc""

This reverts commit 2bd6168e1ebf35f6dbd9b2724339362c493377fc.

Revert "Revert "Added synchronous socket implementation to dart:io.""

This reverts commit a47bce03f5502dac7fb12e777e87b5aa1cfc830b.

BUG=
==========

to

==========
Various fixes for sync socket implementation.


Revert "Revert "Added sync_socket.h include in sync_socket_win.cc""

This reverts commit 2bd6168e1ebf35f6dbd9b2724339362c493377fc.

Revert "Revert "Added synchronous socket implementation to dart:io.""

This reverts commit a47bce03f5502dac7fb12e777e87b5aa1cfc830b.

BUG=
R=zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/6fbdd19ddb8cc5c9bb38fb1cd910c232a3999d28
==========

[bkonyi, 2017-04-12 21:20:57]

Committed patchset #6 (id:100001) manually as
6fbdd19ddb8cc5c9bb38fb1cd910c232a3999d28 (presubmit successful).