Mojo UDP socket API definition review.

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

Index: mojo/services/public/interfaces/network/udp_socket.mojom
diff --git a/mojo/services/public/interfaces/network/udp_socket.mojom b/mojo/services/public/interfaces/network/udp_socket.mojom
index 6f0cff3efe324a30aa8abca656222b3915c3b920..daae0a49c2478b96cf6903adc9219cf549b7179d 100644
--- a/mojo/services/public/interfaces/network/udp_socket.mojom
+++ b/mojo/services/public/interfaces/network/udp_socket.mojom
@@ -15,6 +15,7 @@ module mojo {
 // - (optional) Set options which are allowed after Bind().
 // - Send / request to receive packets. Received packets will be delivered to
 //   UDPSocketClient.OnReceived().
+
 [Client=UDPSocketClient]
 interface UDPSocket {
   // Allows the socket to share the local address to which it will be bound with
@@ -28,20 +29,28 @@ interface UDPSocket {
   // returned in |bound_addr|.
   Bind(NetAddress addr) => (NetworkError result, NetAddress? bound_addr);
 
-  // Sets the send buffer size (in bytes) for the socket. The socket must be
+  // Sets the OS send buffer size (in bytes) for the socket. The socket must be
   // bound.
-  //
-  // Note: This is only treated as a hint. Even if it succeeds, the service
-  // doesn't guarantee it will conform to the size.
   SetSendBufferSize(uint32 size) => (NetworkError result);
 
-  // Sets the receive buffer size (in bytes) for the socket. The socket must be
-  // bound.
-  //
-  // Note: This is only treated as a hint. Even if it succeeds, the service
-  // doesn't guarantee it will conform to the size.
+  // Sets the OS receive buffer size (in bytes) for the socket. The socket must
+  // be bound.
   SetReceiveBufferSize(uint32 size) => (NetworkError result);
 
+  // Negotiates the maximum number of pending SendTo() requests. If
+  // |requested_size| is set to 0, this method queries the current settings.
+  //
+  // The service stores SendTo() requests in a queue while they are waiting to
+  // be executed (i.e., while they are waiting to be placed in the OS send
+  // buffer and sent out). This method negotiates how many requests (not bytes)
+  // this queue is able to store. If the queue is full, the service fails new
+  // requests directly with error code ERR_INSUFFICIENT_RESOURCES and discards
+  // those packets. If the client wants to avoid such failures, it needs to keep
+  // track of how many SendTo() calls are pending and make sure the number
+  // doesn't exceed the result of this method.
+  NegotiateMaxPendingSendRequests(uint32 requested_size)
+      => (uint32 actual_size);

[willchan no longer on Chromium, 2014/10/06 20:37:54]

I'm being nitpicky, but long-term, you have to document the edge cases better.
Since this asynchronously completes, you need guidance on what happens with
SendTo() calls invoked before the asynchronous completion of the send queue
negotiation.

[yzshen1, 2014/10/07 21:07:21]

Because the service side doesn't know about when exactly the callback is run at
the client side, I think the reasonable behavior is those SendTo() calls will be
handled using the new send queue settings.

+
   // Notifies that the client is ready to accept |number| of packets.
   // Correspondingly, OnReceived() of the UDPSocketClient interface will be
   // called |number| times (errors also count), unless the connection is closed
@@ -56,19 +65,43 @@ interface UDPSocket {
   //   ...
   //   service->ReceiveMorePackets(3);
   //   // The client expects 4 more calls to OnReceived().
+  //
+  // Please note that how ReceiveMorePackets() is used will affect performance
+  // significantly. For example:
+  //   // Approach 1:
+  //   service->ReceiveMorePackets(3);
+  //   // OnReceived() is called.
+  //   // OnReceived() is called.
+  //   // OnReceived() is called.
+  //
+  //   // Approach 2:
+  //   service->ReceiveMorePackets(1);
+  //   // OnReceived() is called.
+  //   service->ReceiveMorePackets(1);
+  //   // OnReceived() is called.
+  //   service->ReceiveMorePackets(1);
+  //   // OnReceived() is called.
+  //
+  // It is very likely that approach 1 will perform better than approach 2,
+  // because in approach 2 getting every packet takes at least the time of a
+  // round trip to the service side.
   ReceiveMorePackets(uint32 number);

[willchan no longer on Chromium, 2014/10/06 20:37:54]

Sorry to be pedantic, but you should probably use say message or datagram. It's
not necessary an IP packet, because of fragmentation.

[yzshen1, 2014/10/07 21:07:21]

Sounds good, thanks!

 
   // Sends data to the specified destination. The socket must be bound.
-  // The method doesn't report the result of the operation.
-  SendToAndForget(NetAddress addr, uint8[] data);
-
-  // Sends data to the specified destination. The socket must be bound.
-  SendTo(NetAddress addr, uint8[] data) => (NetworkError result);
+  // On success, |result.code| is a non-negative number indicating how many
+  // bytes have been written. Otherwise, it is a network error code, including
+  // (but not limited to):
+  // - ERR_INSUFFICIENT_RESOURCES (-12): The service doesn't have sufficient
+  //   resource to complete the operation. One possible cause is that the client
+  //   tries to send too many packets in a short period of time.
+  // TODO(yzshen): Formalize Mojo networking error codes.
+  SendTo(NetAddress dest_addr, uint8[] data) => (NetworkError result);

[willchan no longer on Chromium, 2014/10/06 20:37:54]

I have more comments on long-term stuff for SendTo() and OnReceived() to be more
efficient. Do you want them now, or should I note them elsewhere for potential
future TODOs?

[yzshen1, 2014/10/07 21:07:21]

And I am happy to listen to your suggestion at any time. :) You could put it in
this CL or write a separate mail/doc.
Another choice: I am waiting for the mojo framework to support structural data
over DataPipe. And I will make you the reviewer once I make a version of UDP API
using DataPipe.

Thanks again!

 };
 
 interface UDPSocketClient {
-  // |addr| and |data| are non-NULL on success.
-  OnReceived(NetworkError result, NetAddress? addr, uint8[]? data);
+  // On success, |src_addr| and |data| are non-NULL, |result.code| is a

[wtc, 2014/10/06 17:41:12]

Nit: add "and" before "|result.code|".

[yzshen1, 2014/10/07 21:07:21]

Done.

+  // non-negative number indicating how many bytes have been received.

[wtc, 2014/10/06 17:41:12]

Nit: document what happens on failure (|result.code| is a network error code).

[yzshen1, 2014/10/07 21:07:21]

Done.

+  OnReceived(NetworkError result, NetAddress? src_addr, uint8[]? data);
 };
 
 }