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..73fc97813b8159348ca44ac00413e4b4aada9029 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 send out). This method negotiates how many requests (not bytes)

[Ryan Sleevi, 2014/10/05 23:24:05]

s/send out/sent out/

[yzshen1, 2014/10/06 05:17:01]

Thanks!

+  // this queue is able to store. If the queue is full, the service fails new
+  // requests directly with error code ERR_INSUFFICIENT_RESOURCES and discard

[Ryan Sleevi, 2014/10/05 23:24:05]

s/discard/discards/

[yzshen1, 2014/10/06 05:17:01]

Done.

+  // those packets. If the client wants to avoid such failure, it needs to keep

[Ryan Sleevi, 2014/10/05 23:24:06]

s/failure/failures/

[yzshen1, 2014/10/06 05:17:00]

Thanks! (Sorry, I should have been more careful.)

+  // 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);
+
   // 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,42 @@ 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);
 
   // 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

[Ryan Sleevi, 2014/10/05 23:24:06]

I'm not thrilled with documenting the value as -12

Please be aware that, as far as API guarantees go, //net error codes are NOT
guaranteed to be API-stable with respect to external consumption. They ARE
guaranteed to be source-compatible, but that's it. So, caveat coder.

[yzshen1, 2014/10/06 05:17:01]

I agree. I have talked with others about this. We need to define a set of Mojo
networking error codes, instead of just reusing the /net codes.

Thanks!

+  //   resource to complete the operation. One possible cause is that the client
+  //   tries to send too many packets in a short period of time.
+  SendTo(NetAddress dest_addr, uint8[] data) => (NetworkError result);
 };
 
 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
+  // non-negative number indicating how many bytes have been received.
+  OnReceived(NetworkError result, NetAddress? src_addr, uint8[]? data);
 };
 
 }