Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
612403003:
Mojo UDP socket API definition review. (Closed)
|
Created:
6 years, 2 months ago by yzshen1 Modified:
6 years, 2 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionMojo UDP socket API definition review.
BUG=402671
TEST=None
Committed: https://crrev.com/2087fb8402cbd9512aab3936661d15c80b62b543
Cr-Commit-Position: refs/heads/master@{#298606}
Patch Set 1 #
Total comments: 9
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 8
Patch Set 6 : #
Total comments: 10
Patch Set 7 : #
Messages
Total messages: 27 (5 generated)
willchan@chromium.org changed reviewers: + willchan@chromium.org
https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/udp_socket.mojom:35: // Note: This is only treated as a hint. Even if it succeeds, the service What does this note mean? It's not obvious if the send buffer here refers to the OS kernel send buffer or the mojo service send buffer (if one exists, does it?). How are you handling backpressure (kernel buffers filling up, and notifying when they have space again)?
https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/udp_socket.mojom:35: // Note: This is only treated as a hint. Even if it succeeds, the service On 2014/09/30 22:17:33, willchan OOO until 03-22-15 wrote: > What does this note mean? This is a note copied from the Pepper API, because Ryan mentioned that we actually cannot guarantee that the buffer size setting is respected on all platforms (IIRC). Therefore, it basically means "I will try my best but I don't guarantee anything". :) > > It's not obvious if the send buffer here refers to the OS kernel send buffer or > the mojo service send buffer (if one exists, does it?). It is the OS buffer. The mojo service currently does has a queue for packets that are waiting to be passed to net::UDPServerSocket::SendTo(). I didn't expose that setting -- it is (randomly set as) 32 packets currently. I have a comment for SendTo() (The comment hasn't been landed.): """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.""" https://codereview.chromium.org/596383002/diff/40001/mojo/services/public/int... > > How are you handling backpressure (kernel buffers filling up, and notifying when > they have space again)? Do you mean the callback of net::UDPServerSocket::SendTo() won't be run until kernel buffer has some available space? When net::UDPServerSocket::SendTo() is pending, incoming packets from the mojo client will be stored in the mojo service's pending-send-request queue. If that queue reaches a certain length, new packets from the mojo client will be discarded and an "insufficient resource" error is returned to the client side. I think it would be nicer if we help the client to figure out how fast it could send packets instead of just discarding packets. But I haven't got a very elegant way to do that yet. Do you have any good idea? Thanks!
https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/udp_socket.mojom:35: // Note: This is only treated as a hint. Even if it succeeds, the service On 2014/09/30 22:55:09, yzshen1 wrote: > On 2014/09/30 22:17:33, willchan OOO until 03-22-15 wrote: > > What does this note mean? > > This is a note copied from the Pepper API, because Ryan mentioned that we > actually cannot guarantee that the buffer size setting is respected on all > platforms (IIRC). Therefore, it basically means "I will try my best but I don't > guarantee anything". :) > > > > > It's not obvious if the send buffer here refers to the OS kernel send buffer > or > > the mojo service send buffer (if one exists, does it?). > It is the OS buffer. Do you think it's worthwhile to clarify that in the comments? I suspect so, since I imagine there will be Mojo services that do their own buffering atop the OS. > > The mojo service currently does has a queue for packets that are waiting to be > passed to net::UDPServerSocket::SendTo(). I didn't expose that setting -- it is > (randomly set as) 32 packets currently. I have a comment for SendTo() (The > comment hasn't been landed.): > > """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.""" > > https://codereview.chromium.org/596383002/diff/40001/mojo/services/public/int... > > > > > > How are you handling backpressure (kernel buffers filling up, and notifying > when > > they have space again)? > > Do you mean the callback of net::UDPServerSocket::SendTo() won't be run until > kernel buffer has some available space? > > When net::UDPServerSocket::SendTo() is pending, incoming packets from the mojo > client will be stored in the mojo service's pending-send-request queue. If that > queue reaches a certain length, new packets from the mojo client will be > discarded and an "insufficient resource" error is returned to the client side. > > I think it would be nicer if we help the client to figure out how fast it could > send packets instead of just discarding packets. But I haven't got a very > elegant way to do that yet. Do you have any good idea? Thanks! > Discarding packets at the client is a very poor option :) So I agree we should do something about this. In a single-process non-blocking world, what you would do is watch the file descriptor for it to become writable. At that point, you'd write the message to the UDP socket, and it wouldn't get discarded. The way we handle this in Chromium's network stack is we don't have asynchronous I/O completion (the proactor model), but instead we use the reactor model. We synchronously tell the client whether or not the write() will succeed, and have asynchronous notification of _writability_ (as opposed to asynchronous write _completion_). You can emulate this by making the client a bit thicker so it can handle the reactor model. It can buffer messages in the client, and the IPC messages just do ACKs to signal when the Mojo service has consumed the message. The thick client can keep accepting messages until its buffer fills up, and it can fire a "writable" event after receiving an ACK from the service. Of course, this means having a thick client on top of the raw Mojo client.
On 2014/10/01 00:56:46, willchan OOO until 03-22-15 wrote: > https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... > File mojo/services/public/interfaces/network/udp_socket.mojom (right): > > https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... > mojo/services/public/interfaces/network/udp_socket.mojom:35: // Note: This is > only treated as a hint. Even if it succeeds, the service > On 2014/09/30 22:55:09, yzshen1 wrote: > > On 2014/09/30 22:17:33, willchan OOO until 03-22-15 wrote: > > > What does this note mean? > > > > This is a note copied from the Pepper API, because Ryan mentioned that we > > actually cannot guarantee that the buffer size setting is respected on all > > platforms (IIRC). Therefore, it basically means "I will try my best but I > don't > > guarantee anything". :) > > > > > > > > It's not obvious if the send buffer here refers to the OS kernel send buffer > > or > > > the mojo service send buffer (if one exists, does it?). > > It is the OS buffer. > > Do you think it's worthwhile to clarify that in the comments? I suspect so, > since I imagine there will be Mojo services that do their own buffering atop the > OS. > > > > > The mojo service currently does has a queue for packets that are waiting to be > > passed to net::UDPServerSocket::SendTo(). I didn't expose that setting -- it > is > > (randomly set as) 32 packets currently. I have a comment for SendTo() (The > > comment hasn't been landed.): > > > > """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.""" > > > > > https://codereview.chromium.org/596383002/diff/40001/mojo/services/public/int... > > > > > > > > > > How are you handling backpressure (kernel buffers filling up, and notifying > > when > > > they have space again)? > > > > Do you mean the callback of net::UDPServerSocket::SendTo() won't be run until > > kernel buffer has some available space? > > > > When net::UDPServerSocket::SendTo() is pending, incoming packets from the mojo > > client will be stored in the mojo service's pending-send-request queue. If > that > > queue reaches a certain length, new packets from the mojo client will be > > discarded and an "insufficient resource" error is returned to the client side. > > > > I think it would be nicer if we help the client to figure out how fast it > could > > send packets instead of just discarding packets. But I haven't got a very > > elegant way to do that yet. Do you have any good idea? Thanks! > > > > Discarding packets at the client is a very poor option :) So I agree we should > do something about this. In a single-process non-blocking world, what you would > do is watch the file descriptor for it to become writable. At that point, you'd > write the message to the UDP socket, and it wouldn't get discarded. > > The way we handle this in Chromium's network stack is we don't have asynchronous > I/O completion (the proactor model), but instead we use the reactor model. We > synchronously tell the client whether or not the write() will succeed, and have > asynchronous notification of _writability_ (as opposed to asynchronous write > _completion_). (I probably haven't fully understood.) I understand that the underlying implementation uses the readable/writeable notifications of the socket fd (on Linux, Windows is different). But the public interface of net::UDPServerSocket (and other net/ socket classes) doesn't work that way, it provides read/write completion notifications, which is more similar to the Windows I/O model. > > You can emulate this by making the client a bit thicker so it can handle the > reactor model. It can buffer messages in the client, and the IPC messages just > do ACKs to signal when the Mojo service has consumed the message. The thick > client can keep accepting messages until its buffer fills up, and it can fire a > "writable" event after receiving an ACK from the service. > > Of course, this means having a thick client on top of the raw Mojo client. Providing a client-side library to wrap the "raw" mojo API is not uncommon. However, one issue of buffering at the client side is that we cannot return the real OS error to the client. For example: - the client calls the UDP API to send a packet; the packet is buffered at the client side; the API returns a success to the client. - the packet is transferred to the mojo service side. - the service calls net::UDPServerSocket::SendTo() and gets a failure. - what should we do with the failure? it doesn't seem nice to swallow it silently. I will think more about this issue, and any suggestion will be appreciated! Thanks!
On 2014/10/01 06:47:11, yzshen1 wrote: > On 2014/10/01 00:56:46, willchan OOO until 03-22-15 wrote: > > > https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... > > File mojo/services/public/interfaces/network/udp_socket.mojom (right): > > > > > https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... > > mojo/services/public/interfaces/network/udp_socket.mojom:35: // Note: This is > > only treated as a hint. Even if it succeeds, the service > > On 2014/09/30 22:55:09, yzshen1 wrote: > > > On 2014/09/30 22:17:33, willchan OOO until 03-22-15 wrote: > > > > What does this note mean? > > > > > > This is a note copied from the Pepper API, because Ryan mentioned that we > > > actually cannot guarantee that the buffer size setting is respected on all > > > platforms (IIRC). Therefore, it basically means "I will try my best but I > > don't > > > guarantee anything". :) > > > > > > > > > > > It's not obvious if the send buffer here refers to the OS kernel send > buffer > > > or > > > > the mojo service send buffer (if one exists, does it?). > > > It is the OS buffer. > > > > Do you think it's worthwhile to clarify that in the comments? I suspect so, > > since I imagine there will be Mojo services that do their own buffering atop > the > > OS. > > > > > > > > The mojo service currently does has a queue for packets that are waiting to > be > > > passed to net::UDPServerSocket::SendTo(). I didn't expose that setting -- it > > is > > > (randomly set as) 32 packets currently. I have a comment for SendTo() (The > > > comment hasn't been landed.): > > > > > > """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.""" > > > > > > > > > https://codereview.chromium.org/596383002/diff/40001/mojo/services/public/int... > > > > > > > > > > > > > > How are you handling backpressure (kernel buffers filling up, and > notifying > > > when > > > > they have space again)? > > > > > > Do you mean the callback of net::UDPServerSocket::SendTo() won't be run > until > > > kernel buffer has some available space? > > > > > > When net::UDPServerSocket::SendTo() is pending, incoming packets from the > mojo > > > client will be stored in the mojo service's pending-send-request queue. If > > that > > > queue reaches a certain length, new packets from the mojo client will be > > > discarded and an "insufficient resource" error is returned to the client > side. > > > > > > I think it would be nicer if we help the client to figure out how fast it > > could > > > send packets instead of just discarding packets. But I haven't got a very > > > elegant way to do that yet. Do you have any good idea? Thanks! > > > > > > > Discarding packets at the client is a very poor option :) So I agree we should > > do something about this. In a single-process non-blocking world, what you > would > > do is watch the file descriptor for it to become writable. At that point, > you'd > > write the message to the UDP socket, and it wouldn't get discarded. > > > > The way we handle this in Chromium's network stack is we don't have > asynchronous > > I/O completion (the proactor model), but instead we use the reactor model. We > > synchronously tell the client whether or not the write() will succeed, and > have > > asynchronous notification of _writability_ (as opposed to asynchronous write > > _completion_). > > (I probably haven't fully understood.) > I understand that the underlying implementation uses the readable/writeable > notifications of the socket fd (on Linux, Windows is different). But the public > interface of net::UDPServerSocket (and other net/ socket classes) doesn't work > that way, it provides read/write completion notifications, which is more similar > to the Windows I/O model. > > > > > You can emulate this by making the client a bit thicker so it can handle the > > reactor model. It can buffer messages in the client, and the IPC messages just > > do ACKs to signal when the Mojo service has consumed the message. The thick > > client can keep accepting messages until its buffer fills up, and it can fire > a > > "writable" event after receiving an ACK from the service. > > > > Of course, this means having a thick client on top of the raw Mojo client. > > Providing a client-side library to wrap the "raw" mojo API is not uncommon. > However, one issue of buffering at the client side is that we cannot return the > real OS error to the client. > For example: > - the client calls the UDP API to send a packet; the packet is buffered at the > client side; the API returns a success to the client. > - the packet is transferred to the mojo service side. > - the service calls net::UDPServerSocket::SendTo() and gets a failure. > - what should we do with the failure? it doesn't seem nice to swallow it > silently. After some more thoughts: we just don't return a success to the client at step#1, and wait until the server side returns the real error code. Yeah. I think this is doable. I will work on it and update the CL. > > I will think more about this issue, and any suggestion will be appreciated! > Thanks!
wtc@chromium.org changed reviewers: + wtc@chromium.org
Review comments on patch set 1: The API seems reasonable. https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/net_address.mojom (right): https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/net_address.mojom:24: uint8[16] addr; IMPORTANT: this is missing the sin6_flowinfo and sin6_scope_id members of struct sockaddr_in6. Are you sure they are not needed? (I haven't thought about this carefully.) https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/net_address.mojom:33: NetAddressIPv6? ipv6; Is this the equivalent of a C union? https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/udp_socket.mojom:71: // |addr| and |data| are non-NULL on success. Nit: document what |addr| is, or rename it |peer_addr| or |sender_addr| to be more descriptive.
Thanks, Wan-Teh! https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/net_address.mojom (right): https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/net_address.mojom:24: uint8[16] addr; On 2014/10/02 02:00:51, wtc wrote: > > IMPORTANT: this is missing the sin6_flowinfo and sin6_scope_id members of struct > sockaddr_in6. Are you sure they are not needed? (I haven't thought about this > carefully.) This is the same as the Pepper API. IIRC, Ryan suggested to omit those fields because they were not supported consistently on all platforms. What do you think? And if we need they later, we can introduce them then (as optional fields). https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/net_address.mojom:33: NetAddressIPv6? ipv6; On 2014/10/02 02:00:51, wtc wrote: > > Is this the equivalent of a C union? It is less efficient than union space-wise, but the idea is the same: ipv4 and ipv6 are two separate struct pointers, whether they are NULL depends on the value of |family|. https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/612403003/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/udp_socket.mojom:71: // |addr| and |data| are non-NULL on success. On 2014/10/02 02:00:52, wtc wrote: > > Nit: document what |addr| is, or rename it |peer_addr| or |sender_addr| to be > more descriptive. Done. I used "src_addr" and also changed the parameter of SendTo/SendToAndForget() to "dest_addr". Thanks!
yzshen@chromium.org changed reviewers: + brettw@chromium.org
Hi, Will and Wan-Teh. I think I have followed your suggestions and made changes accordingly. Please take another look. And adding Brett as reviewer.
Patch set 4 LGTM. Please consider my review as a drive-by review, unless willchan or brettw doesn't have time to review this. https://codereview.chromium.org/612403003/diff/60001/mojo/services/public/int... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/612403003/diff/60001/mojo/services/public/int... mojo/services/public/interfaces/network/udp_socket.mojom:36: // doesn't guarantee it will conform to the size. I believe this note can be deleted. If this note is referring to the problem on Windows, net::UDPSocketWin::SetSendBufferSize already detects this problem and will return ERR_SOCKET_SEND_BUFFER_SIZE_UNCHANGEABLE. If some other platform has a similar problem, then we can keep this note but please try to find out what that platform is and add a reference to a detailed description of the problem. https://codereview.chromium.org/612403003/diff/60001/mojo/services/public/int... mojo/services/public/interfaces/network/udp_socket.mojom:60: // Notifies that the client is ready to accept |number| of packets. Nit: it would be good to explain why it is useful to pass a |number| > 1. The reason was not obvious to me.
yzshen@chromium.org changed reviewers: + rsleevi@chromium.org
Thanks Wan-Teh! Hi, Ryan. IIRC, you suggested the "best effort" note for SetSendBufferSize/SetReceiveBufferSize when we were working on the Pepper API. According to Wan-Teh, it seems we can delete the note now that we have handled this issue for Windows. What do you think? Thanks! https://codereview.chromium.org/612403003/diff/60001/mojo/services/public/int... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/612403003/diff/60001/mojo/services/public/int... mojo/services/public/interfaces/network/udp_socket.mojom:36: // doesn't guarantee it will conform to the size. On 2014/10/02 22:14:05, wtc wrote: > > I believe this note can be deleted. If this note is referring to the problem on > Windows, net::UDPSocketWin::SetSendBufferSize already detects this problem and > will return ERR_SOCKET_SEND_BUFFER_SIZE_UNCHANGEABLE. > > If some other platform has a similar problem, then we can keep this note but > please try to find out what that platform is and add a reference to a detailed > description of the problem. Done. https://codereview.chromium.org/612403003/diff/60001/mojo/services/public/int... mojo/services/public/interfaces/network/udp_socket.mojom:60: // Notifies that the client is ready to accept |number| of packets. On 2014/10/02 22:14:05, wtc wrote: > > Nit: it would be good to explain why it is useful to pass a |number| > 1. The > reason was not obvious to me. Done.
Friendly ping, reviewers that haven't LG-ed this CL. Would you please take another look or let me know if you don't think I should wait for your LG? Thanks!
It's been a while since reviewing the Pepper bits. I'm mostly apathetic, so LGTM :) I don't know if this matters in the new Mojo world, but this doesn't let you specify egress interfaces. For ChromeOS and VPNs, that has been a discussion of "nice to have", so it's something you may wish to consider in future iterations of this API (in coordination with them). I believe Wan-Teh noted this in terms of the absence of some of the IPv6 fields. https://codereview.chromium.org/612403003/diff/80001/mojo/services/public/int... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/612403003/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/udp_socket.mojom:45: // buffer and send out). This method negotiates how many requests (not bytes) s/send out/sent out/ https://codereview.chromium.org/612403003/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/udp_socket.mojom:47: // requests directly with error code ERR_INSUFFICIENT_RESOURCES and discard s/discard/discards/ https://codereview.chromium.org/612403003/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/udp_socket.mojom:48: // those packets. If the client wants to avoid such failure, it needs to keep s/failure/failures/ https://codereview.chromium.org/612403003/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/udp_socket.mojom:94: // - ERR_INSUFFICIENT_RESOURCES (-12): The service doesn't have sufficient 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.
Thanks Ryan! Out of curiosity, WRT the absence of IPv6 fields, I noticed that the IPEndPoint class in /net only has |address_| and |port_|. Does that mean Chrome currently doesn't support those fields? Or is IPEndPoint not the right class to look at? https://codereview.chromium.org/612403003/diff/80001/mojo/services/public/int... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/612403003/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/udp_socket.mojom:45: // buffer and send out). This method negotiates how many requests (not bytes) On 2014/10/05 23:24:05, Ryan Sleevi wrote: > s/send out/sent out/ Thanks! https://codereview.chromium.org/612403003/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/udp_socket.mojom:47: // requests directly with error code ERR_INSUFFICIENT_RESOURCES and discard On 2014/10/05 23:24:05, Ryan Sleevi wrote: > s/discard/discards/ Done. https://codereview.chromium.org/612403003/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/udp_socket.mojom:48: // those packets. If the client wants to avoid such failure, it needs to keep On 2014/10/05 23:24:06, Ryan Sleevi wrote: > s/failure/failures/ Thanks! (Sorry, I should have been more careful.) https://codereview.chromium.org/612403003/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/udp_socket.mojom:94: // - ERR_INSUFFICIENT_RESOURCES (-12): The service doesn't have sufficient On 2014/10/05 23:24:06, Ryan Sleevi wrote: > 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. 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!
On 2014/10/06 05:17:01, yzshen1 wrote: > Thanks Ryan! > > Out of curiosity, WRT the absence of IPv6 fields, I noticed that the IPEndPoint > class in /net only has |address_| and |port_|. Does that mean Chrome currently > doesn't support those fields? Or is IPEndPoint not the right class to look at? > We haven't exposed these yet. As //net provides a conceptually high level API, and because the URI syntax for scope identifiers is a fragmented wasteland, as haven't yet exposed it. We do have bugs open for URI support, and WebRTC and Enterprise have both been having preliminary discussions. I'm not arguing for making the change in this CL, but I am making sure you're aware of the discussions and use cases, since I imagine Mojo consumers may have a need sooner than non-Mojo. > https://codereview.chromium.org/612403003/diff/80001/mojo/services/public/int... > File mojo/services/public/interfaces/network/udp_socket.mojom (right): > > https://codereview.chromium.org/612403003/diff/80001/mojo/services/public/int... > mojo/services/public/interfaces/network/udp_socket.mojom:45: // buffer and send > out). This method negotiates how many requests (not bytes) > On 2014/10/05 23:24:05, Ryan Sleevi wrote: > > s/send out/sent out/ > > Thanks! > > https://codereview.chromium.org/612403003/diff/80001/mojo/services/public/int... > mojo/services/public/interfaces/network/udp_socket.mojom:47: // requests > directly with error code ERR_INSUFFICIENT_RESOURCES and discard > On 2014/10/05 23:24:05, Ryan Sleevi wrote: > > s/discard/discards/ > > Done. > > https://codereview.chromium.org/612403003/diff/80001/mojo/services/public/int... > mojo/services/public/interfaces/network/udp_socket.mojom:48: // those packets. > If the client wants to avoid such failure, it needs to keep > On 2014/10/05 23:24:06, Ryan Sleevi wrote: > > s/failure/failures/ > > Thanks! (Sorry, I should have been more careful.) > > https://codereview.chromium.org/612403003/diff/80001/mojo/services/public/int... > mojo/services/public/interfaces/network/udp_socket.mojom:94: // - > ERR_INSUFFICIENT_RESOURCES (-12): The service doesn't have sufficient > On 2014/10/05 23:24:06, Ryan Sleevi wrote: > > 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. > > 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!
On Sun, Oct 5, 2014 at 10:23 PM, <rsleevi@chromium.org> wrote: > On 2014/10/06 05:17:01, yzshen1 wrote: > >> Thanks Ryan! >> > > Out of curiosity, WRT the absence of IPv6 fields, I noticed that the >> > IPEndPoint > >> class in /net only has |address_| and |port_|. Does that mean Chrome >> currently >> doesn't support those fields? Or is IPEndPoint not the right class to >> look at? >> > > > We haven't exposed these yet. As //net provides a conceptually high level > API, > and because the URI syntax for scope identifiers is a fragmented > wasteland, as > haven't yet exposed it. > > We do have bugs open for URI support, and WebRTC and Enterprise have both > been > having preliminary discussions. I'm not arguing for making the change in > this > CL, but I am making sure you're aware of the discussions and use cases, > since I > imagine Mojo consumers may have a need sooner than non-Mojo. > > Thank you for the detailed explanation! I will keep that in mind. > > > > https://codereview.chromium.org/612403003/diff/80001/mojo/ > services/public/interfaces/network/udp_socket.mojom > >> File mojo/services/public/interfaces/network/udp_socket.mojom (right): >> > > > https://codereview.chromium.org/612403003/diff/80001/mojo/ > services/public/interfaces/network/udp_socket.mojom#newcode45 > >> mojo/services/public/interfaces/network/udp_socket.mojom:45: // buffer >> and >> > send > >> out). This method negotiates how many requests (not bytes) >> On 2014/10/05 23:24:05, Ryan Sleevi wrote: >> > s/send out/sent out/ >> > > Thanks! >> > > > https://codereview.chromium.org/612403003/diff/80001/mojo/ > services/public/interfaces/network/udp_socket.mojom#newcode47 > >> mojo/services/public/interfaces/network/udp_socket.mojom:47: // requests >> directly with error code ERR_INSUFFICIENT_RESOURCES and discard >> On 2014/10/05 23:24:05, Ryan Sleevi wrote: >> > s/discard/discards/ >> > > Done. >> > > > https://codereview.chromium.org/612403003/diff/80001/mojo/ > services/public/interfaces/network/udp_socket.mojom#newcode48 > >> mojo/services/public/interfaces/network/udp_socket.mojom:48: // those >> packets. >> If the client wants to avoid such failure, it needs to keep >> On 2014/10/05 23:24:06, Ryan Sleevi wrote: >> > s/failure/failures/ >> > > Thanks! (Sorry, I should have been more careful.) >> > > > https://codereview.chromium.org/612403003/diff/80001/mojo/ > services/public/interfaces/network/udp_socket.mojom#newcode94 > >> mojo/services/public/interfaces/network/udp_socket.mojom:94: // - >> ERR_INSUFFICIENT_RESOURCES (-12): The service doesn't have sufficient >> On 2014/10/05 23:24:06, Ryan Sleevi wrote: >> > 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. >> > > 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! >> > > > > https://codereview.chromium.org/612403003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patch set 6 LGTM. https://codereview.chromium.org/612403003/diff/100001/mojo/services/public/in... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/612403003/diff/100001/mojo/services/public/in... mojo/services/public/interfaces/network/udp_socket.mojom:102: // On success, |src_addr| and |data| are non-NULL, |result.code| is a Nit: add "and" before "|result.code|". https://codereview.chromium.org/612403003/diff/100001/mojo/services/public/in... mojo/services/public/interfaces/network/udp_socket.mojom:103: // non-negative number indicating how many bytes have been received. Nit: document what happens on failure (|result.code| is a network error code).
yzshen: None of my comments are blocking, so you should feel free to commit anytime. If you just have some place where I can stream comments asynchronously in a non-blocking fashion, that'd be great. https://codereview.chromium.org/612403003/diff/100001/mojo/services/public/in... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/612403003/diff/100001/mojo/services/public/in... mojo/services/public/interfaces/network/udp_socket.mojom:52: => (uint32 actual_size); 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. https://codereview.chromium.org/612403003/diff/100001/mojo/services/public/in... mojo/services/public/interfaces/network/udp_socket.mojom:88: ReceiveMorePackets(uint32 number); Sorry to be pedantic, but you should probably use say message or datagram. It's not necessary an IP packet, because of fragmentation. https://codereview.chromium.org/612403003/diff/100001/mojo/services/public/in... mojo/services/public/interfaces/network/udp_socket.mojom:98: SendTo(NetAddress dest_addr, uint8[] data) => (NetworkError result); 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?
Seems fine, high-level LGTM.
> yzshen: None of my comments are blocking, so you should > feel free to commit > anytime. If you just have some place where I can stream > comments asynchronously > in a non-blocking fashion, that'd be great. You could shoot me a mail and I will start a CL for our discussion in that case? What do you think? Thanks a lot! https://codereview.chromium.org/612403003/diff/100001/mojo/services/public/in... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/612403003/diff/100001/mojo/services/public/in... mojo/services/public/interfaces/network/udp_socket.mojom:52: => (uint32 actual_size); On 2014/10/06 20:37:54, willchan OOO until 03-22-15 wrote: > 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. 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. https://codereview.chromium.org/612403003/diff/100001/mojo/services/public/in... mojo/services/public/interfaces/network/udp_socket.mojom:88: ReceiveMorePackets(uint32 number); On 2014/10/06 20:37:54, willchan OOO until 03-22-15 wrote: > Sorry to be pedantic, but you should probably use say message or datagram. It's > not necessary an IP packet, because of fragmentation. > Sounds good, thanks! https://codereview.chromium.org/612403003/diff/100001/mojo/services/public/in... mojo/services/public/interfaces/network/udp_socket.mojom:98: SendTo(NetAddress dest_addr, uint8[] data) => (NetworkError result); On 2014/10/06 20:37:54, willchan OOO until 03-22-15 wrote: > 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? 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! https://codereview.chromium.org/612403003/diff/100001/mojo/services/public/in... mojo/services/public/interfaces/network/udp_socket.mojom:102: // On success, |src_addr| and |data| are non-NULL, |result.code| is a On 2014/10/06 17:41:12, wtc wrote: > > Nit: add "and" before "|result.code|". Done. https://codereview.chromium.org/612403003/diff/100001/mojo/services/public/in... mojo/services/public/interfaces/network/udp_socket.mojom:103: // non-negative number indicating how many bytes have been received. On 2014/10/06 17:41:12, wtc wrote: > > Nit: document what happens on failure (|result.code| is a network error code). Done.
The CQ bit was checked by yzshen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/612403003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as 8ea26d21098870292e81ea674d6919f1759708c6
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/2087fb8402cbd9512aab3936661d15c80b62b543 Cr-Commit-Position: refs/heads/master@{#298606} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
