Add TCP socket mojo interfaces

mojo/services/public/interfaces/network/tcp_socket.mojom

Index: mojo/services/public/interfaces/network/tcp_socket.mojom
diff --git a/mojo/services/public/interfaces/network/tcp_socket.mojom b/mojo/services/public/interfaces/network/tcp_socket.mojom
new file mode 100644
index 0000000000000000000000000000000000000000..a64f10e9a498b722d1c2ea179bfef943ecd44b4a
--- /dev/null
+++ b/mojo/services/public/interfaces/network/tcp_socket.mojom
@@ -0,0 +1,78 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import "mojo/services/public/interfaces/network/net_address.mojom"
+import "mojo/services/public/interfaces/network/network_error.mojom"
+
+module mojo {
+
+struct TCPSocketInfo {
+  NetAddress local_address;
+  NetAddress remote_address;
+};
+
+// Represents a TCP socket. A socket can represent either a connection to a
+// remote host, or a local server socket waiting for incoming connections.
+//
+// For client connections:
+//  - Connect(), providing a pair of streams to use for data transfer.
+//  - Read and write from the streams as desired.
+//  - Close().
+//
+// For server connections:
+//  - Bind() to allocate a port.
+//  - Listen() to start listening for incoming connections.
+//  - For as long as you want to accept incoming connections:
+//    - Accept() to wait for and initiate a connection once one is available.
+//  - Close().
+interface TCPSocket {
+  // Binds this connection to a local port. The TCPSocket must not be bound,
+  // listening, or connected.
+  //
+  // This function is required to initialize a server socket for using Listen(),
+  // but is optional when using Connect() to initiate a client connection. On
+  // success, built_to will indicate the local address that was bound.

[Ryan Sleevi, 2014/09/30 20:48:08]

typo: built_to -> bound_to

+  Bind(NetAddress addr) => (NetworkError result, NetAddress? bound_to);
+
+  // Starts listening on this socket for incoming connection requests. The
+  // socket must be bound and not connected.
+  //
+  // The backlog is a hint indicating the number of incoming connections that
+  // can be queued up at one time waiting for Accept(). This queuing will
+  // happen when connections are incoming faster than Accept() can be called.

[Ryan Sleevi, 2014/09/30 21:35:28]

Note that in normal socket authoring, what you want is to use SOMAXCONN, and let
the system dynamcally determine an appropriate amount dependent on other values.

With this API, there's no way for an API author to know a sane/reasonable value
for backlog, and that can have impact as to the ability to optimize different
strategies here (e.g. for memory or high performance).

[brettw, 2014/09/30 21:50:43]

I was just copying the BSD Listen() function. I'm not sure how you want me to
change this.

+  Listen(int32 backlog) => (NetworkError result);

[Ryan Sleevi, 2014/09/30 20:48:08]

This creates an inconsistent interface. You can create a Listen socket that
supports Connect, which is an inconsistent layering/separation of
responsibilities.

It's unclear if you're trying to do this object oriented (as line 29 implies),
or if you're trying to mirror the BSD socket APIs (as implied by your naming and
choice of functions), but this seems like a logical inconsistency between the
two.

[brettw, 2014/09/30 21:06:17]

This object attempts to represent a sockfd. So you can either do listen or
connect on it. There is a mojo object that represents this sockfd. But I'm not
sure I undertand the object-oriented objection.

[viettrungluu, 2014/09/30 21:25:48]

So I now understand why I hate this API.

Fundamentally, the idea of mojo APIs should be that a message pipe represents a
capability.

This API only offers a very coarse capability. Any restrictions on powers would
have to be enforced by the service (or a proxy), not by the capability system.

For example, one may want to restrict:
1) the ability to bind (even for "client" sockets -- since doing so consumes a
resource, and may block others); or
2) the ability to listen at all.

This API doesn't offer a way to restrict either in a reasonable way.

[Ryan Sleevi, 2014/09/30 21:35:28]

From looking at the past work on Pepper and Extensions, it's struck me as a
little weird, with the interface trying to be some hybrid between functional and
OO.

That is, the BSD sockets API is functional, with the SOCKET (or int) always
passed along to all the methods, which are just a bunch of free functions.

This API (and those before it), try to bring some sugar to this, by making it an
object that you have (which is implicitly the first param, ala a |this|
pointer). However, this makes it somewhat weird, because

a) For some APIs, you don't have an object yet, so you need to create an object.
Sometimes this is seen as a "static creator function" (eg: BSD's "socket()" call
may become "Socket::Create(x, y, TCP)" or something), other times it's seen as
some other API entirely "SocketsAPI::CreateTCPSocket(x, y)".

b) Generally, OO designs try to make it difficult for objects to get in
inconsistent states, by separating out responsibilities. That is, this object is
a "listening" socket, this is a "reading/writing" socket, etc. This is not
something that the BSD API does (sockfd for everything), since a sockfd is an
opaque object. That is, this just highlights a tension between 'traditional' OO
design of Single-Responsibility and the 'functional' API design of 'opaque
objects'

I don't know the concrete suggestion here, but it's something you'll want/need
to figure out with your API. Do you want to be BSD-like, which is to say,
basically a bunch of free-floating static functions that take an explicit object
- or do you want to try to sugar things into logical objects with
responsibilities?

[brettw, 2014/09/30 22:16:17]

The goal is to make it straightforward to write a wrapper that provides the BSD
socket functions. Hence the current weirdness. This was also a requirement of
Pepper.

[Ryan Sleevi, 2014/10/01 04:05:23]

This function in particular is NOT a straightforward wrapper.

It misses some of the very advanced - and necessary - capabilities.

As an app author, I have no reasonable way to know what a sensible backlog is to
set, nor is there a way for the service (or proxy) to express limitations of
that capability.

That is, you want your backlog to be roughly related to the rate of incoming
connections and how long it takes you to service each incoming connection. I
have no way of knowing how long it will take a service to Accept() a socket,
even if I perfectly control my event loop. I also have no way of knowing what
the overheads are (for example, if I write a 4MB chunk of data, will that cause
a head-of-line blocking issue for my attempt to Accept() a socket? In today's
IPC system, IT WILL)

There's no API for knowing what a sensible default maximum may be (e.g.
SOMAXCONN).
There's no way to query what the service (or proxy) has limited the ultimate
maximum to be (e.g. /proc/sys/net/somaxconn)
There's no way to let the system manage whether or not accepted sockets are
ack'd or not.

From an implementation standpoint, I don't know whether the Listen+Accept
implementation is
a) Handled by the service itself (e.g. these calls to Listen or Accept are
wrappers to C++ code in the mojo service)
b) Handled as capabilities (e.g. on Linux, the TCPSocket represents an FD sent
over a descriptor, and all the calls actually happen in the calling process)

So I have no way of knowing whether Listen(10) means that the service will
continually call BSD-accept(), until it has 10 sockfds waiting for me to call
the Mojo-Accept(), or if it will only call BSD-accept() whenever I call
Mojo-Accept(). The latency overhead of BSD-accept() per Mojo-accept() means I
have further latencies in my loop that I can't predict.


From an interface perspective, I totally understand where you're coming from,
and absolutely agree that mirroring the BSD socket APIs is not only good, but
arguably NECESSARY if one is to write high performance applications. However, I
also share Trung's concern that this doesn't really reflect the Mojo-mideset of
capability passing, and I get especially confused when trying to reason about
how the implementation SHOULD look (as a consumer and as an implementor), given
the current documentation.

This is hard and messy, and I don't have a pat answer. At a minimum, more
documentation is needed, perhaps not in this header, to actually document what
are the guarantees and what isn't.

The next most concrete suggestion I would have is to reach out to net-dev@ AND
(not or) potential consumers of this API, and look at writing a hypothetical
sample application using this API, _BEFORE_ implementing. This is the sort of
API design discussion from http://mollyrocket.com/casey/stream_0029.html , and
would have immediately caught a number of the issues found when looking at the
Extensions Sockets API and, I believe, some of the Pepper Sockets APIs. Having
some fresh eyes play, just with pseudocode, will probably highlight similar
questions of "Well, what should I do or put here" 

+
+  // Accepts a connection. The socket must be listening.
+  //
+  // This function is called to accept an incoming connection.  The caller
+  // provides the streams that will be used to send and receive data on the
+  // connection.
+  //
+  // On success, the callback will be provided with the new socket that
+  // represents the connection. It will already be in a connected state and the
+  // data streams will be ready to use.
+  Accept(handle<data_pipe_consumer> send_stream,
+         handle<data_pipe_producer> receive_stream) =>
+      (NetworkError result, TCPSocket? socket, TCPSocketInfo? info);

[Ryan Sleevi, 2014/09/30 20:48:07]

What happens if there are no sockets pending acceptance? Does this go into an
eternal pending acceptance state with a dangling callback?

Is there any way for the API to indicate that additional connections should be
stopped/rejected, as there is in BSD, to indicate exceptional conditions (such
as DOS or high-load conditions)

[brettw, 2014/09/30 21:06:17]

Yes.
No. Can you point me to this?

+
+  // Connects the socket to the given address. The socket must not be listening
+  // or previously connected. Binding the socket beforehand is optional.
+  Connect(NetAddress addr,
+          handle<data_pipe_consumer> send_stream,
+          handle<data_pipe_producer> receive_stream) =>
+      (NetworkError result, TCPSocketInfo? info);
+
+  // Closes the connection or stops listening for connections, depending on
+  // whether this socket is in server or client mode. If Close() is not called,
+  // the connection will be implicitly closes when the TCPSocket has been
+  // deleted.
+  Close();

[Ryan Sleevi, 2014/09/30 20:48:08]

You'll want to rethink how this interface behaves.

In the BSD sockets API, you can shutdown for read, write, or both, and get
information about the shutdown state.

In a TLS world, Close() semantics normally imply an _asynchronous_ behaviour, in
that a TLS "CloseAlert" will be sent before the TCP connection is closed.
Indeed, in a TLS-on-TCP world, you can "Close" a socket (for writing), which
sends a CloseAlert, but still Read (to consume pre-existing/in flight
Application data), then Read an EOF (aka the TLS close alert), and reuse the TCP
connection.

From our lessons in the //net socket, and with work on SPDY, it's a regret of
ours that we didn't make the StreamSocket Close() interface asynchronous from
the start.

[brettw, 2014/09/30 21:06:16]

I don't follow entirely.

Connection reuse is something we can come back and add later, but I want the
current API to allow it to be done in an incremental fashion. Can you lay out
the API you want?

Note that there is no read or write method on this because these are done on the
data pipes the application passed to Connect/etc. These pipes can be closed
independently of the connection, which sounds like it covers at least some of
your use-case. Potentially later we could add a ReuseConnection function that
you could supply new streams for the existing connection.

[Ryan Sleevi, 2014/09/30 21:35:29]

Concrete proposals:
1) Make the API distinguish between closing for read and closing for write. This
is equivalent to shutdown() - http://linux.die.net/man/3/shutdown

2) Make the API asynchronous. That is, whether composing sockets or simply
implementing atop native APIs, shutdown() is not a guaranteed blocking
operation. Include a way to signal that the shutdown has completed.

2 is tricky, because, at least in //net, shutdown MAY be synchronous, and it'd
be "nice" to avoid a callback. However, for Mojo, I don't think that's something
supported/part of the design? Just calling it out.
The comments weren't so much about re-use, but trying to understand how these
semantics map onto the above socket APIs widely implemented.

That is, if shutting down the write socket is equivalent to SHUT_WR, it'd be
good to note that - and to guarantee that API behaviour (which is not implicit
in our //net sockets). Or to document that this part of the API may change in
future versions to support that behaviour, but that it's NOT implied in the
current version.

The asynchronous close is more useful if you ever wanted to layer multi-protocol
over a single TCP socket, and there ARE use cases for that (XMPP and
SMTP+STARTTLS). In this case, you'd compose a TLS socket (with it's own
read/write channels) atop the TCP socket (or, more aptly, the channels you
supplied when creating the TCP socket).

Eventually, when you want to Close() the TLS socket , you'd have an asynchronous
close method that would return only when it had enqueued the TLS close on the
write end (and, for security/correctness consumed the TLS close from the read
in), and then you could continue using the mojo channels from the TCP layer for
your plaintext communication.

In a TLS world (and, arguably, SPDY), a use case is sending a TLS close (or SPDY
GOAWAY), which notifies the peer you would like to wind things down, _but_ then
continuing to read application data / data frames until the peer acknowledges
your goaway/close with its own goaway/close.

At that point, you're now back to a 'simple' data flow, ala a TCP socket.

I mention all of this now because I assume, perhaps incorrectly, that you will
want to have similar concepts if and when you expose these higher level
protocols. If that's the case, then you should do what BSD sockets do, and
expose shutdown() [as in, I want you to go away] as something distinct from
close(), and allow both to have asynchronous implications.

[willchan no longer on Chromium, 2014/09/30 22:03:14]

Can you clarify more details on the asynchronous part? In BSD sockets, you can
close() a non-blocking socket even though the data has not been completely
transferred to the peer. It stays buffered in the kernel. For a TLS on TCP
socket, it's not immediately obvious to me what would be wrong with that.

I have vague recollections of you having issues with this WRT how SPDY layers on
top of TLS/TCP sockets, but I don't recall. Can you unpack that some more?

+
+  // Controls whether small writes are coalesced to make TCP segments, and
+  // instead delivers data immediately. The default value is true (coalescing
+  // enabled).
+  SetCoaleseWrites(bool coalesce);

[Ryan Sleevi, 2014/09/30 20:48:07]

Why not call this what it is (and which most socket APIs reflect it is), namely,
Nagle's algorithm?

If you're not using Nagles, but something else, then that's a VERY surprising
thing for API consumers, and really needs to be spelt out more. And I suspect
there'd be pushback on it.

[brettw, 2014/09/30 21:06:17]

I mentioned this in the comment.
We're not "using" anything at this level, this flag just gets passed to the
net/OS layers.

[Ryan Sleevi, 2014/09/30 21:35:29]

Did you mean the existing comment, or an update?

Just because there's different strategies for coalescing.
Then spell out that it's nagles as the flag you're passing down.
It's a smell that //net doesn't (and this has surprised some)

[brettw, 2014/09/30 21:50:43]

This was added in a comment in the patch that was uploaded after you looked at
this. I don't want to name the function SetUseNagles or something since I think
that is confusing.

+};
+
+}