Added synchronous socket implementation to dart:io.

runtime/bin/sync_socket_patch.dart

+// Copyright (c) 2017, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+@patch
+class RawSynchronousSocket {
+  @patch
+  static RawSynchronousSocket connectSync(host, int port) {
+    return _RawSynchronousSocket.connectSync(host, port);
+  }
+}
+
+class _RawSynchronousSocket implements RawSynchronousSocket {
+  final _NativeSynchronousSocket _socket;
+
+  _RawSynchronousSocket(this._socket) {}
+
+  static RawSynchronousSocket connectSync(host, int port) {
+    _throwOnBadPort(port);
+    return new _RawSynchronousSocket(
+        _NativeSynchronousSocket.connectSync(host, port));
+  }
+
+  InternetAddress get address => _socket.address;
+  int get port => _socket.port;
+  InternetAddress get remoteAddress => _socket.remoteAddress;
+  int get remotePort => _socket.remotePort;
+
+  int available() => _socket.available;
+
+  void closeSync() => _socket.closeSync();
+
+  int readIntoSync(List<int> buffer, [int start = 0, int end]) =>
+      _socket.readIntoSync(buffer, start, end);
+
+  List<int> readSync(int bytes) => _socket.readSync(bytes);
+
+  void shutdown(SocketDirection direction) => _socket.shutdown(direction);
+
+  void writeFromSync(List<int> buffer, [int start = 0, int end]) =>
+      _socket.writeFromSync(buffer, start, end);
+}
+
+// The NativeFieldWrapperClass1 can not be used with a mixin, due to missing
+// implicit constructor.
+class _NativeSynchronousSocketNativeWrapper extends NativeFieldWrapperClass1 {}
+
+// The _NativeSynchronousSocket class encapsulates a synchronous OS socket.
+class _NativeSynchronousSocket extends _NativeSynchronousSocketNativeWrapper {
+  // Socket close state.
+  bool isClosed = false;
+  bool isClosedRead = false;
+  bool isClosedWrite = false;
+
+  // Holds the address used to connect the socket.
+  InternetAddress localAddress;
+
+  // Holds the port of the socket, 0 if not known.
+  int localPort = 0;
+
+  _ReadWriteResourceInfo resourceInfo;

[zra, 2017/04/07 16:29:41]

It looks like you need to call _SocketResourceInfo.SocketClosed() on this at
some point.

[bkonyi, 2017/04/10 19:20:06]

Added SocketClosed() call in CloseSync()

+
+  InternetAddress get address => localAddress;
+  int get available => nativeAvailable();
+
+  int get port {
+    if (localPort != 0) return localPort;

[zra, 2017/04/07 16:29:41]

Please put single line ifs in {}

[bkonyi, 2017/04/10 19:20:06]

Done.

+    if (isClosed) throw const SocketException.closed();
+    var result = nativeGetPort();
+    if (result is OSError) throw result;
+    return localPort = result;
+  }
+
+  InternetAddress get remoteAddress {
+    if (isClosed) throw const SocketException.closed();
+    var result = nativeGetRemotePeer();
+    if (result is OSError) throw result;
+    var addr = result[0];
+    var type = new InternetAddressType._from(addr[0]);
+    return new _InternetAddress(addr[1], null, addr[2]);
+  }
+
+  int get remotePort {
+    if (isClosed) throw const SocketException.closed();
+    var result = nativeGetRemotePeer();
+    if (result is OSError) throw result;
+    return result[1];
+  }
+
+  static _NativeSynchronousSocket connectSync(host, int port) {

[zra, 2017/04/07 16:29:41]

statics like this that generate an instance I think we tend to put before
instance methods.

[bkonyi, 2017/04/10 19:20:06]

Ah, I wasn't sure where static methods like this should go relative to the
get/set methods. Moved.

+    List<_InternetAddress> addresses = null;
+    var error = null;
+    if (host is _InternetAddress) {
+      addresses = [host];
+    } else {
+      try {
+        addresses = lookup(host);
+      } catch (e) {
+        error = e;
+      }
+      if (error != null || addresses == null || addresses.isEmpty) {
+        throw createError(error, "Failed host lookup: '$host'");
+      }
+    }
+    assert(addresses is List);
+    var it = addresses.iterator;
+    _NativeSynchronousSocket connectNext() {
+      if (!it.moveNext()) {
+        // Could not connect. Throw the first connection error we encountered.
+        assert(error != null);
+        throw error;
+      }
+      var address = it.current;
+      var socket = new _NativeSynchronousSocket();
+      socket.localAddress = address;
+      var result = socket.nativeCreateConnectSync(address._in_addr, port);
+      if (result is OSError) {
+        // Keep first error, if present.
+        if (error == null) {
+          int errorCode = result.errorCode;

[zra, 2017/04/07 16:29:40]

errorCode doesn't appear to be used.

[bkonyi, 2017/04/10 19:20:06]

Removed.

+          error = createError(result, "Connection failed", address, port);
+        }
+        return connectNext();
+      } else {
+        // Query the local port, for error messages.
+        try {
+          socket.port;
+        } catch (e) {
+          error = createError(e, "Connection failed", address, port);

[zra, 2017/04/07 16:29:41]

if (error == null)

[bkonyi, 2017/04/10 19:20:05]

Done.

+          return connectNext();
+        }
+        setupResourceInfo(socket);
+      }
+      return socket;
+    }
+
+    return connectNext();
+  }
+
+  void closeSync() {
+    if (!isClosed) {
+      nativeCloseSync();
+      isClosed = true;
+    }
+  }
+
+  // Create the appropriate error/exception from different returned
+  // error objects.
+  static createError(error, String message,
+      [InternetAddress address, int port]) {
+    if (error is OSError) {
+      return new SocketException(message,
+          osError: error, address: address, port: port);
+    } else {
+      return new SocketException(message, address: address, port: port);
+    }
+  }
+
+  static List<InternetAddress> lookup(String host,
+      {InternetAddressType type: InternetAddressType.ANY}) {
+    var response = nativeLookupRequest(host, type._value);
+    if (response is OSError) {
+      throw response;
+    }
+    // TODO(bkonyi) do we need this first empty element? Or is this something specific

[zra, 2017/04/07 16:29:41]

I don't see what this is used for either. Let's get rid of it.

[bkonyi, 2017/04/10 19:20:06]

Done.

+    // for io_service?
+    return response.skip(1).map((result) {
+      var type = new InternetAddressType._from(result[0]);
+      return new _InternetAddress(result[1], host, result[2]);
+    }).toList();
+  }
+
+  int readIntoSync(List<int> buffer, int start, int end) {

[zra, 2017/04/07 16:29:40]

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.

[bkonyi, 2017/04/10 19:20:06]

Done.

+    if (buffer == null) {
+      throw new ArgumentError("Illegal buffer: buffer cannot be null");
+    }
+    if (start != null && ((start >= buffer.length) || (start < 0))) {
+      throw new ArgumentError("Illegal start $start");
+    }
+    if ((start != null && end != null) &&
+        ((start > end) || (end > buffer.length))) {
+      throw new ArgumentError("Illegal range [$start, $end)");
+    }
+    int len = 0;
+    if (start == null) {
+      start = 0;
+      len = buffer.length;
+    } else if (end == null) {
+      len = buffer.length - start;
+    } else {
+      len = end - start;
+    }
+    List<int> result = readSync(len);
+    len = min(result.length, len);
+
+    for (int i = start; i < start + len; i++) {
+      buffer[i] = result[i - start];
+    }
+    return result.length;
+  }
+
+  List<int> readSync(int len) {
+    if (len != null && len <= 0) {
+      throw new ArgumentError("Illegal length $len");
+    }
+    if (isClosed) return null;
+    if (len == 0) return null;
+    var result = nativeRead(len);
+    if (result is OSError) {
+      throw result;
+      return null;
+    }
+    if (result != null) {
+      assert(resourceInfo != null);
+      if (resourceInfo != null) {
+        resourceInfo.totalRead += result.length;
+      }
+    }
+    assert(resourceInfo != null);

[zra, 2017/04/07 16:29:41]

If you move this assert above if (result != null) then you only need it once.

[bkonyi, 2017/04/10 19:20:06]

Done.

+    if (resourceInfo != null) {
+      resourceInfo.didRead();
+    }
+    return result;
+  }
+
+  static void setupResourceInfo(_NativeSynchronousSocket socket) {
+    socket.resourceInfo = new _SocketResourceInfo(socket);
+  }
+
+  void shutdown(SocketDirection direction) {
+    if (!isClosed) {

[zra, 2017/04/07 16:29:41]

if (isClosed) return

then un-indent, here and below.

[bkonyi, 2017/04/10 19:20:06]

Done.

+      switch (direction) {
+        case SocketDirection.RECEIVE:
+          shutdownRead();
+          break;
+        case SocketDirection.SEND:
+          shutdownWrite();
+          break;
+        case SocketDirection.BOTH:
+          closeSync();
+          break;
+        default:
+          throw new ArgumentError(direction);
+      }
+    }
+  }
+
+  void shutdownRead() {
+    if (!isClosed) {
+      if (isClosedWrite) {
+        closeSync();
+      } else {
+        nativeShutdownRead();

[zra, 2017/04/07 16:29:41]

What if isClosedRead is already true?

[bkonyi, 2017/04/10 19:20:05]

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.

+      }
+      isClosedRead = true;
+    }
+  }
+
+  void shutdownWrite() {
+    if (!isClosed) {
+      if (isClosedRead) {
+        closeSync();
+      } else {
+        nativeShutdownWrite();
+      }
+      isClosedWrite = true;
+    }
+  }
+
+  void writeFromSync(List<int> buffer, int start, int end) {

[zra, 2017/04/07 16:29:41]

Have a look at _RandomAccessFile.writeFromSync for ideas about how to clean up
this function.

[bkonyi, 2017/04/10 19:20:05]

Done.

+    if (isClosed) return;
+    int offset = start;
+    int bytes = null;
+    if (buffer is! List) throw new ArgumentError();

[zra, 2017/04/07 16:29:41]

Please give an error message.

[bkonyi, 2017/04/10 19:20:06]

Acknowledged.

+    if (offset == null) offset = 0;
+    if (end == null) {

[zra, 2017/04/07 16:29:41]

Can you use RangeError.checkValidRange()?

[bkonyi, 2017/04/10 19:20:06]

I didn't know that existed... super useful! Done.

+      if (offset > (buffer.length)) {
+        throw new RangeError.value(offset);
+      }
+      bytes = buffer.length - offset;
+    } else {
+      bytes = end - offset;
+    }
+    if (offset is! int || bytes is! int) {

[zra, 2017/04/07 16:29:40]

This should probably be up at the top of the function.

[bkonyi, 2017/04/10 19:20:06]

Acknowledged.

+      throw new ArgumentError("Invalid arguments to write on Socket");
+    }
+    if (bytes == 0) return;
+    if (offset < 0) throw new RangeError.value(offset);
+    if (bytes < 0) throw new RangeError.value(bytes);
+    if ((offset + bytes) > buffer.length) {
+      throw new RangeError.value(offset + bytes);
+    }
+    _BufferAndStart bufferAndStart =
+        _ensureFastAndSerializableByteData(buffer, offset, offset + bytes);
+    var result =
+        nativeWrite(bufferAndStart.buffer, bufferAndStart.start, bytes);
+    if (result is OSError) {
+      throw result;
+    }
+    // TODO(bkonyi) we don't currently handle short writes or reads.
+    // The result may be negative, if we forced a short write for testing
+    // purposes. Negate the result to get the actual number of bytes written.
+    if (result < 0) result = -result;

[zra, 2017/04/07 16:29:41]

Let's remove this logic if it isn't supported.

[bkonyi, 2017/04/10 19:20:05]

Done.

+    assert(resourceInfo != null);
+    if (resourceInfo != null) {
+      resourceInfo.addWrite(result);
+    }
+  }
+
+  // Native method declarations.
+  static nativeLookupRequest(host, int type)
+      native "SynchronousSocket_LookupRequest";
+  nativeCreateConnectSync(host, int port)
+      native "SynchronousSocket_CreateConnectSync";
+  nativeAvailable() native "SynchronousSocket_Available";
+  nativeCloseSync() native "SynchronousSocket_CloseSync";
+  int nativeGetPort() native "SynchronousSocket_GetPort";
+  List nativeGetRemotePeer() native "SynchronousSocket_GetRemotePeer";
+  nativeRead(int len) native "SynchronousSocket_Read";
+  nativeShutdownRead() native "SynchronousSocket_ShutdownRead";
+  nativeShutdownWrite() native "SynchronousSocket_ShutdownWrite";
+  nativeWrite(List<int> buffer, int offset, int bytes)
+      native "SynchronousSocket_WriteList";
+}