Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
613683006:
Add TCP socket mojo interfaces (Closed)
|
Created:
6 years, 2 months ago by brettw Modified:
6 years, 2 months ago Reviewers:
willchan no longer on Chromium, Ryan Sleevi, wtc, viettrungluu, yzshen1, darin (slow to review) 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. |
DescriptionAdd TCP socket mojo interfaces
BUG=
Committed: https://crrev.com/c103dc5243dba96e245bf7e0559f7c4d3b3ab574
Cr-Commit-Position: refs/heads/master@{#297907}
Patch Set 1 #
Total comments: 6
Patch Set 2 : BSD like #
Total comments: 9
Patch Set 3 : listen #Patch Set 4 : comments #
Total comments: 4
Patch Set 5 : only local bound addr #
Total comments: 19
Patch Set 6 : #
Total comments: 1
Patch Set 7 : Remove all options #Patch Set 8 : split it apart #
Total comments: 3
Patch Set 9 : Added todo for Darin's suggestion. #Patch Set 10 : Implement network service functions #
Total comments: 12
Messages
Total messages: 59 (8 generated)
brettw@chromium.org changed reviewers: + viettrungluu@chromium.org, yzshen@chromium.org
Initial pass for discussion. Having DataPipe be separate makes this surprisingly simple, so +1 for that. Things to consider: Could be more like Pepper/unix sockets and have everything on one socket. I didn't like having to require Bind then Connect for the client socket case. It's OK for a sync API, but requires a lot more work for an async one since there's another state in the state machine. I kept the Bind/Listen split in the server API because it seemed like it's nice to have a separate step tell you if the port is in use. We can consider adding this step to the first Listen call to eliminate this state machine step. I think this is what this API does: https://developer.chrome.com/apps/sockets_tcpServer One async call to async listen is much easier than two, and it's not clear you lose much. Thoughts? I guess we need a Close() on the server API to stop accepting connections. I'll add it later. There was only one option in the Pepper API not already covered by DataPipe's buffering, which was the write coalescing. I added a separate function to set this. This could be an enum like Pepper, but I think this way is more clear, especially if Mojo can handle API versioning cleaner than Pepper did. I added the local and remote address to the callbacks. This is annoying, but otherwise we'll need an async API to get them, which is super annoying. Alternatively, we could have a struct of "Properties" that get passed back (not sure if we'll need other state).
https://codereview.chromium.org/613683006/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/tcp_socket.mojom:19: // implicitly closes when the TCPSocket has been deleted. "deleted"? https://codereview.chromium.org/613683006/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/tcp_socket.mojom:20: Close(); Is this really necessary at all? In general, this interface seems kind of broken. It conflates two things: the ability to make connections and the connection itself. (E.g., what happens if you call Connect() twice?) https://codereview.chromium.org/613683006/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/tcp_socket_server.mojom (right): https://codereview.chromium.org/613683006/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/tcp_socket_server.mojom:20: NetAddress local_address, Why does this need a local address?
Btw, rsleevi think we should ask the net-dev@ whether the approach of providing a bunch of low-level socket APIs is the right direction. (I will send such a mail shortly and CC you.) https://codereview.chromium.org/613683006/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/tcp_socket.mojom:10: interface TCPSocket { Do we need a Bind() function here? https://codereview.chromium.org/613683006/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/tcp_socket.mojom:15: NetAddress local_address, you might want to use NetAddress? here, so that it is okay to return NULL on failure. (? means nullable, nullable/non-nullable is enforced during message validation.) https://codereview.chromium.org/613683006/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/tcp_socket_server.mojom (right): https://codereview.chromium.org/613683006/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/tcp_socket_server.mojom:11: [Client=TCPSocketServerClient] Do you need this?
Darin's recommendation is to go with more of a BSD-like approach (this one is incompatible). I'll rework this and we'll see what it looks like.
PTAL. The new patch is more like BSD sockets, where a TCPSocket is the same as a sockfd that confusingly conflates both client and server sockets. I moved the reported addresses into a structure which makes the callbacks more clear. This ends up being almost exactly like the Pepper APIs with no local state.
https://codereview.chromium.org/613683006/diff/20001/mojo/services/public/int... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/20001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:26: // 1. Listen() to wait for an incoming connection. This is different from what Pepper does. Is it intended? In Pepper: Listen() starts listening for incoming connections. And it needs to be called only once. Accept() waits for an incoming connection until one is available. https://codereview.chromium.org/613683006/diff/20001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:35: Bind(NetAddress addr) => (NetworkError result); For server socket, it seems we don't have a way to get its resulting bound address, right? https://codereview.chromium.org/613683006/diff/20001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:61: (NetworkError result, TCPSocket socket, TCPSocketInfo info); You might want to append '?' to the type TCPSocket and TCPSocketInfo. So that the service can return NULL (on failure). https://codereview.chromium.org/613683006/diff/20001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:70: // Closes the connection. If Close() is not called, the connection will be Maybe we could revise the comment to also cover the server socket case.
What do you think about adding a ListenOn() function that binds and starts listening? This saves having to write the intermediate state machine and is almost always what you want. But we probably want to keep the separate Bind() function in case people are porting existing socket code. Or do you think this is superfluous? https://codereview.chromium.org/613683006/diff/20001/mojo/services/public/int... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/20001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:26: // 1. Listen() to wait for an incoming connection. You're right, I misunderstood what Pepper/BSD did. Fixed. https://codereview.chromium.org/613683006/diff/20001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:35: Bind(NetAddress addr) => (NetworkError result); Added info. https://codereview.chromium.org/613683006/diff/20001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:61: (NetworkError result, TCPSocket socket, TCPSocketInfo info); Good idea, I added this everywhere. https://codereview.chromium.org/613683006/diff/20001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:70: // Closes the connection. If Close() is not called, the connection will be Done
https://codereview.chromium.org/613683006/diff/20001/mojo/services/public/int... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/20001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:15: // Represents a TCP socket. A socket can represent either a connection to a Why is it important to combine these two cases into one interface?
On 2014/09/30 19:22:15, viettrungluu wrote: > https://codereview.chromium.org/613683006/diff/20001/mojo/services/public/int... > File mojo/services/public/interfaces/network/tcp_socket.mojom (right): > > https://codereview.chromium.org/613683006/diff/20001/mojo/services/public/int... > mojo/services/public/interfaces/network/tcp_socket.mojom:15: // Represents a TCP > socket. A socket can represent either a connection to a > Why is it important to combine these two cases into one interface? That's the whole thing with BSD compatibility. You don't know when creating a socket or binding it whether it will be used in server or client mode. With this interface, there is a 1:1 correspondence between a sockfd and a TCPSocket mojo object.
And maybe we should consider adding some networking experts to review the API. :) Thanks! https://codereview.chromium.org/613683006/diff/60001/mojo/services/public/int... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/60001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:35: Bind(NetAddress addr) => (NetworkError result, TCPSocketInfo? info); Because there is no remote address at this point yet, maybe it makes sense to simply remove TCPSocketInfo, and use local_addr and remote_addr as needed?
https://codereview.chromium.org/613683006/diff/60001/mojo/services/public/int... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/60001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:35: Bind(NetAddress addr) => (NetworkError result, TCPSocketInfo? info); I changed this case to be a NetAddr? but kept the struct for the other cases. Having the struct is much easier for the two addresses since it is easy to get the arguments confused.
brettw@chromium.org changed reviewers: + rsleevi@chromium.org
+Sleevi if he cares.
https://codereview.chromium.org/613683006/diff/60001/mojo/services/public/int... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/60001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:74: SetCoaleseWrites(bool coalesce); Having a setter but no getter seems bad. Also, not being able to set multiple options (if/when we add more) at once seems bad. Maybe this should be: SetOptions(TCPSocketOptions options) => (NetworkError result); where TCPSocketOptions looks like: struct TCPSocketOptions { bool? coalesce_writes; }; (There should also be a |GetOptions() => (TCPSocketOptions? options);| or something like that.)
Thanks. I looped this into a thread on net-dev@ where we were discussing yzshen's UDP API https://groups.google.com/a/chromium.org/d/msg/net-dev/vxejIkO_jgA/OQX2dUpRq5wJ It would be great if you could get feedback on these before landing, or at least remain highly flexible towards change. If there's timing sensitivity, it would also be great to chime in.
It's fine to wait a few days for this.
https://codereview.chromium.org/613683006/diff/60001/mojo/services/public/int... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/60001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:74: SetCoaleseWrites(bool coalesce); My plan was to use the mojo versioning to add functions for different options. There are three options in Pepper and the other ones control buffering, so I wasn't particularly concerned that we'd add more. If we do add more, an enum corresponding to the BSD socket API would be better, since the struct seems like it will make it harder to add options in a backwards-compatible way. I feel like this API will be used more often at a lower-level than the Pepper one, so maybe some of the conjestion control options would make sense?
https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:35: // success, built_to will indicate the local address that was bound. typo: built_to -> bound_to https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:44: Listen(int32 backlog) => (NetworkError result); 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. https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:57: (NetworkError result, TCPSocket? socket, TCPSocketInfo? info); 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) https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:70: Close(); 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. https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:75: SetCoaleseWrites(bool coalesce); 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.
https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:44: Listen(int32 backlog) => (NetworkError result); On 2014/09/30 20:48:08, Ryan Sleevi wrote: > 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. 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. https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:57: (NetworkError result, TCPSocket? socket, TCPSocketInfo? info); On 2014/09/30 20:48:07, Ryan Sleevi wrote: > What happens if there are no sockets pending acceptance? Does this go into an > eternal pending acceptance state with a dangling callback? Yes. > 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) No. Can you point me to this? https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:70: Close(); 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. https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:75: SetCoaleseWrites(bool coalesce); On 2014/09/30 20:48:07, Ryan Sleevi wrote: > Why not call this what it is (and which most socket APIs reflect it is), namely, > Nagle's algorithm? I mentioned this in the comment. > 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. We're not "using" anything at this level, this flag just gets passed to the net/OS layers.
https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:44: Listen(int32 backlog) => (NetworkError result); On 2014/09/30 21:06:17, brettw wrote: > On 2014/09/30 20:48:08, Ryan Sleevi wrote: > > 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. > > 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. 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.
Regarding dynamic adjustment of pending sockets: Turns out I was misremembering the details of Windows' implementation. Windows can/will automatically adjust/increase the socket backlog for you (depending on registry flags). Linux, as far as I recall, will just cap you to the max of the max conns (configured by the server config, NOT the hardcoded symbol SOMAXCON) https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:43: // happen when connections are incoming faster than Accept() can be called. 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). https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:44: Listen(int32 backlog) => (NetworkError result); > 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. 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? https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:70: Close(); On 2014/09/30 21:06:16, brettw wrote: > 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? 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. > > 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. 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. https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:75: SetCoaleseWrites(bool coalesce); On 2014/09/30 21:06:17, brettw wrote: > On 2014/09/30 20:48:07, Ryan Sleevi wrote: > > Why not call this what it is (and which most socket APIs reflect it is), > namely, > > Nagle's algorithm? > > I mentioned this in the comment. Did you mean the existing comment, or an update? Just because there's different strategies for coalescing. > > > 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. > > We're not "using" anything at this level, this flag just gets passed to the > net/OS layers. 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)
https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:43: // happen when connections are incoming faster than Accept() can be called. I was just copying the BSD Listen() function. I'm not sure how you want me to change this. https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:75: SetCoaleseWrites(bool coalesce); > 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) 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.
On 2014/09/30 21:25:48, viettrungluu wrote: > https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... > File mojo/services/public/interfaces/network/tcp_socket.mojom (right): > > https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... > mojo/services/public/interfaces/network/tcp_socket.mojom:44: Listen(int32 > backlog) => (NetworkError result); > On 2014/09/30 21:06:17, brettw wrote: > > On 2014/09/30 20:48:08, Ryan Sleevi wrote: > > > 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. > > > > 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. > > 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. Concrete suggestions welcome.
willchan@chromium.org changed reviewers: + willchan@chromium.org
https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:70: Close(); On 2014/09/30 20:48:08, Ryan Sleevi wrote: > 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. 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?
https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:44: Listen(int32 backlog) => (NetworkError result); 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.
https://codereview.chromium.org/613683006/diff/100001/mojo/services/public/in... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/100001/mojo/services/public/in... mojo/services/public/interfaces/network/tcp_socket.mojom:74: SetCoaleseWrites(bool coalesce); typo: Coalesce That said, in further thinking about this API, I think it's somewhat weird when compared to your data_pipe API. That is, Nagling is necessary when you're writing discrete chunks, but this doesn't, you have a direct stream API. As such, the semantics of what it means to coalesce writes is weird, and I have trouble predicting what will happen one way or the other. That is, I don't expect a write to the data pipe to IMMEDIATELY trigger a TCP write, just by virtue of IPC / mojo semantics. Instead, what I'd (naively) expect to happen is that internally the stream is going to buffer until an event loop begins processing messages and then send. Maybe I'm over-thinking, as I can see lots of ways of implementing the data pipes, each with different semantics. For example, you might IPC a message over each time someone writes to the send_stream (like our current Pepper sockets). Or you might write into a shared memory buffer, appending data to the pending list (like the kernel's underlying socket implementation). In both of these cases, saying nagling at the transport layer ignores the latencies involved in the IPC layer that nagling is meant to address. I realize right now I'm relying on the interface-as-the-documentation, and I guess I don't know what to predict what will happen.
https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... File mojo/services/public/interfaces/network/tcp_socket.mojom (right): https://codereview.chromium.org/613683006/diff/80001/mojo/services/public/int... mojo/services/public/interfaces/network/tcp_socket.mojom:44: Listen(int32 backlog) => (NetworkError result); On 2014/09/30 22:16:17, brettw wrote: > 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. 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"
I would _love_ it if you guys wanted to design the best possible API and I would encourage you to get involved if you find it interesting. From my perspective, I think you're overthinking the goal of this current API. I want to write a demo app that shows communication on the internet, or maybe a local server for some files for testing purposes. The goal is not to design the best possible network API that can function as a high-performance server, handle TLS, SPDY, etc. This is not the final API. Enhancements and rewrites will be done. There is no binary stability. Nothing critical will use this API. I'll delete the backlog parameter and leave the pending connection undefined for now. I'll also delete the coalescing function and just use the default since it seems like there are still some open questions and I don't actually need it now anyway.
Thanks Brett for the useful context. All, it's usually best to assume that if you're not included on a CL as a reviewer that that was an oversight rather than a deliberate action. The code review system allows anyone to add themselves as a reviewer for anything because speaking up is part of our culture. So is writing straw man CLs like this and sending them out. Can we embrace both? Sometimes those CLs even land. Code isn't immutable. We're nowhere near even contemplating freezing this stuff. I want to make sure we continue to foster an environment where people are motivated to propose new things. This is not to gloss over the real concerns about building a production quality API. First iterations are seldom that API. Thanks. -Ben On Tue, Sep 30, 2014 at 9:31 PM, <brettw@chromium.org> wrote: > I would _love_ it if you guys wanted to design the best possible API and I > would > encourage you to get involved if you find it interesting. > > From my perspective, I think you're overthinking the goal of this current > API. I > want to write a demo app that shows communication on the internet, or > maybe a > local server for some files for testing purposes. The goal is not to > design the > best possible network API that can function as a high-performance server, > handle > TLS, SPDY, etc. > > This is not the final API. Enhancements and rewrites will be done. There > is no > binary stability. Nothing critical will use this API. > > I'll delete the backlog parameter and leave the pending connection > undefined for > now. I'll also delete the coalescing function and just use the default > since it > seems like there are still some open questions and I don't actually need > it now > anyway. > > https://codereview.chromium.org/613683006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Sep 30, 2014 9:47 PM, "Ben Goodger" <beng@google.com> wrote: > > Thanks Brett for the useful context. > > All, it's usually best to assume that if you're not included on a CL as a reviewer that that was an oversight rather than a deliberate action. The code review system allows anyone to add themselves as a reviewer for anything because speaking up is part of our culture. So is writing straw man CLs like this and sending them out. Can we embrace both? Sometimes those CLs even land. Code isn't immutable. We're nowhere near even contemplating freezing this stuff. I want to make sure we continue to foster an environment where people are motivated to propose new things. This is not to gloss over the real concerns about building a production quality API. First iterations are seldom that API. > > Thanks. > > -Ben > Ben, I think we're particularly sensitive because the past few APIs haven't had that flexibility, nor have they had the engineering commitment to revisit past decisions, and thus have ossified into Serious Projects to Clean Up. Certainly, of all people in Chrome I trust to iterate and refactor, Brett, Darin, and Trung are at the top of that list. The challenge when CLs come by is that we often lack the context to know the tradeoffs, what discussions have been had, and what decisions have been made. So we approach it the same as we have other APIs - trying to gather that data, provide feedback, and ask 'If this were the final API, do we know what the tradeoffs are and are we happy with them'. We also want to encourage proactive outreach, mostly because we've also seen teams come back and refactor, and end up with things worse than starting. Just knowing what the initial goals for a minimum viable project make it tremendously easier to provide targeted and actionable feedback. I have no doubt everyone is working towards the same goal, and the more we in the net team can help mojo with that, the better for everyone :) > On Tue, Sep 30, 2014 at 9:31 PM, <brettw@chromium.org> wrote: >> >> I would _love_ it if you guys wanted to design the best possible API and I would >> encourage you to get involved if you find it interesting. >> >> From my perspective, I think you're overthinking the goal of this current API. I >> want to write a demo app that shows communication on the internet, or maybe a >> local server for some files for testing purposes. The goal is not to design the >> best possible network API that can function as a high-performance server, handle >> TLS, SPDY, etc. >> >> This is not the final API. Enhancements and rewrites will be done. There is no >> binary stability. Nothing critical will use this API. >> >> I'll delete the backlog parameter and leave the pending connection undefined for >> now. I'll also delete the coalescing function and just use the default since it >> seems like there are still some open questions and I don't actually need it now >> anyway. >> >> https://codereview.chromium.org/613683006/ > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Tue, Sep 30, 2014 at 10:15 PM, Ryan Sleevi <rsleevi@chromium.org> wrote: > > On Sep 30, 2014 9:47 PM, "Ben Goodger" <beng@google.com> wrote: >> >> Thanks Brett for the useful context. >> >> All, it's usually best to assume that if you're not included on a CL as a >> reviewer that that was an oversight rather than a deliberate action. The >> code review system allows anyone to add themselves as a reviewer for >> anything because speaking up is part of our culture. So is writing straw man >> CLs like this and sending them out. Can we embrace both? Sometimes those CLs >> even land. Code isn't immutable. We're nowhere near even contemplating >> freezing this stuff. I want to make sure we continue to foster an >> environment where people are motivated to propose new things. This is not to >> gloss over the real concerns about building a production quality API. First >> iterations are seldom that API. >> >> Thanks. >> >> -Ben >> > > Ben, > > I think we're particularly sensitive because the past few APIs haven't had > that flexibility, nor have they had the engineering commitment to revisit > past decisions, and thus have ossified into Serious Projects to Clean Up. > > Certainly, of all people in Chrome I trust to iterate and refactor, Brett, > Darin, and Trung are at the top of that list. > > The challenge when CLs come by is that we often lack the context to know the > tradeoffs, what discussions have been had, and what decisions have been > made. So we approach it the same as we have other APIs - trying to gather > that data, provide feedback, and ask 'If this were the final API, do we know > what the tradeoffs are and are we happy with them'. > > We also want to encourage proactive outreach, mostly because we've also seen > teams come back and refactor, and end up with things worse than starting. > > Just knowing what the initial goals for a minimum viable project make it > tremendously easier to provide targeted and actionable feedback. > > I have no doubt everyone is working towards the same goal, and the more we > in the net team can help mojo with that, the better for everyone :) +1 to everything Ryan said. I just want to emphasize that I don't want the net team to be a blocker here. You're experimenting with a new API and iterating. That's great. But we'd like to be involved early on for all the reasons Ryan stated. And, although you suggest it might just be oversight, that's unfortunately what happens every other time, which is why I'm concerned about process failure. In Mojo's case, there are 7 .mojom files already committed without having discussed anything with the net team. I don't think y'all are deliberately not including us, but probably just think we don't need to be involved yet. But we want to be, because in all other cases we've been involved too late, and it's hurt us. > >> On Tue, Sep 30, 2014 at 9:31 PM, <brettw@chromium.org> wrote: >>> >>> I would _love_ it if you guys wanted to design the best possible API and >>> I would >>> encourage you to get involved if you find it interesting. >>> >>> From my perspective, I think you're overthinking the goal of this current >>> API. I >>> want to write a demo app that shows communication on the internet, or >>> maybe a >>> local server for some files for testing purposes. The goal is not to >>> design the >>> best possible network API that can function as a high-performance server, >>> handle >>> TLS, SPDY, etc. >>> >>> This is not the final API. Enhancements and rewrites will be done. There >>> is no >>> binary stability. Nothing critical will use this API. >>> >>> I'll delete the backlog parameter and leave the pending connection >>> undefined for >>> now. I'll also delete the coalescing function and just use the default >>> since it >>> seems like there are still some open questions and I don't actually need >>> it now >>> anyway. >>> >>> https://codereview.chromium.org/613683006/ >> >> To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM (Just mean that I don't think I have the knowledge to provide any further suggestions at this point.) I personally think that it is probably hard to make sure the first draft have everything covered. As long as we agree that there are problems to solve, and keep working together to solve them, we will have a great API at the end. After all, we share the same goal and all work very hard towards it. Good night. :)
On Tue, Sep 30, 2014 at 10:52 PM, William Chan (ιζΊζ) <willchan@chromium.org> wrote: > On Tue, Sep 30, 2014 at 10:15 PM, Ryan Sleevi <rsleevi@chromium.org> > wrote: > > > > On Sep 30, 2014 9:47 PM, "Ben Goodger" <beng@google.com> wrote: > >> > >> Thanks Brett for the useful context. > >> > >> All, it's usually best to assume that if you're not included on a CL as > a > >> reviewer that that was an oversight rather than a deliberate action. The > >> code review system allows anyone to add themselves as a reviewer for > >> anything because speaking up is part of our culture. So is writing > straw man > >> CLs like this and sending them out. Can we embrace both? Sometimes > those CLs > >> even land. Code isn't immutable. We're nowhere near even contemplating > >> freezing this stuff. I want to make sure we continue to foster an > >> environment where people are motivated to propose new things. This is > not to > >> gloss over the real concerns about building a production quality API. > First > >> iterations are seldom that API. > >> > >> Thanks. > >> > >> -Ben > >> > > > > Ben, > > > > I think we're particularly sensitive because the past few APIs haven't > had > > that flexibility, nor have they had the engineering commitment to revisit > > past decisions, and thus have ossified into Serious Projects to Clean Up. > > > > Certainly, of all people in Chrome I trust to iterate and refactor, > Brett, > > Darin, and Trung are at the top of that list. > > > > The challenge when CLs come by is that we often lack the context to know > the > > tradeoffs, what discussions have been had, and what decisions have been > > made. So we approach it the same as we have other APIs - trying to gather > > that data, provide feedback, and ask 'If this were the final API, do we > know > > what the tradeoffs are and are we happy with them'. > > > > We also want to encourage proactive outreach, mostly because we've also > seen > > teams come back and refactor, and end up with things worse than starting. > > > > Just knowing what the initial goals for a minimum viable project make it > > tremendously easier to provide targeted and actionable feedback. > > > > I have no doubt everyone is working towards the same goal, and the more > we > > in the net team can help mojo with that, the better for everyone :) > > +1 to everything Ryan said. > > I just want to emphasize that I don't want the net team to be a > blocker here. You're experimenting with a new API and iterating. > That's great. But we'd like to be involved early on for all the > reasons Ryan stated. And, although you suggest it might just be > oversight, that's unfortunately what happens every other time, which > is why I'm concerned about process failure. In Mojo's case, there are > 7 .mojom files already committed without having discussed anything > with the net team. I don't think y'all are deliberately not including > us, but probably just think we don't need to be involved yet. But we > want to be, because in all other cases we've been involved too late, > and it's hurt us. > Will, I don't see a process failure here. I don't see a problem with committing experimental code, having to do with networking, that hasn't yet been reviewed by the folks most knowledgeable about networking. Surely there is room in our project for experimentation. That's all that is going on here. I think I also made this clear to you and Ryan about a month ago, so I'm surprised that you are surprised. At that time I pointed out that we had some crude network service code already, and I encouraged you guys to get involved with it. Remember our discussion in Ryan's office? I think it is great that you are both interested in making things better here, but let's not let perfection be the enemy of progress. Folks are experimenting. We'd like to use the source code repository as a way to share our experiments. -Darin > > > >> On Tue, Sep 30, 2014 at 9:31 PM, <brettw@chromium.org> wrote: > >>> > >>> I would _love_ it if you guys wanted to design the best possible API > and > >>> I would > >>> encourage you to get involved if you find it interesting. > >>> > >>> From my perspective, I think you're overthinking the goal of this > current > >>> API. I > >>> want to write a demo app that shows communication on the internet, or > >>> maybe a > >>> local server for some files for testing purposes. The goal is not to > >>> design the > >>> best possible network API that can function as a high-performance > server, > >>> handle > >>> TLS, SPDY, etc. > >>> > >>> This is not the final API. Enhancements and rewrites will be done. > There > >>> is no > >>> binary stability. Nothing critical will use this API. > >>> > >>> I'll delete the backlog parameter and leave the pending connection > >>> undefined for > >>> now. I'll also delete the coalescing function and just use the default > >>> since it > >>> seems like there are still some open questions and I don't actually > need > >>> it now > >>> anyway. > >>> > >>> https://codereview.chromium.org/613683006/ > >> > >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I think we are just surprised that it went from a discussion in a room to committed code. +1 to experiments +1 to not blocking on us +1 to moving quickly and breaking things But it would be great just to cc net-dev@, say 'hey, we're experimenting, follow along here', and move on. Even better if there's context and design docs to save review cycles. Or just context and design docs :) Either way, exciting to make progress on this, sorry we can't own it ourselves, and in future CLS or experiments, please feel absolutely free to shoot a note to the list for those to follow along and chime in. And DEFINITELY before anything goes to become part of external consumers / decisions we have to support for 6 mos-year for 'reasons' On Sep 30, 2014 11:10 PM, "Darin Fisher" <darin@chromium.org> wrote: > > > On Tue, Sep 30, 2014 at 10:52 PM, William Chan (ιζΊζ) < > willchan@chromium.org> wrote: > >> On Tue, Sep 30, 2014 at 10:15 PM, Ryan Sleevi <rsleevi@chromium.org> >> wrote: >> > >> > On Sep 30, 2014 9:47 PM, "Ben Goodger" <beng@google.com> wrote: >> >> >> >> Thanks Brett for the useful context. >> >> >> >> All, it's usually best to assume that if you're not included on a CL >> as a >> >> reviewer that that was an oversight rather than a deliberate action. >> The >> >> code review system allows anyone to add themselves as a reviewer for >> >> anything because speaking up is part of our culture. So is writing >> straw man >> >> CLs like this and sending them out. Can we embrace both? Sometimes >> those CLs >> >> even land. Code isn't immutable. We're nowhere near even contemplating >> >> freezing this stuff. I want to make sure we continue to foster an >> >> environment where people are motivated to propose new things. This is >> not to >> >> gloss over the real concerns about building a production quality API. >> First >> >> iterations are seldom that API. >> >> >> >> Thanks. >> >> >> >> -Ben >> >> >> > >> > Ben, >> > >> > I think we're particularly sensitive because the past few APIs haven't >> had >> > that flexibility, nor have they had the engineering commitment to >> revisit >> > past decisions, and thus have ossified into Serious Projects to Clean >> Up. >> > >> > Certainly, of all people in Chrome I trust to iterate and refactor, >> Brett, >> > Darin, and Trung are at the top of that list. >> > >> > The challenge when CLs come by is that we often lack the context to >> know the >> > tradeoffs, what discussions have been had, and what decisions have been >> > made. So we approach it the same as we have other APIs - trying to >> gather >> > that data, provide feedback, and ask 'If this were the final API, do we >> know >> > what the tradeoffs are and are we happy with them'. >> > >> > We also want to encourage proactive outreach, mostly because we've also >> seen >> > teams come back and refactor, and end up with things worse than >> starting. >> > >> > Just knowing what the initial goals for a minimum viable project make it >> > tremendously easier to provide targeted and actionable feedback. >> > >> > I have no doubt everyone is working towards the same goal, and the more >> we >> > in the net team can help mojo with that, the better for everyone :) >> >> +1 to everything Ryan said. >> >> I just want to emphasize that I don't want the net team to be a >> blocker here. You're experimenting with a new API and iterating. >> That's great. But we'd like to be involved early on for all the >> reasons Ryan stated. And, although you suggest it might just be >> oversight, that's unfortunately what happens every other time, which >> is why I'm concerned about process failure. In Mojo's case, there are >> 7 .mojom files already committed without having discussed anything >> with the net team. I don't think y'all are deliberately not including >> us, but probably just think we don't need to be involved yet. But we >> want to be, because in all other cases we've been involved too late, >> and it's hurt us. >> > > Will, > > I don't see a process failure here. I don't see a problem with committing > experimental code, having to do with networking, that hasn't yet been > reviewed by the folks most knowledgeable about networking. Surely there is > room in our project for experimentation. That's all that is going on here. > I think I also made this clear to you and Ryan about a month ago, so I'm > surprised that you are surprised. At that time I pointed out that we had > some crude network service code already, and I encouraged you guys to get > involved with it. Remember our discussion in Ryan's office? I think it is > great that you are both interested in making things better here, but let's > not let perfection be the enemy of progress. Folks are experimenting. We'd > like to use the source code repository as a way to share our experiments. > > -Darin > > > >> > >> >> On Tue, Sep 30, 2014 at 9:31 PM, <brettw@chromium.org> wrote: >> >>> >> >>> I would _love_ it if you guys wanted to design the best possible API >> and >> >>> I would >> >>> encourage you to get involved if you find it interesting. >> >>> >> >>> From my perspective, I think you're overthinking the goal of this >> current >> >>> API. I >> >>> want to write a demo app that shows communication on the internet, or >> >>> maybe a >> >>> local server for some files for testing purposes. The goal is not to >> >>> design the >> >>> best possible network API that can function as a high-performance >> server, >> >>> handle >> >>> TLS, SPDY, etc. >> >>> >> >>> This is not the final API. Enhancements and rewrites will be done. >> There >> >>> is no >> >>> binary stability. Nothing critical will use this API. >> >>> >> >>> I'll delete the backlog parameter and leave the pending connection >> >>> undefined for >> >>> now. I'll also delete the coalescing function and just use the default >> >>> since it >> >>> seems like there are still some open questions and I don't actually >> need >> >>> it now >> >>> anyway. >> >>> >> >>> https://codereview.chromium.org/613683006/ >> >> >> >> >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Tue, Sep 30, 2014 at 11:21 PM, Ryan Sleevi <rsleevi@chromium.org> wrote: > I think we are just surprised that it went from a discussion in a room to > committed code. > I thought I made it clear that there was already committed code. Remember how I was referring to the crude network service we already have? :-) Yes, we are extending it just as I also mentioned we would do. > +1 to experiments > +1 to not blocking on us > +1 to moving quickly and breaking things > > But it would be great just to cc net-dev@, say 'hey, we're experimenting, > follow along here', and move on. > It sounds good, but it is easy to overlook in practice when we're just experimenting. I think it is a bit much to expect folks to know to broadcast their experiments. > Even better if there's context and design docs to save review cycles. Or > just context and design docs :) > > Either way, exciting to make progress on this, sorry we can't own it > ourselves, and in future CLS or experiments, please feel absolutely free to > shoot a note to the list for those to follow along and chime in. > > And DEFINITELY before anything goes to become part of external consumers / > decisions we have to support for 6 mos-year for 'reasons' > I'm super sensitive to saddling anyone with support burdens. Please know that. We aren't going to make such commitments in a vacuum. -Darin > On Sep 30, 2014 11:10 PM, "Darin Fisher" <darin@chromium.org> wrote: > >> >> >> On Tue, Sep 30, 2014 at 10:52 PM, William Chan (ιζΊζ) < >> willchan@chromium.org> wrote: >> >>> On Tue, Sep 30, 2014 at 10:15 PM, Ryan Sleevi <rsleevi@chromium.org> >>> wrote: >>> > >>> > On Sep 30, 2014 9:47 PM, "Ben Goodger" <beng@google.com> wrote: >>> >> >>> >> Thanks Brett for the useful context. >>> >> >>> >> All, it's usually best to assume that if you're not included on a CL >>> as a >>> >> reviewer that that was an oversight rather than a deliberate action. >>> The >>> >> code review system allows anyone to add themselves as a reviewer for >>> >> anything because speaking up is part of our culture. So is writing >>> straw man >>> >> CLs like this and sending them out. Can we embrace both? Sometimes >>> those CLs >>> >> even land. Code isn't immutable. We're nowhere near even contemplating >>> >> freezing this stuff. I want to make sure we continue to foster an >>> >> environment where people are motivated to propose new things. This is >>> not to >>> >> gloss over the real concerns about building a production quality API. >>> First >>> >> iterations are seldom that API. >>> >> >>> >> Thanks. >>> >> >>> >> -Ben >>> >> >>> > >>> > Ben, >>> > >>> > I think we're particularly sensitive because the past few APIs haven't >>> had >>> > that flexibility, nor have they had the engineering commitment to >>> revisit >>> > past decisions, and thus have ossified into Serious Projects to Clean >>> Up. >>> > >>> > Certainly, of all people in Chrome I trust to iterate and refactor, >>> Brett, >>> > Darin, and Trung are at the top of that list. >>> > >>> > The challenge when CLs come by is that we often lack the context to >>> know the >>> > tradeoffs, what discussions have been had, and what decisions have been >>> > made. So we approach it the same as we have other APIs - trying to >>> gather >>> > that data, provide feedback, and ask 'If this were the final API, do >>> we know >>> > what the tradeoffs are and are we happy with them'. >>> > >>> > We also want to encourage proactive outreach, mostly because we've >>> also seen >>> > teams come back and refactor, and end up with things worse than >>> starting. >>> > >>> > Just knowing what the initial goals for a minimum viable project make >>> it >>> > tremendously easier to provide targeted and actionable feedback. >>> > >>> > I have no doubt everyone is working towards the same goal, and the >>> more we >>> > in the net team can help mojo with that, the better for everyone :) >>> >>> +1 to everything Ryan said. >>> >>> I just want to emphasize that I don't want the net team to be a >>> blocker here. You're experimenting with a new API and iterating. >>> That's great. But we'd like to be involved early on for all the >>> reasons Ryan stated. And, although you suggest it might just be >>> oversight, that's unfortunately what happens every other time, which >>> is why I'm concerned about process failure. In Mojo's case, there are >>> 7 .mojom files already committed without having discussed anything >>> with the net team. I don't think y'all are deliberately not including >>> us, but probably just think we don't need to be involved yet. But we >>> want to be, because in all other cases we've been involved too late, >>> and it's hurt us. >>> >> >> Will, >> >> I don't see a process failure here. I don't see a problem with committing >> experimental code, having to do with networking, that hasn't yet been >> reviewed by the folks most knowledgeable about networking. Surely there is >> room in our project for experimentation. That's all that is going on here. >> I think I also made this clear to you and Ryan about a month ago, so I'm >> surprised that you are surprised. At that time I pointed out that we had >> some crude network service code already, and I encouraged you guys to get >> involved with it. Remember our discussion in Ryan's office? I think it is >> great that you are both interested in making things better here, but let's >> not let perfection be the enemy of progress. Folks are experimenting. We'd >> like to use the source code repository as a way to share our experiments. >> >> -Darin >> >> >> >>> > >>> >> On Tue, Sep 30, 2014 at 9:31 PM, <brettw@chromium.org> wrote: >>> >>> >>> >>> I would _love_ it if you guys wanted to design the best possible API >>> and >>> >>> I would >>> >>> encourage you to get involved if you find it interesting. >>> >>> >>> >>> From my perspective, I think you're overthinking the goal of this >>> current >>> >>> API. I >>> >>> want to write a demo app that shows communication on the internet, or >>> >>> maybe a >>> >>> local server for some files for testing purposes. The goal is not to >>> >>> design the >>> >>> best possible network API that can function as a high-performance >>> server, >>> >>> handle >>> >>> TLS, SPDY, etc. >>> >>> >>> >>> This is not the final API. Enhancements and rewrites will be done. >>> There >>> >>> is no >>> >>> binary stability. Nothing critical will use this API. >>> >>> >>> >>> I'll delete the backlog parameter and leave the pending connection >>> >>> undefined for >>> >>> now. I'll also delete the coalescing function and just use the >>> default >>> >>> since it >>> >>> seems like there are still some open questions and I don't actually >>> need >>> >>> it now >>> >>> anyway. >>> >>> >>> >>> https://codereview.chromium.org/613683006/ >>> >> >>> >> >>> >> >> To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I think we all agree there's no reason to panic (yet), and we all want to be able to move quickly and experiment. I'm sympathetic to Ryan and Will, though, since there's a high likelihood that tomorrow's stable API will evolve only slightly from today's crappy, purely experimental API. (I apologize for what happened with Pepper -- the initial network "APIs" were strictly private and for Flash's purposes only. *facepalm*) That said, here are some thoughts: * Being able to "emulate" the standard BSD socket API reasonably well is a requirement. To enforce this, I suggest that any implementation come with such wrappers (with functions of different names, Γ la winsock). * That said, such wrappers need not offer the best performance, nor should we design the API to make that emulation particularly easy to implement. While many consumers would use the BSD socket API wrapper library or some other library, those who use the API directly should get a well-designed, performant API. * In particular, a logically-correct "objected-oriented" API (in Ryan's terminology) is probably the way to go. In the long run, having an oddball API that breaks your platform's expectations is probably a bad idea. (I'd guess the creators of Winsock still regret what they did, and, e.g., we're still discovering bad security consequences of it even now.) * On the other hand, I'm less concerned about performance, since implementations are less permanent. Note that not only is the .mojom just an interface, but so too are message pipes and data pipes (i.e., if it looks like a message pipe and quacks like a message pipe, then it's a message pipe, even if internally it's totally different from other message pipes). * So there's a lot of room for optimization: one could implement a partly-in-process networking stack (passing around FDs and such), producing "faux" data pipes and such. In a hypothetical distant future, one might even in-kernel implementations. For now, however, we favor straightforward implementations which can be implemented on many different OSes. On Tue, Sep 30, 2014 at 11:21 PM, Ryan Sleevi <rsleevi@chromium.org> wrote: > I think we are just surprised that it went from a discussion in a room to > committed code. > > +1 to experiments > +1 to not blocking on us > +1 to moving quickly and breaking things > > But it would be great just to cc net-dev@, say 'hey, we're experimenting, > follow along here', and move on. > > Even better if there's context and design docs to save review cycles. Or > just context and design docs :) > > Either way, exciting to make progress on this, sorry we can't own it > ourselves, and in future CLS or experiments, please feel absolutely free to > shoot a note to the list for those to follow along and chime in. > > And DEFINITELY before anything goes to become part of external consumers / > decisions we have to support for 6 mos-year for 'reasons' > On Sep 30, 2014 11:10 PM, "Darin Fisher" <darin@chromium.org> wrote: > >> >> >> On Tue, Sep 30, 2014 at 10:52 PM, William Chan (ιζΊζ) < >> willchan@chromium.org> wrote: >> >>> On Tue, Sep 30, 2014 at 10:15 PM, Ryan Sleevi <rsleevi@chromium.org> >>> wrote: >>> > >>> > On Sep 30, 2014 9:47 PM, "Ben Goodger" <beng@google.com> wrote: >>> >> >>> >> Thanks Brett for the useful context. >>> >> >>> >> All, it's usually best to assume that if you're not included on a CL >>> as a >>> >> reviewer that that was an oversight rather than a deliberate action. >>> The >>> >> code review system allows anyone to add themselves as a reviewer for >>> >> anything because speaking up is part of our culture. So is writing >>> straw man >>> >> CLs like this and sending them out. Can we embrace both? Sometimes >>> those CLs >>> >> even land. Code isn't immutable. We're nowhere near even contemplating >>> >> freezing this stuff. I want to make sure we continue to foster an >>> >> environment where people are motivated to propose new things. This is >>> not to >>> >> gloss over the real concerns about building a production quality API. >>> First >>> >> iterations are seldom that API. >>> >> >>> >> Thanks. >>> >> >>> >> -Ben >>> >> >>> > >>> > Ben, >>> > >>> > I think we're particularly sensitive because the past few APIs haven't >>> had >>> > that flexibility, nor have they had the engineering commitment to >>> revisit >>> > past decisions, and thus have ossified into Serious Projects to Clean >>> Up. >>> > >>> > Certainly, of all people in Chrome I trust to iterate and refactor, >>> Brett, >>> > Darin, and Trung are at the top of that list. >>> > >>> > The challenge when CLs come by is that we often lack the context to >>> know the >>> > tradeoffs, what discussions have been had, and what decisions have been >>> > made. So we approach it the same as we have other APIs - trying to >>> gather >>> > that data, provide feedback, and ask 'If this were the final API, do >>> we know >>> > what the tradeoffs are and are we happy with them'. >>> > >>> > We also want to encourage proactive outreach, mostly because we've >>> also seen >>> > teams come back and refactor, and end up with things worse than >>> starting. >>> > >>> > Just knowing what the initial goals for a minimum viable project make >>> it >>> > tremendously easier to provide targeted and actionable feedback. >>> > >>> > I have no doubt everyone is working towards the same goal, and the >>> more we >>> > in the net team can help mojo with that, the better for everyone :) >>> >>> +1 to everything Ryan said. >>> >>> I just want to emphasize that I don't want the net team to be a >>> blocker here. You're experimenting with a new API and iterating. >>> That's great. But we'd like to be involved early on for all the >>> reasons Ryan stated. And, although you suggest it might just be >>> oversight, that's unfortunately what happens every other time, which >>> is why I'm concerned about process failure. In Mojo's case, there are >>> 7 .mojom files already committed without having discussed anything >>> with the net team. I don't think y'all are deliberately not including >>> us, but probably just think we don't need to be involved yet. But we >>> want to be, because in all other cases we've been involved too late, >>> and it's hurt us. >>> >> >> Will, >> >> I don't see a process failure here. I don't see a problem with committing >> experimental code, having to do with networking, that hasn't yet been >> reviewed by the folks most knowledgeable about networking. Surely there is >> room in our project for experimentation. That's all that is going on here. >> I think I also made this clear to you and Ryan about a month ago, so I'm >> surprised that you are surprised. At that time I pointed out that we had >> some crude network service code already, and I encouraged you guys to get >> involved with it. Remember our discussion in Ryan's office? I think it is >> great that you are both interested in making things better here, but let's >> not let perfection be the enemy of progress. Folks are experimenting. We'd >> like to use the source code repository as a way to share our experiments. >> >> -Darin >> >> >> >>> > >>> >> On Tue, Sep 30, 2014 at 9:31 PM, <brettw@chromium.org> wrote: >>> >>> >>> >>> I would _love_ it if you guys wanted to design the best possible API >>> and >>> >>> I would >>> >>> encourage you to get involved if you find it interesting. >>> >>> >>> >>> From my perspective, I think you're overthinking the goal of this >>> current >>> >>> API. I >>> >>> want to write a demo app that shows communication on the internet, or >>> >>> maybe a >>> >>> local server for some files for testing purposes. The goal is not to >>> >>> design the >>> >>> best possible network API that can function as a high-performance >>> server, >>> >>> handle >>> >>> TLS, SPDY, etc. >>> >>> >>> >>> This is not the final API. Enhancements and rewrites will be done. >>> There >>> >>> is no >>> >>> binary stability. Nothing critical will use this API. >>> >>> >>> >>> I'll delete the backlog parameter and leave the pending connection >>> >>> undefined for >>> >>> now. I'll also delete the coalescing function and just use the >>> default >>> >>> since it >>> >>> seems like there are still some open questions and I don't actually >>> need >>> >>> it now >>> >>> anyway. >>> >>> >>> >>> https://codereview.chromium.org/613683006/ >>> >> >>> >> >>> >> >> To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
New patch. This splits everything apart into separate client and server socket objects, with an intermediate "BoundSocket" to represent a bound socket that isn't yet a client or server socket. I believe it should be straightforward to write a BSD adapter layer on top of this. The downside is that more mojo objects and connections are made This has the minumum functionality. There are no options and the only way to close anything is to close the mojo objects (which will always be possible). I believe this should be non-controversial, as any options, connection reuse, async close, etc. should be strict supersets of this behavior.
Overall it LGTM just one nit https://codereview.chromium.org/613683006/diff/140001/mojo/services/public/in... File mojo/services/public/interfaces/network/tcp_bound_socket.mojom (right): https://codereview.chromium.org/613683006/diff/140001/mojo/services/public/in... mojo/services/public/interfaces/network/tcp_bound_socket.mojom:23: StartListening(TCPServerSocket& server); Does it need a callback reporting NetworkError?
https://codereview.chromium.org/613683006/diff/140001/mojo/services/public/in... File mojo/services/public/interfaces/network/tcp_bound_socket.mojom (right): https://codereview.chromium.org/613683006/diff/140001/mojo/services/public/in... mojo/services/public/interfaces/network/tcp_bound_socket.mojom:23: StartListening(TCPServerSocket& server); On 2014/10/02 00:21:46, yzshen1 wrote: > Does it need a callback reporting NetworkError? I'm going to do the implementation and see what we need here. I'm guessing an error callback on the ServerSocketClient can handle this case, since I think we will need to be able to handle the case where an error happens while we're listening. But if that can't happen and StartListening can fail, I'll add the error to a callback here instead.
On 2014/10/02 04:09:30, brettw wrote: > https://codereview.chromium.org/613683006/diff/140001/mojo/services/public/in... > File mojo/services/public/interfaces/network/tcp_bound_socket.mojom (right): > > https://codereview.chromium.org/613683006/diff/140001/mojo/services/public/in... > mojo/services/public/interfaces/network/tcp_bound_socket.mojom:23: > StartListening(TCPServerSocket& server); > On 2014/10/02 00:21:46, yzshen1 wrote: > > Does it need a callback reporting NetworkError? > > I'm going to do the implementation and see what we need here. I'm guessing an > error callback on the ServerSocketClient can handle this case, since I think we > will need to be able to handle the case where an error happens while we're > listening. But if that can't happen and StartListening can fail, I'll add the > error to a callback here instead. SGTM Thanks!
darin@chromium.org changed reviewers: + darin@chromium.org
https://codereview.chromium.org/613683006/diff/140001/mojo/services/public/in... File mojo/services/public/interfaces/network/network_service.mojom (right): https://codereview.chromium.org/613683006/diff/140001/mojo/services/public/in... mojo/services/public/interfaces/network/network_service.mojom:22: // Creates a TCP socket bound to a given local address. This bound socket not that this needs to happen now, but i think it'll make sense to have two separate NetworkService-type interfaces. One is a high level, potentially origin sensitive / origin bound, network service. This would have HTTP, WebSockets, Cookie configuration, etc. The other is a low-level network service that HTTP and WebSockets could be based on. I think TCP, UDP, DNS, proxy config should all be part of such a lower level network service interface.
I added a todo for Darin's suggestion.
The CQ bit was checked by brettw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/613683006/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by brettw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/613683006/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as 32af5e955f7160c802d29b167f5b1f9cdcb76c95
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/c103dc5243dba96e245bf7e0559f7c4d3b3ab574 Cr-Commit-Position: refs/heads/master@{#297907}
Message was sent while issue was closed.
wtc@chromium.org changed reviewers: + wtc@chromium.org
Message was sent while issue was closed.
Patch set 10 LGTM. https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... File mojo/services/public/interfaces/network/network_service.mojom (right): https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... mojo/services/public/interfaces/network/network_service.mojom:16: // high-level origin-build requests like WebSockets and HTTP, and the other for Typo: origin-build => origin-bound https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... mojo/services/public/interfaces/network/network_service.mojom:52: NetAddress? local_address); 1. Few applications are interested in the local address in this case, so returning local_address is almost always wasted work (it requires a getsockname() system call). 2. Nit: just curious why the local_address return value is put on its own line, unlike line 39. https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... File mojo/services/public/interfaces/network/tcp_bound_socket.mojom (right): https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... mojo/services/public/interfaces/network/tcp_bound_socket.mojom:23: StartListening(TCPServerSocket& server); Should we expose the 'backlog' input parameter of the listen() BSD socket function? http://pubs.opengroup.org/onlinepubs/7908799/xns/listen.html https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... File mojo/services/public/interfaces/network/tcp_client_socket.mojom (right): https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... mojo/services/public/interfaces/network/tcp_client_socket.mojom:7: // Represents a TCP socket connected to a report system. What is a "report system"? Did you mean a "remote system"? https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... File mojo/services/public/interfaces/network/tcp_server_socket.mojom (right): https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... mojo/services/public/interfaces/network/tcp_server_socket.mojom:21: TCPClientSocket& client_socket) Nit: it is confusing that TCPServerSocket::AcceptConnection operates on a TCPClientSocket. This is a naming issue. I hope we can come up with a better name for a socket (either client or server side) that has been connected to the peer. One idea is TCPConnectedSocket. This name has one issue -- if the class has an IsConnected() method, that can also be confusing. But it seems less confusing than using a TCPClientSocket on the server side.
Message was sent while issue was closed.
Will do the rest of the suggestions, thanks! https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... File mojo/services/public/interfaces/network/network_service.mojom (right): https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... mojo/services/public/interfaces/network/network_service.mojom:52: NetAddress? local_address); On 2014/10/03 22:37:43, wtc wrote: > > 1. Few applications are interested in the local address in this case, so > returning local_address is almost always wasted work (it requires a > getsockname() system call). How expensive is this syscall? I did it this way because otherwise getting the local address requires an async IPC which is very inconvenient for applications that do want this value. https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... File mojo/services/public/interfaces/network/tcp_bound_socket.mojom (right): https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... mojo/services/public/interfaces/network/tcp_bound_socket.mojom:23: StartListening(TCPServerSocket& server); Yes, a previous patch had this but this caused the code review to go off into places I didn't want to go, so I just removed it and will hardcode it to 2 or something until we feel like designing a system for doing options.
Message was sent while issue was closed.
Like I said to yzshen, all my comments are non-blocking and asynchronous. Just commenting here since there's not a better place to send comments. https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... File mojo/services/public/interfaces/network/network_service.mojom (right): https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... mojo/services/public/interfaces/network/network_service.mojom:15: // TODO Darin suggfests that this should probably be two classes. One for s/suggfests/suggests/ And I agree with Darin's suggestion. https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... mojo/services/public/interfaces/network/network_service.mojom:52: NetAddress? local_address); On 2014/10/03 22:54:44, brettw wrote: > On 2014/10/03 22:37:43, wtc wrote: > > > > 1. Few applications are interested in the local address in this case, so > > returning local_address is almost always wasted work (it requires a > > getsockname() system call). > > How expensive is this syscall? I did it this way because otherwise getting the > local address requires an async IPC which is very inconvenient for applications > that do want this value. To my knowledge, it's a cheap syscall, but it's still got the syscall overhead. But I agree with wtc@ that most applications don't care. https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... File mojo/services/public/interfaces/network/tcp_client_socket.mojom (right): https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... mojo/services/public/interfaces/network/tcp_client_socket.mojom:10: // by the caller when creating the socket. Should the mojo client have the ability to trigger a close() syscall on the socket descriptor? Or does it need to close each datapipe individually (which would trigger separate shutdown() syscall invocations for the read/write directions)? https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... mojo/services/public/interfaces/network/tcp_client_socket.mojom:12: // TODO(brettw) here we put options and whatnot for controlling the Nit: s/whatnot/what not/
Message was sent while issue was closed.
https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... File mojo/services/public/interfaces/network/network_service.mojom (right): https://codereview.chromium.org/613683006/diff/180001/mojo/services/public/in... mojo/services/public/interfaces/network/network_service.mojom:52: NetAddress? local_address); On 2014/10/03 22:54:44, brettw wrote: > > How expensive is this syscall? I did it this way because otherwise getting the > local address requires an async IPC which is very inconvenient for applications > that do want this value. The getsockname system call should be a simple system call, so the cost is probably dominated by the user space/kernel context switch. Getting the local address of such a TCP client socket is an uncommon operation (does any code in Chromium do this?), so it is fine for it to be suboptimal. |
