Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2803543006:
Added synchronous socket implementation to dart:io. (Closed)
|
Created:
3 years, 8 months ago by bkonyi Modified:
3 years, 8 months ago CC:
reviews_dartlang.org, vm-dev_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdded synchronous socket implementation to dart:io.
BUG=
R=zra@google.com
Committed: https://github.com/dart-lang/sdk/commit/a1f87ebdb835efdc91db5f18f81603658f1f7e32
Patch Set 1 #Patch Set 2 : Small fix for MacOS #
Total comments: 113
Patch Set 3 : Addressed first round of comments #
Total comments: 82
Patch Set 4 : Addressed second round of comments #
Total comments: 26
Patch Set 5 : Added synchronous socket implementation to dart:io. #
Total comments: 14
Patch Set 6 : Updated io_patch files to include RawSynchronousSocket definitions #
Total comments: 4
Patch Set 7 : Changed signature for GetSocketIdNativeField #
Messages
Total messages: 20 (2 generated)
bkonyi@google.com changed reviewers: + asiva@google.com, zra@google.com
PTAL when you get a chance!
First round of comments. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/eventhandle... File runtime/bin/eventhandler_win.h (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/eventhandle... runtime/bin/eventhandler_win.h:449: explicit ClientSocket(intptr_t s, bool sync = false) The synchronous socket implementation should be completely separate from the asynchronous implementation. So, let's not overload this class. Instead, make a new wrapper around the HANDLE in sync_socket_win.cc/h. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket.cc File runtime/bin/sync_socket.cc (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:28: if (Dart_GetNativeArgumentCount(args) == 2) { The logic can be rearranged a bit to avoid a lot of indentation. E.g.: if (DartGetNativeArgumentCount(args) != 2) { // set error return; } ... if (addresses == NULL) { // set error return; } etc. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:30: DartUtils::GetStringValue(Dart_GetNativeArgument(args, 0)); See Dart_GetNativeStringArgument instead https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:31: int type = DartUtils::GetIntegerValue(Dart_GetNativeArgument(args, 1)); See Dart_GetNativeIntegerArgument instead https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:38: Dart_Handle array = Dart_NewList(addresses->count() + 1); If Dart API calls can return error handles, then we need to check for them with Dart_IsError() and Dart_PropagateError() if so. We are probably not as consistent about this everywhere in runtime/bin as we should be, but we can do it right here. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:39: Dart_ListSetAt(array, 0, Dart_NewInteger(0)); Why is this here? https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:65: Dart_SetReturnValue(args, DartUtils::NewDartArgumentError("")); Can you give an error message here? https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:78: OSError error; This declaration should go inside the else case. the OSError constructor can allocate memory and might do a bunch of other stuff depending on the platform, so we should only do that work in the error case. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:93: ASSERT(Dart_IsList(buffer_obj)); This is probably an assert because the argument comes from the patch file, but I think it would be better to be consistent about how we're checking arguments, and instead return an ArgumentError. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:126: // Available failed. Mark socket as having data, to trigger a future read This doesn't make any sense for a synchronous socket. Instead, we should return an OSError here. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:194: OSError os_error; Not needed since you are using DartUtils::NewDartOSError() below. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:207: OSError os_error; ditto https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:210: if (addr != NULL) { if (addr == NULL) { // set error return; } https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:237: } delete socket; https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:241: void SynchronousSocket::ReuseSocketIdNativeField(Dart_Handle handle, Not needed. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket.h File runtime/bin/sync_socket.h (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.h:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file 2017 https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.h:14: #include "bin/builtin.h" It looks like some of these includes are probably not needed. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.h:36: static void ReuseSocketIdNativeField(Dart_Handle handle, The native wrapper instances won't be reused, so this should be unneeded. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.h:44: ~SynchronousSocket() { ASSERT(fd_ == kClosedFd); } This should be public since SynchronousSocket isn't reference counted. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.h:49: Dart_Port port_; I think this field isn't needed.
https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/eventhandle... File runtime/bin/eventhandler_win.h (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/eventhandle... runtime/bin/eventhandler_win.h:449: explicit ClientSocket(intptr_t s, bool sync = false) On 2017/04/07 05:29:27, zra wrote: > The synchronous socket implementation should be completely separate from the > asynchronous implementation. So, let's not overload this class. Instead, make a > new wrapper around the HANDLE in sync_socket_win.cc/h. After thinking about this some more, I think we should cut the Windows implementation from this CL since you don't have a windows box to develop on. We can mark the implementation UNIMPLEMENTED() in sync_socket_win.cc and mark the tests as Crash in the status file.
More comments... https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket.cc File runtime/bin/sync_socket.cc (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:172: Dart_SetReturnValue(args, DartUtils::NewDartOSError(&os_error)); Above, you return an ArgumentError on a bad argument. Why is it an OSError here? https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... File runtime/bin/sync_socket_android.cc (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_android.cc:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file 2017 https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_android.cc:12: #include <errno.h> // NOLINT Are all of these includes really needed? https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_android.cc:60: if ((result == 0) || (errno == EINPROGRESS)) { The socket is not non-blocking, so errno should never be EINPROGRESS. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... File runtime/bin/sync_socket_fuchsia.cc (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_fuchsia.cc:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file 2017 https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_fuchsia.cc:60: if ((result == 0) || (errno == EINPROGRESS)) { No EINPROGRESS. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... File runtime/bin/sync_socket_linux.cc (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_linux.cc:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file 2017 https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_linux.cc:60: if ((result == 0) || (errno == EINPROGRESS)) { No EINPROGRESS https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... File runtime/bin/sync_socket_macos.cc (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_macos.cc:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file 2017 https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_macos.cc:57: if ((result == 0) || (errno == EINPROGRESS)) { No EINPROGRESS. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... File runtime/bin/sync_socket_patch.dart (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:61: _ReadWriteResourceInfo resourceInfo; It looks like you need to call _SocketResourceInfo.SocketClosed() on this at some point. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:67: if (localPort != 0) return localPort; Please put single line ifs in {} https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:90: static _NativeSynchronousSocket connectSync(host, int port) { statics like this that generate an instance I think we tend to put before instance methods. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:120: int errorCode = result.errorCode; errorCode doesn't appear to be used. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:129: error = createError(e, "Connection failed", address, port); if (error == null) https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:165: // TODO(bkonyi) do we need this first empty element? Or is this something specific I don't see what this is used for either. Let's get rid of it. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:173: int readIntoSync(List<int> buffer, int start, int end) { This shouldn't be implemented on top of readSync. The point of this API is to provide a way to avoid extra copies. Please take a look at the implementation of File.readIntoSync and File_ReadInto in runtime/bin/file.cc. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:219: assert(resourceInfo != null); If you move this assert above if (result != null) then you only need it once. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:231: if (!isClosed) { if (isClosed) return then un-indent, here and below. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:253: nativeShutdownRead(); What if isClosedRead is already true? https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:270: void writeFromSync(List<int> buffer, int start, int end) { Have a look at _RandomAccessFile.writeFromSync for ideas about how to clean up this function. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:274: if (buffer is! List) throw new ArgumentError(); Please give an error message. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:276: if (end == null) { Can you use RangeError.checkValidRange()? https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:284: if (offset is! int || bytes is! int) { This should probably be up at the top of the function. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:303: if (result < 0) result = -result; Let's remove this logic if it isn't supported. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... File runtime/bin/sync_socket_win.cc (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_win.cc:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file 2017 https://codereview.chromium.org/2803543006/diff/10018/sdk/lib/io/sync_socket.... File sdk/lib/io/sync_socket.dart (right): https://codereview.chromium.org/2803543006/diff/10018/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file 2017 https://codereview.chromium.org/2803543006/diff/10018/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:8: * A low-level class for communicating over a TCP socket. Add warnings that direct people to the asynchronous Socket or RawSocket, that this implementation is not high-performance, that the operations will block the calling thread such that it will handle no other events, and that it should probably only be used for connections to localhost. https://codereview.chromium.org/2803543006/diff/10018/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:35: * Reads into an existing [List<int>] from the socket. If [start] is present, These comments should have a short one sentence description followed by a longer explanation in a new paragraph. https://codereview.chromium.org/2803543006/diff/10018/tests/standalone/io/raw... File tests/standalone/io/raw_synchronous_socket_test.dart (right): https://codereview.chromium.org/2803543006/diff/10018/tests/standalone/io/raw... tests/standalone/io/raw_synchronous_socket_test.dart:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file 2017 https://codereview.chromium.org/2803543006/diff/10018/tests/standalone/io/raw... tests/standalone/io/raw_synchronous_socket_test.dart:5: // VMOptions= Remove this if there aren't any. https://codereview.chromium.org/2803543006/diff/10018/tests/standalone/io/raw... tests/standalone/io/raw_synchronous_socket_test.dart:17: void testArguments() { How about null? https://codereview.chromium.org/2803543006/diff/10018/tests/standalone/io/raw... tests/standalone/io/raw_synchronous_socket_test.dart:23: RawSynchronousSocket.connectSync(LOOPBACK_IP_V4_STRING, 0, backlog: -1)); what is backlog? https://codereview.chromium.org/2803543006/diff/10018/tests/standalone/io/raw... tests/standalone/io/raw_synchronous_socket_test.dart:24: } We should probably have more tests with bogus arguments also for the instance methods. https://codereview.chromium.org/2803543006/diff/10018/tests/standalone/io/raw... tests/standalone/io/raw_synchronous_socket_test.dart:227: } catch (e, stack) { print("Echo test failed in the client"); rethrow; https://codereview.chromium.org/2803543006/diff/10018/tests/standalone/io/raw... tests/standalone/io/raw_synchronous_socket_test.dart:396: socket.writeFromSync(data); // Should this work? It's weird that this doesn't throw when closed in both directions, but does throw when closed for sending.
This should cover the first round of comments... PTAL. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/eventhandle... File runtime/bin/eventhandler_win.h (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/eventhandle... runtime/bin/eventhandler_win.h:449: explicit ClientSocket(intptr_t s, bool sync = false) On 2017/04/07 14:41:12, zra wrote: > On 2017/04/07 05:29:27, zra wrote: > > The synchronous socket implementation should be completely separate from the > > asynchronous implementation. So, let's not overload this class. Instead, make > a > > new wrapper around the HANDLE in sync_socket_win.cc/h. > > After thinking about this some more, I think we should cut the Windows > implementation from this CL since you don't have a windows box to develop on. We > can mark the implementation UNIMPLEMENTED() in sync_socket_win.cc and mark the > tests as Crash in the status file. Sounds good. Removed. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket.cc File runtime/bin/sync_socket.cc (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:28: if (Dart_GetNativeArgumentCount(args) == 2) { On 2017/04/07 05:29:27, zra wrote: > The logic can be rearranged a bit to avoid a lot of indentation. E.g.: > > if (DartGetNativeArgumentCount(args) != 2) { > // set error > return; > } > ... > if (addresses == NULL) { > // set error > return; > } > > etc. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:30: DartUtils::GetStringValue(Dart_GetNativeArgument(args, 0)); On 2017/04/07 05:29:27, zra wrote: > See Dart_GetNativeStringArgument instead Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:31: int type = DartUtils::GetIntegerValue(Dart_GetNativeArgument(args, 1)); On 2017/04/07 05:29:27, zra wrote: > See Dart_GetNativeIntegerArgument instead Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:38: Dart_Handle array = Dart_NewList(addresses->count() + 1); On 2017/04/07 05:29:27, zra wrote: > If Dart API calls can return error handles, then we need to check for them with > Dart_IsError() and Dart_PropagateError() if so. We are probably not as > consistent about this everywhere in runtime/bin as we should be, but we can do > it right here. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:39: Dart_ListSetAt(array, 0, Dart_NewInteger(0)); On 2017/04/07 05:29:27, zra wrote: > Why is this here? There's a TODO in sync_socket_patch which references this. The async socket implementation did this, my guess is to allow for an error code to be returned, but I couldn't find out where it was set. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:65: Dart_SetReturnValue(args, DartUtils::NewDartArgumentError("")); On 2017/04/07 05:29:27, zra wrote: > Can you give an error message here? Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:78: OSError error; On 2017/04/07 05:29:27, zra wrote: > This declaration should go inside the else case. the OSError constructor can > allocate memory and might do a bunch of other stuff depending on the platform, > so we should only do that work in the error case. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:93: ASSERT(Dart_IsList(buffer_obj)); On 2017/04/07 05:29:27, zra wrote: > This is probably an assert because the argument comes from the patch file, but I > think it would be better to be consistent about how we're checking arguments, > and instead return an ArgumentError. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:126: // Available failed. Mark socket as having data, to trigger a future read On 2017/04/07 05:29:27, zra wrote: > This doesn't make any sense for a synchronous socket. Instead, we should return > an OSError here. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:172: Dart_SetReturnValue(args, DartUtils::NewDartOSError(&os_error)); On 2017/04/07 16:29:40, zra wrote: > Above, you return an ArgumentError on a bad argument. Why is it an OSError here? Not sure... Probably something that migrated from the socket.cc implementation that I didn't catch. Changed. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:194: OSError os_error; On 2017/04/07 05:29:28, zra wrote: > Not needed since you are using DartUtils::NewDartOSError() below. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:207: OSError os_error; On 2017/04/07 05:29:27, zra wrote: > ditto Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:210: if (addr != NULL) { On 2017/04/07 05:29:27, zra wrote: > if (addr == NULL) { > // set error > return; > } Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:237: } On 2017/04/07 05:29:28, zra wrote: > delete socket; Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:241: void SynchronousSocket::ReuseSocketIdNativeField(Dart_Handle handle, On 2017/04/07 05:29:27, zra wrote: > Not needed. Removed. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket.h File runtime/bin/sync_socket.h (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.h:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file On 2017/04/07 05:29:28, zra wrote: > 2017 Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.h:14: #include "bin/builtin.h" On 2017/04/07 05:29:28, zra wrote: > It looks like some of these includes are probably not needed. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.h:36: static void ReuseSocketIdNativeField(Dart_Handle handle, On 2017/04/07 05:29:28, zra wrote: > The native wrapper instances won't be reused, so this should be unneeded. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.h:44: ~SynchronousSocket() { ASSERT(fd_ == kClosedFd); } On 2017/04/07 05:29:28, zra wrote: > This should be public since SynchronousSocket isn't reference counted. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket.h:49: Dart_Port port_; On 2017/04/07 05:29:28, zra wrote: > I think this field isn't needed. You're right. Removed. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... File runtime/bin/sync_socket_android.cc (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_android.cc:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file On 2017/04/07 16:29:40, zra wrote: > 2017 Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_android.cc:12: #include <errno.h> // NOLINT On 2017/04/07 16:29:40, zra wrote: > Are all of these includes really needed? Apparently not. I've removed them here and on most other platforms. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_android.cc:60: if ((result == 0) || (errno == EINPROGRESS)) { On 2017/04/07 16:29:40, zra wrote: > The socket is not non-blocking, so errno should never be EINPROGRESS. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... File runtime/bin/sync_socket_fuchsia.cc (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_fuchsia.cc:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file On 2017/04/07 16:29:40, zra wrote: > 2017 Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_fuchsia.cc:60: if ((result == 0) || (errno == EINPROGRESS)) { On 2017/04/07 16:29:40, zra wrote: > No EINPROGRESS. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... File runtime/bin/sync_socket_linux.cc (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_linux.cc:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file On 2017/04/07 16:29:40, zra wrote: > 2017 Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_linux.cc:60: if ((result == 0) || (errno == EINPROGRESS)) { On 2017/04/07 16:29:40, zra wrote: > No EINPROGRESS Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... File runtime/bin/sync_socket_macos.cc (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_macos.cc:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file On 2017/04/07 16:29:40, zra wrote: > 2017 Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_macos.cc:57: if ((result == 0) || (errno == EINPROGRESS)) { On 2017/04/07 16:29:40, zra wrote: > No EINPROGRESS. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... File runtime/bin/sync_socket_patch.dart (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:61: _ReadWriteResourceInfo resourceInfo; On 2017/04/07 16:29:41, zra wrote: > It looks like you need to call _SocketResourceInfo.SocketClosed() on this at > some point. Added SocketClosed() call in CloseSync() https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:67: if (localPort != 0) return localPort; On 2017/04/07 16:29:41, zra wrote: > Please put single line ifs in {} Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:90: static _NativeSynchronousSocket connectSync(host, int port) { On 2017/04/07 16:29:41, zra wrote: > statics like this that generate an instance I think we tend to put before > instance methods. Ah, I wasn't sure where static methods like this should go relative to the get/set methods. Moved. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:120: int errorCode = result.errorCode; On 2017/04/07 16:29:40, zra wrote: > errorCode doesn't appear to be used. Removed. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:129: error = createError(e, "Connection failed", address, port); On 2017/04/07 16:29:41, zra wrote: > if (error == null) Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:165: // TODO(bkonyi) do we need this first empty element? Or is this something specific On 2017/04/07 16:29:41, zra wrote: > I don't see what this is used for either. Let's get rid of it. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:173: int readIntoSync(List<int> buffer, int start, int end) { On 2017/04/07 16:29:40, zra wrote: > This shouldn't be implemented on top of readSync. The point of this API is to > provide a way to avoid extra copies. Please take a look at the implementation of > File.readIntoSync and File_ReadInto in runtime/bin/file.cc. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:219: assert(resourceInfo != null); On 2017/04/07 16:29:41, zra wrote: > If you move this assert above if (result != null) then you only need it once. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:231: if (!isClosed) { On 2017/04/07 16:29:41, zra wrote: > if (isClosed) return > > then un-indent, here and below. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:253: nativeShutdownRead(); On 2017/04/07 16:29:41, zra wrote: > What if isClosedRead is already true? Then the native shutdown read method would be called again, even though it wouldn't do anything. I've added a check for this with the if (isClosed || ...) to just return. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:270: void writeFromSync(List<int> buffer, int start, int end) { On 2017/04/07 16:29:41, zra wrote: > Have a look at _RandomAccessFile.writeFromSync for ideas about how to clean up > this function. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:274: if (buffer is! List) throw new ArgumentError(); On 2017/04/07 16:29:41, zra wrote: > Please give an error message. Acknowledged. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:276: if (end == null) { On 2017/04/07 16:29:41, zra wrote: > Can you use RangeError.checkValidRange()? I didn't know that existed... super useful! Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:284: if (offset is! int || bytes is! int) { On 2017/04/07 16:29:40, zra wrote: > This should probably be up at the top of the function. Acknowledged. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:303: if (result < 0) result = -result; On 2017/04/07 16:29:41, zra wrote: > Let's remove this logic if it isn't supported. Done. https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... File runtime/bin/sync_socket_win.cc (right): https://codereview.chromium.org/2803543006/diff/10018/runtime/bin/sync_socket... runtime/bin/sync_socket_win.cc:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file On 2017/04/07 16:29:41, zra wrote: > 2017 Done. https://codereview.chromium.org/2803543006/diff/10018/sdk/lib/io/sync_socket.... File sdk/lib/io/sync_socket.dart (right): https://codereview.chromium.org/2803543006/diff/10018/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file On 2017/04/07 16:29:41, zra wrote: > 2017 Done. https://codereview.chromium.org/2803543006/diff/10018/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:8: * A low-level class for communicating over a TCP socket. On 2017/04/07 16:29:41, zra wrote: > Add warnings that direct people to the asynchronous Socket or RawSocket, that > this implementation is not high-performance, that the operations will block the > calling thread such that it will handle no other events, and that it should > probably only be used for connections to localhost. Done. https://codereview.chromium.org/2803543006/diff/10018/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:35: * Reads into an existing [List<int>] from the socket. If [start] is present, On 2017/04/07 16:29:41, zra wrote: > These comments should have a short one sentence description followed by a longer > explanation in a new paragraph. Done. https://codereview.chromium.org/2803543006/diff/10018/tests/standalone/io/raw... File tests/standalone/io/raw_synchronous_socket_test.dart (right): https://codereview.chromium.org/2803543006/diff/10018/tests/standalone/io/raw... tests/standalone/io/raw_synchronous_socket_test.dart:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file On 2017/04/07 16:29:42, zra wrote: > 2017 Done. https://codereview.chromium.org/2803543006/diff/10018/tests/standalone/io/raw... tests/standalone/io/raw_synchronous_socket_test.dart:5: // VMOptions= On 2017/04/07 16:29:41, zra wrote: > Remove this if there aren't any. Done. https://codereview.chromium.org/2803543006/diff/10018/tests/standalone/io/raw... tests/standalone/io/raw_synchronous_socket_test.dart:17: void testArguments() { On 2017/04/07 16:29:42, zra wrote: > How about null? Done. https://codereview.chromium.org/2803543006/diff/10018/tests/standalone/io/raw... tests/standalone/io/raw_synchronous_socket_test.dart:23: RawSynchronousSocket.connectSync(LOOPBACK_IP_V4_STRING, 0, backlog: -1)); On 2017/04/07 16:29:42, zra wrote: > what is backlog? Something that came from the RawSocket tests. I couldn't find backlog there either, so I assumed this is a check for completely invalid parameters. https://codereview.chromium.org/2803543006/diff/10018/tests/standalone/io/raw... tests/standalone/io/raw_synchronous_socket_test.dart:24: } On 2017/04/07 16:29:42, zra wrote: > We should probably have more tests with bogus arguments also for the instance > methods. Done. https://codereview.chromium.org/2803543006/diff/10018/tests/standalone/io/raw... tests/standalone/io/raw_synchronous_socket_test.dart:227: } catch (e, stack) { On 2017/04/07 16:29:42, zra wrote: > print("Echo test failed in the client"); > rethrow; Done. https://codereview.chromium.org/2803543006/diff/10018/tests/standalone/io/raw... tests/standalone/io/raw_synchronous_socket_test.dart:396: socket.writeFromSync(data); // Should this work? On 2017/04/07 16:29:41, zra wrote: > It's weird that this doesn't throw when closed in both directions, but does > throw when closed for sending. I've gone ahead and changed all the read/write operations to throw exceptions when they're closed.
https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/eventhandle... File runtime/bin/eventhandler_win.h (right): https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/eventhandle... runtime/bin/eventhandler_win.h:449: explicit ClientSocket(intptr_t s, bool sync = false) Let's remove the Windows implementation from this CL. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket.cc File runtime/bin/sync_socket.cc (right): https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:42: args, DartUtils::NewDartArgumentError("Invalid value for host")); host_arg is already the error that we want to propagate here, so there's no need to make a new one. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:49: args, DartUtils::NewDartArgumentError("Invalid value for host")); ditto https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:56: args, DartUtils::NewDartArgumentError("Invalid value for port")); ditto https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:71: DART_CHECK_VALID_AND_PROPAGATE(array); This leaks 'addresses'. Here and below, we should instead be doing: if (Dart_IsError(handle)) { delete addresses; Dart_SetReturnValue(args, handle); return; } You can wrap that up locally in this file with a macro. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:80: Dart_ListSetAt(entry, 0, type); Dart_ListSetAt returns a handle that needs to be checked for an error, as well. For example, if the access is out-of-bounds. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:93: result = array; 'result' doesn't appear to do very much. Maybe rename array -> result, or just return array below. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:112: Dart_SetReturnValue(args, Dart_True()); Dart_SetBooleanReturnValue(args, true); https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:142: Dart_SetReturnValue(args, Dart_NewInteger(bytes_written)); Dart_SetIntegerReturnValue(args, bytes_written); https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:167: Can we ASSERT something here about the list being long enough? There's a check in the Dart code, but we should have it in both places to make sure no one breaks the contract. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:175: Dart_SetReturnValue(args, Dart_NewInteger(bytes_read)); Dart_SetIntegerReturnValue(args, bytes_read); https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:188: Dart_SetReturnValue(args, Dart_NewInteger(available)); Dart_SetIntegerReturnValue(args, available); https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:208: if (DartUtils::GetInt64Value(Dart_GetNativeArgument(args, 1), &length)) { if (!...) { // set error return return; } then un-nest. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:224: Dart_SetReturnValue(args, Dart_Null()); I think Dart_Null() is already the default if nothing explicitly set. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:256: Dart_SetReturnValue(args, Dart_NewInteger(port)); Dart_SetIntegerReturnValue(args, port) https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:273: DART_CHECK_VALID_AND_PROPAGATE(list); This can leak 'addr'. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:278: Dart_ListSetAt(entry, 0, Dart_NewInteger(addr->GetType())); Check the result of Dart_ListSetAt() https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:308: DART_CHECK_VALID_AND_PROPAGATE(err); This can leak 'socket'. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... File runtime/bin/sync_socket_patch.dart (right): https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:16: _RawSynchronousSocket(this._socket) {} {} not needed. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:140: var type = new InternetAddressType._from(addr[0]); type is unused. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:181: return response.map((result) { A for-loop that adds elements to a preallocated list will be faster and have fewer intermediate allocations. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:182: var type = new InternetAddressType._from(result[0]); type is unused. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:199: throw new ArgumentError("start cannot be equal to null"); strike "equal to" https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:219: if (len != null && len < 0) { Please put () around clauses. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:228: return null; this statement is unreachable. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... File runtime/bin/sync_socket_win.cc (right): https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_win.cc:10: #include "bin/builtin.h" remove unused includes. https://codereview.chromium.org/2803543006/diff/30001/runtime/include/dart_api.h File runtime/include/dart_api.h (right): https://codereview.chromium.org/2803543006/diff/30001/runtime/include/dart_ap... runtime/include/dart_api.h:398: Dart_PropagateError(handle); \ Dart_PropagateError is actually a little bit dangerous because it skips over C++ frames without giving any opportunity for cleanup, so I don't think we should be making it any more convenient to use. https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... File sdk/lib/io/sync_socket.dart (right): https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:8: * A low-level class for communicating synchronously over a TCP socket. Add another line break here. https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:9: * Warning: This socket implementation is not suitable for most Socket I/O tasks I'd prefer something more like: "Warning: [RawSynchronousSocket] should probably only be used to connect to 'localhost'. The operations below will block the calling thread to wait for a response from the network. The thread can process no other events while waiting for these operations to complete. [RawSynchronousSocket] is not suitable for applications that require high performance or asynchronous I/O such as a server. Instead such applications should use the non-blocking sockets and asynchronous operations in the Socket or RawSocket classes." https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:19: * Creates a new socket connection to the host and port and returns a strike: 'to the host and port' https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:23: * [String], [connect] will perform a [InternetAddress.lookup] and try connect -> connectSync https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:58: * Blocks and waits for a response from the server. "Reads up to [bytes] bytes from the socket." https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:60: * Blocks and waits for a response of up to a specified number of bytes Replace "remove server" with "socket". https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:90: * Returns the port used by this socket. strike "Returns" https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:95: * Returns the remote port connected to by this socket. ditto https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:100: * Returns the [InternetAddress] used to connect this socket. ditto https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:105: * Returns the remote [InternetAddress] connected to by this socket. ditto https://codereview.chromium.org/2803543006/diff/30001/tests/standalone/io/raw... File tests/standalone/io/raw_synchronous_socket_test.dart (right): https://codereview.chromium.org/2803543006/diff/30001/tests/standalone/io/raw... tests/standalone/io/raw_synchronous_socket_test.dart:353: // ensure reads or writes cannot be performed if the socket has been shutdown for long line
https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket.cc File runtime/bin/sync_socket.cc (right): https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:24: static const int kSocketIdNativeField = 0; I have one high level comment, maybe does not need to be addressed in this CL but possibly in a follow-up CL. The code in this file seems very similar to the code in socket.cc, maybe it is possible to share between these two files. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... File runtime/bin/sync_socket_android.cc (right): https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_android.cc:16: namespace bin { ditto comment about sharing this with socket_android.cc
Addressed the latest round of comments. PTAL https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/eventhandle... File runtime/bin/eventhandler_win.h (right): https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/eventhandle... runtime/bin/eventhandler_win.h:449: explicit ClientSocket(intptr_t s, bool sync = false) On 2017/04/10 21:39:42, zra wrote: > Let's remove the Windows implementation from this CL. Thought I had done this before. Removed. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket.cc File runtime/bin/sync_socket.cc (right): https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:24: static const int kSocketIdNativeField = 0; On 2017/04/10 22:51:46, siva wrote: > I have one high level comment, maybe does not need to be addressed in this CL > but possibly in a follow-up CL. > > The code in this file seems very similar to the code in socket.cc, maybe it is > possible to share between these two files. They are very similar, but I think I've gotten most of the code that can be shared pulled into SocketBase. I'll take another look to see if we can pull more out into SocketBase in another CL. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:42: args, DartUtils::NewDartArgumentError("Invalid value for host")); On 2017/04/10 21:39:43, zra wrote: > host_arg is already the error that we want to propagate here, so there's no need > to make a new one. Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:49: args, DartUtils::NewDartArgumentError("Invalid value for host")); On 2017/04/10 21:39:42, zra wrote: > ditto Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:56: args, DartUtils::NewDartArgumentError("Invalid value for port")); On 2017/04/10 21:39:43, zra wrote: > ditto Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:71: DART_CHECK_VALID_AND_PROPAGATE(array); On 2017/04/10 21:39:43, zra wrote: > This leaks 'addresses'. > > Here and below, we should instead be doing: > > if (Dart_IsError(handle)) { > delete addresses; > Dart_SetReturnValue(args, handle); > return; > } > > You can wrap that up locally in this file with a macro. Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:80: Dart_ListSetAt(entry, 0, type); On 2017/04/10 21:39:42, zra wrote: > Dart_ListSetAt returns a handle that needs to be checked for an error, as well. > For example, if the access is out-of-bounds. Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:93: result = array; On 2017/04/10 21:39:43, zra wrote: > 'result' doesn't appear to do very much. Maybe rename array -> result, or just > return array below. Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:112: Dart_SetReturnValue(args, Dart_True()); On 2017/04/10 21:39:43, zra wrote: > Dart_SetBooleanReturnValue(args, true); Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:142: Dart_SetReturnValue(args, Dart_NewInteger(bytes_written)); On 2017/04/10 21:39:43, zra wrote: > Dart_SetIntegerReturnValue(args, bytes_written); Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:167: On 2017/04/10 21:39:43, zra wrote: > Can we ASSERT something here about the list being long enough? There's a check > in the Dart code, but we should have it in both places to make sure no one > breaks the contract. Do you mean check to see if the offset and bytes still fall within the valid range? https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:175: Dart_SetReturnValue(args, Dart_NewInteger(bytes_read)); On 2017/04/10 21:39:42, zra wrote: > Dart_SetIntegerReturnValue(args, bytes_read); Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:188: Dart_SetReturnValue(args, Dart_NewInteger(available)); On 2017/04/10 21:39:43, zra wrote: > Dart_SetIntegerReturnValue(args, available); Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:208: if (DartUtils::GetInt64Value(Dart_GetNativeArgument(args, 1), &length)) { On 2017/04/10 21:39:42, zra wrote: > if (!...) { > // set error return > return; > } > > then un-nest. Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:224: Dart_SetReturnValue(args, Dart_Null()); On 2017/04/10 21:39:43, zra wrote: > I think Dart_Null() is already the default if nothing explicitly set. Acknowledged. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:256: Dart_SetReturnValue(args, Dart_NewInteger(port)); On 2017/04/10 21:39:43, zra wrote: > Dart_SetIntegerReturnValue(args, port) Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:273: DART_CHECK_VALID_AND_PROPAGATE(list); On 2017/04/10 21:39:42, zra wrote: > This can leak 'addr'. Fixed. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:278: Dart_ListSetAt(entry, 0, Dart_NewInteger(addr->GetType())); On 2017/04/10 21:39:43, zra wrote: > Check the result of Dart_ListSetAt() Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:308: DART_CHECK_VALID_AND_PROPAGATE(err); On 2017/04/10 21:39:42, zra wrote: > This can leak 'socket'. What would be the best way to deal with this case? We can't return an error to Dart with this call in it's current state without propagating the error. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... File runtime/bin/sync_socket_android.cc (right): https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_android.cc:16: namespace bin { On 2017/04/10 22:51:46, siva wrote: > ditto comment about sharing this with socket_android.cc Acknowledged. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... File runtime/bin/sync_socket_patch.dart (right): https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:16: _RawSynchronousSocket(this._socket) {} On 2017/04/10 21:39:43, zra wrote: > {} not needed. Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:140: var type = new InternetAddressType._from(addr[0]); On 2017/04/10 21:39:43, zra wrote: > type is unused. Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:181: return response.map((result) { On 2017/04/10 21:39:43, zra wrote: > A for-loop that adds elements to a preallocated list will be faster and have > fewer intermediate allocations. Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:182: var type = new InternetAddressType._from(result[0]); On 2017/04/10 21:39:43, zra wrote: > type is unused. Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:199: throw new ArgumentError("start cannot be equal to null"); On 2017/04/10 21:39:43, zra wrote: > strike "equal to" Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:219: if (len != null && len < 0) { On 2017/04/10 21:39:43, zra wrote: > Please put () around clauses. Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_patch.dart:228: return null; On 2017/04/10 21:39:43, zra wrote: > this statement is unreachable. Done. https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... File runtime/bin/sync_socket_win.cc (right): https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket_win.cc:10: #include "bin/builtin.h" On 2017/04/10 21:39:43, zra wrote: > remove unused includes. I've removed what I think should be fine to remove, but I'm not sure about the rest. https://codereview.chromium.org/2803543006/diff/30001/runtime/include/dart_api.h File runtime/include/dart_api.h (right): https://codereview.chromium.org/2803543006/diff/30001/runtime/include/dart_ap... runtime/include/dart_api.h:398: Dart_PropagateError(handle); \ On 2017/04/10 21:39:43, zra wrote: > Dart_PropagateError is actually a little bit dangerous because it skips over C++ > frames without giving any opportunity for cleanup, so I don't think we should be > making it any more convenient to use. Ah okay, good to know. Removed. https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... File sdk/lib/io/sync_socket.dart (right): https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:8: * A low-level class for communicating synchronously over a TCP socket. On 2017/04/10 21:39:43, zra wrote: > Add another line break here. Done. https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:9: * Warning: This socket implementation is not suitable for most Socket I/O tasks On 2017/04/10 21:39:44, zra wrote: > I'd prefer something more like: > > "Warning: [RawSynchronousSocket] should probably only be used to connect to > 'localhost'. The operations below will block the calling thread to wait for a > response from the network. The thread can process no other events while waiting > for these operations to complete. [RawSynchronousSocket] is not suitable for > applications that require high performance or asynchronous I/O such as a server. > Instead such applications should use the non-blocking sockets and asynchronous > operations in the Socket or RawSocket classes." Done. https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:19: * Creates a new socket connection to the host and port and returns a On 2017/04/10 21:39:44, zra wrote: > strike: 'to the host and port' Done. https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:23: * [String], [connect] will perform a [InternetAddress.lookup] and try On 2017/04/10 21:39:44, zra wrote: > connect -> connectSync Done. https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:58: * Blocks and waits for a response from the server. On 2017/04/10 21:39:44, zra wrote: > "Reads up to [bytes] bytes from the socket." Done. https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:60: * Blocks and waits for a response of up to a specified number of bytes On 2017/04/10 21:39:44, zra wrote: > Replace "remove server" with "socket". Done. https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:90: * Returns the port used by this socket. On 2017/04/10 21:39:43, zra wrote: > strike "Returns" Done. https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:95: * Returns the remote port connected to by this socket. On 2017/04/10 21:39:44, zra wrote: > ditto Done. https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:100: * Returns the [InternetAddress] used to connect this socket. On 2017/04/10 21:39:44, zra wrote: > ditto Done. https://codereview.chromium.org/2803543006/diff/30001/sdk/lib/io/sync_socket.... sdk/lib/io/sync_socket.dart:105: * Returns the remote [InternetAddress] connected to by this socket. On 2017/04/10 21:39:44, zra wrote: > ditto Done. https://codereview.chromium.org/2803543006/diff/30001/tests/standalone/io/raw... File tests/standalone/io/raw_synchronous_socket_test.dart (right): https://codereview.chromium.org/2803543006/diff/30001/tests/standalone/io/raw... tests/standalone/io/raw_synchronous_socket_test.dart:353: // ensure reads or writes cannot be performed if the socket has been shutdown for On 2017/04/10 21:39:44, zra wrote: > long line Done.
We're getting close! https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket.cc File runtime/bin/sync_socket.cc (right): https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:167: On 2017/04/11 01:30:50, bkonyi wrote: > On 2017/04/10 21:39:43, zra wrote: > > Can we ASSERT something here about the list being long enough? There's a check > > in the Dart code, but we should have it in both places to make sure no one > > breaks the contract. > > Do you mean check to see if the offset and bytes still fall within the valid > range? Yes. See the ASSERT in a similar location in File_ReadInto in runtime/bin/file.cc https://codereview.chromium.org/2803543006/diff/30001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:308: DART_CHECK_VALID_AND_PROPAGATE(err); On 2017/04/11 01:30:51, bkonyi wrote: > On 2017/04/10 21:39:42, zra wrote: > > This can leak 'socket'. > > What would be the best way to deal with this case? We can't return an error to > Dart with this call in it's current state without propagating the error. There are two options: 1. Check that nothing is leaked from the callers of this function if you call Dart_PropagateError() here. 2. Return the error handle, and check Dart_IsError() at the call sites. (2) is probably safer, but we're also probably not consistent about it in other places. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket.cc File runtime/bin/sync_socket.cc (right): https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:50: char* host = NULL; nit: Maybe declare this closer to where it's used. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:52: int64_t type = 0; ditto https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:56: if (Dart_IsError(host_arg)) { You can use your macro here. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:62: if (Dart_IsError(host_arg)) { ditto https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:68: if (Dart_IsError(port_error)) { ditto https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:76: nit: This whitespace is allowed by the style guide, but I find it nice to have a check like the one below right next to the call whose result is being checked. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:83: Dart_Handle error; nit: It's a little confusing to declare this between the assignment to 'array' and the error check on 'array'. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:226: if (Dart_IsError(result)) { IOBuffer::Allocate can't return an error handle. (It probably should return an error handle instead of calling Dart_PropagateError(), but we shouldn't do that refactoring in this CL.) https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:240: if (Dart_IsError(new_result)) { IOBuffer::Allocate can't return an error handle. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:245: return; Return the error that's in new_result https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:249: IOBuffer::Free(buffer); IOBuffer attaches 'buffer' to a WeakPersistentHandle that has a finalizer. GC will clean up the object that we aren't returning and run its finalizer, which will delete 'buffer'. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:317: #undef DART_CHECK_ERROR_AND_CLEANUP This is probably unnecessary. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket.h File runtime/bin/sync_socket.h (right): https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.h:13: #include "bin/socket_base.h" Alphabetize.
Hopefully this should do it! PTAL :) https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket.cc File runtime/bin/sync_socket.cc (right): https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:50: char* host = NULL; On 2017/04/11 16:19:40, zra wrote: > nit: Maybe declare this closer to where it's used. Done. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:52: int64_t type = 0; On 2017/04/11 16:19:41, zra wrote: > ditto Done. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:56: if (Dart_IsError(host_arg)) { On 2017/04/11 16:19:40, zra wrote: > You can use your macro here. Done. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:62: if (Dart_IsError(host_arg)) { On 2017/04/11 16:19:40, zra wrote: > ditto Done. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:68: if (Dart_IsError(port_error)) { On 2017/04/11 16:19:40, zra wrote: > ditto Done. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:76: On 2017/04/11 16:19:40, zra wrote: > nit: This whitespace is allowed by the style guide, but I find it nice to have a > check like the one below right next to the call whose result is being checked. Makes sense. I've removed the whitespace. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:83: Dart_Handle error; On 2017/04/11 16:19:40, zra wrote: > nit: It's a little confusing to declare this between the assignment to 'array' > and the error check on 'array'. I agree. I've moved the declaration to the first time error is used below. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:226: if (Dart_IsError(result)) { On 2017/04/11 16:19:40, zra wrote: > IOBuffer::Allocate can't return an error handle. (It probably should return an > error handle instead of calling Dart_PropagateError(), but we shouldn't do that > refactoring in this CL.) Acknowledged. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:240: if (Dart_IsError(new_result)) { On 2017/04/11 16:19:41, zra wrote: > IOBuffer::Allocate can't return an error handle. Acknowledged. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:245: return; On 2017/04/11 16:19:40, zra wrote: > Return the error that's in new_result Done. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:249: IOBuffer::Free(buffer); On 2017/04/11 16:19:40, zra wrote: > IOBuffer attaches 'buffer' to a WeakPersistentHandle that has a finalizer. GC > will clean up the object that we aren't returning and run its finalizer, which > will delete 'buffer'. Acknowledged. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:317: #undef DART_CHECK_ERROR_AND_CLEANUP On 2017/04/11 16:19:40, zra wrote: > This is probably unnecessary. Yes, this was left over from some rough stuff I was doing... I'll remove it. https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket.h File runtime/bin/sync_socket.h (right): https://codereview.chromium.org/2803543006/diff/50001/runtime/bin/sync_socket... runtime/bin/sync_socket.h:13: #include "bin/socket_base.h" On 2017/04/11 16:19:41, zra wrote: > Alphabetize. Done.
To avoid angering dart2js and ddc, we also need to fill in dummy @patch implementations here: https://github.com/dart-lang/sdk/blob/master/sdk/lib/_internal/js_runtime/lib... and here: https://github.com/dart-lang/sdk/blob/master/pkg/dev_compiler/tool/input_sdk/... https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket.cc File runtime/bin/sync_socket.cc (right): https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:306: Dart_SetNativeInstanceField(handle, kSocketIdNativeField, For these two functions, please choose one of the two options that I set out in the last group of comments. https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:317: Dart_GetNativeInstanceField(socket_obj, kSocketIdNativeField, &id); The return of Dart_GetNativeInstanceField should also be checked for an error handle.
I think this covers the last set of comments... PTAL. https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket.cc File runtime/bin/sync_socket.cc (right): https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:306: Dart_SetNativeInstanceField(handle, kSocketIdNativeField, On 2017/04/11 20:08:28, zra wrote: > For these two functions, please choose one of the two options that I set out in > the last group of comments. Done. https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:317: Dart_GetNativeInstanceField(socket_obj, kSocketIdNativeField, &id); On 2017/04/11 20:08:28, zra wrote: > The return of Dart_GetNativeInstanceField should also be checked for an error > handle. Done.
https://codereview.chromium.org/2803543006/diff/90001/runtime/bin/sync_socket.cc File runtime/bin/sync_socket.cc (right): https://codereview.chromium.org/2803543006/diff/90001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:24: Dart_SetReturnValue(args, DartUtils::NewDartOSError()); \ This will mask a Dart error handle with an OSError for errno = 0, which will be very confusing. Let's remove this macro, and instead propagate the error handle to the callers of GetSocketIdNativeField. https://codereview.chromium.org/2803543006/diff/90001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:347: SynchronousSocket* SynchronousSocket::GetSocketIdNativeField( Dart_Handle GetSocketIdNativeField(Dart_Handle socket_obj, SynchronousSocket** socket); so that the caller can do the right thing with the error handle.
Comments addressed. PTAL. https://codereview.chromium.org/2803543006/diff/90001/runtime/bin/sync_socket.cc File runtime/bin/sync_socket.cc (right): https://codereview.chromium.org/2803543006/diff/90001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:24: Dart_SetReturnValue(args, DartUtils::NewDartOSError()); \ On 2017/04/11 21:21:45, zra wrote: > This will mask a Dart error handle with an OSError for errno = 0, which will be > very confusing. Let's remove this macro, and instead propagate the error handle > to the callers of GetSocketIdNativeField. Done. https://codereview.chromium.org/2803543006/diff/90001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:347: SynchronousSocket* SynchronousSocket::GetSocketIdNativeField( On 2017/04/11 21:21:45, zra wrote: > Dart_Handle GetSocketIdNativeField(Dart_Handle socket_obj, SynchronousSocket** > socket); > > so that the caller can do the right thing with the error handle. Done.
lgtm
Description was changed from ========== Added synchronous socket implementation to dart:io. BUG= ========== to ========== Added synchronous socket implementation to dart:io. BUG= R=zra@google.com Committed: https://github.com/dart-lang/sdk/commit/a1f87ebdb835efdc91db5f18f81603658f1f7e32 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:110001) manually as a1f87ebdb835efdc91db5f18f81603658f1f7e32 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket.cc File runtime/bin/sync_socket.cc (right): https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:149: Dart_TypedDataReleaseData(buffer_obj); Dart_TypedDataReleaseData(buffer_obj); can be hoisted down to the end. https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:175: DART_CHECK_ERROR(result); If byte_read is 0 then there is nothing to set in the List... https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:226: } is SocketBase::Read guaranteed to never return 0? In ReadList above it seems to account for 0. https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket.h File runtime/bin/sync_socket.h (right): https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket... runtime/bin/sync_socket.h:10: #endif The socket* files seem to use #if !defined(DART_IO_DISABLED) ... ... ... #endif // !defined(DART_IO_DISABLED) why not use the same pattern here too? https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket... runtime/bin/sync_socket.h:31: static SynchronousSocket* GetSocketIdNativeField(Dart_Handle socket); The get/set API is not uniform set takes an intptr_t id get returns SynchronousSocket* Maybe we should make them consistent (I see that the async socket API is also similar, something to consider for another CL).
Message was sent while issue was closed.
Siva, I've addressed your comments in this CL here: https://codereview.chromium.org/2814773004/ https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket.cc File runtime/bin/sync_socket.cc (right): https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:149: Dart_TypedDataReleaseData(buffer_obj); On 2017/04/11 23:04:47, siva wrote: > Dart_TypedDataReleaseData(buffer_obj); > > can be hoisted down to the end. Done. https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:175: DART_CHECK_ERROR(result); On 2017/04/11 23:04:47, siva wrote: > If byte_read is 0 then there is nothing to set in the List... That's right. I've gone ahead and changed the check to > 0 and then if bytes_read == 0 we just set the return value instead of making the Dart_ListSetAsBytes call for no reason. https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket... runtime/bin/sync_socket.cc:226: } On 2017/04/11 23:04:47, siva wrote: > is SocketBase::Read guaranteed to never return 0? In ReadList above it seems to > account for 0. Read can return 0 if asked to read 0 bytes or if the connection is closed. If bytes_read == 0, we just return null for this call. https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket.h File runtime/bin/sync_socket.h (right): https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket... runtime/bin/sync_socket.h:10: #endif On 2017/04/11 23:04:47, siva wrote: > The socket* files seem to use > #if !defined(DART_IO_DISABLED) > ... > ... > ... > #endif // !defined(DART_IO_DISABLED) > > why not use the same pattern here too? That pattern is only used in the socket*.cc files, and the pattern here matches socket.h. Is this something we want to change in both files? https://codereview.chromium.org/2803543006/diff/70001/runtime/bin/sync_socket... runtime/bin/sync_socket.h:31: static SynchronousSocket* GetSocketIdNativeField(Dart_Handle socket); On 2017/04/11 23:04:47, siva wrote: > The get/set API is not uniform > set takes an intptr_t id > get returns SynchronousSocket* > > Maybe we should make them consistent (I see that the async socket API is also > similar, something to consider for another CL). Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
