Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815993002:
Adds a method to TCPServerSocket to adopt a socket. (Closed)
DescriptionAdds a method to TCPServerSocket to adopt a socket.
This is used by headless to adopt a socket opened by a parent process
for remote debugging.
In order to keep the interfaces as regular as possible, this also renames and adds a few methods to the underlying TCPSocketWin, TCPSocketPosix, and SocketPosix classes.
The method rename resulted in bluetooth code changing, so ortuno@ is added as an owner reviewer for the bluetooth code.
Also adds tests for the Adopt[C|Unc]onnectedSocket methods. The server method is just a single line, so there is no explicit test for it.
The Dependent Patchset below uses this new method to
adopt the socket as the devtools remote-debug server connection.
BUG=624837
Review-Url: https://codereview.chromium.org/2815993002
Cr-Commit-Position: refs/heads/master@{#468793}
Committed: https://chromium.googlesource.com/chromium/src/+/26f0a139eaac0d4554cd83c9b736724d2c6553be
Patch Set 1 #Patch Set 2 : Fixed (I hope) Windows compile failure. #
Total comments: 8
Patch Set 3 : Corrected typo in new method name. #Patch Set 4 : Refactored Adopt...Socket methods per mmenke's review comments #Patch Set 5 : Rebased to fix merge conflict #
Total comments: 17
Patch Set 6 : Improved AdoptConnectedSocket test, per review. #Patch Set 7 : Fixed typo in Windows code. #
Total comments: 6
Patch Set 8 : Refined GetSocketDescriptor per review. #
Dependent Patchsets: Messages
Total messages: 60 (42 generated)
The CQ bit was checked by rvera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rvera@chromium.org changed reviewers: + dvallet@chromium.org, jri@chromium.org
Hi Jana, Andrew McGregor gave me your name as an appropriate owner to review a small change to the Chrome TCP stack. I'd appreciate it if you could take a look at this, or delegate it to another owener if you can't or would prefer not to. dvallet@ is the TL of my team, doing a local code review. Thanks, Raul
rvera@chromium.org changed required reviewers: + jri@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by rvera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Adds a method to TCPServerSocket to adopt a socket, for Posix only. This is used by headless to adopt a socket opened by a parent process for remote debugging. Also adds a test for the AdoptConnectedSocket method of TCPSocketPosix, as this was missing. The server method is just a single line, so there is no explicit test for it. This will be followed by a CL that implements using this new method to adopt the socket as the devtools remote-debug server connection. BUG=624837 ========== to ========== Adds a method to TCPServerSocket to adopt a socket, for Posix only. This is used by headless to adopt a socket opened by a parent process for remote debugging. Also adds a test for the AdoptConnectedSocket method of TCPSocketPosix, as this was missing. The server method is just a single line, so there is no explicit test for it. The Dependent Patchset below uses this new method to adopt the socket as the devtools remote-debug server connection. BUG=624837 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
jri@chromium.org changed reviewers: + mmenke@chromium.org
jri@chromium.org changed required reviewers: - jri@chromium.org
+mmenke, since he's better suited. https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... File net/socket/tcp_server_socket.h (right): https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... net/socket/tcp_server_socket.h:35: int AdopListeningRawSocket(int socket_fd); Use "Adopt" instead of "Adop".
https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... File net/socket/tcp_server_socket.cc (right): https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... net/socket/tcp_server_socket.cc:24: return socket_.AdoptConnectedSocket(socket_fd, IPEndPoint()); Using IPEndPoint() seems to violate the API promise, and "AdoptConnectedSocket()" is rather inaccurate for this use case, since the socket isn't connected. TcpSocketWin has an AdoptListenSocket method, but it expects socket that isn't yet listening. I don't suppose we could make a POSIX equivalent, and just use that, and start listening on it ourselves? https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... File net/socket/tcp_server_socket.h (right): https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... net/socket/tcp_server_socket.h:36: #endif Why do we need a POSIX-only method? net/ is supposed to be platform independent.
Thanks for the review. Replies in the file comments. https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... File net/socket/tcp_server_socket.cc (right): https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... net/socket/tcp_server_socket.cc:24: return socket_.AdoptConnectedSocket(socket_fd, IPEndPoint()); On 2017/04/17 at 17:42:18, mmenke wrote: > Using IPEndPoint() seems to violate the API promise, and "AdoptConnectedSocket()" is rather inaccurate for this use case, since the socket isn't connected. > > TcpSocketWin has an AdoptListenSocket method, but it expects socket that isn't yet listening. I don't suppose we could make a POSIX equivalent, and just use that, and start listening on it ourselves? The AdoptConnectedSocket method on TCPSocketPosix was already there, including the misnomer, and used by other code. I have tried to keep my changes to the net directory to a minimum. Expecting that the socket is not yet listening would make this entire change pointless, as the whole idea is to provide a mechanism that allows a parent process to connect immediately, even though Chrome is still starting up and won't reach this call point for some time. See the motivating bug: crbug.com/624837, and the corresponding other CL: https://codereview.chromium.org/2810353003 https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... File net/socket/tcp_server_socket.h (right): https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... net/socket/tcp_server_socket.h:35: int AdopListeningRawSocket(int socket_fd); On 2017/04/17 at 17:27:57, Jana wrote: > Use "Adopt" instead of "Adop". Oops. Thanks. Done. https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... net/socket/tcp_server_socket.h:36: #endif On 2017/04/17 at 17:42:18, mmenke wrote: > Why do we need a POSIX-only method? net/ is supposed to be platform independent. Because Windows does not inherit sockets the same way as POSIX systems do, as far as I can tell. It appears that open sockets are passed on Windows only by active communication between processes, in which case it would be difficult to solve my actual use case, as described in crbug.com/624837. TCPSocketPosix and TCPSocketWin are already different, offering platform-specific methods used by other code. This change effectively elevates one such method to TCPServerSocket, because I need access to that functionality at that level (see https://codereview.chromium.org/2810353003). Regularizing all the socket-adoption methods to make all of this platform independent would be a much larger task, and it may not be possible to guarantee the same semantics across all systems.
https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... File net/socket/tcp_server_socket.cc (right): https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... net/socket/tcp_server_socket.cc:24: return socket_.AdoptConnectedSocket(socket_fd, IPEndPoint()); On 2017/04/17 23:51:57, Raul Vera wrote: > On 2017/04/17 at 17:42:18, mmenke wrote: > > Using IPEndPoint() seems to violate the API promise, and > "AdoptConnectedSocket()" is rather inaccurate for this use case, since the > socket isn't connected. > > > > TcpSocketWin has an AdoptListenSocket method, but it expects socket that isn't > yet listening. I don't suppose we could make a POSIX equivalent, and just use > that, and start listening on it ourselves? > > The AdoptConnectedSocket method on TCPSocketPosix was already there, including > the misnomer, and used by other code. I have tried to keep my changes to the net > directory to a minimum. > > Expecting that the socket is not yet listening would make this entire change > pointless, as the whole idea is to provide a mechanism that allows a parent > process to connect immediately, even though Chrome is still starting up and > won't reach this call point for some time. See the motivating bug: > crbug.com/624837, and the corresponding other CL: > https://codereview.chromium.org/2810353003 I'm not seeing a consumer in the chrome repo that passes in a connection that's not connected. Regardless, mind renaming the method (Just AdoptSocket is fine. There aren't a lot of consumers), and documenting it? (Important things: Takes ownership of a socket that may or may not be connected or bound. The IPEndPoint is optional, and is what will be returned by GetPeerAddress(). https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... File net/socket/tcp_server_socket.h (right): https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... net/socket/tcp_server_socket.h:36: #endif On 2017/04/17 23:51:57, Raul Vera wrote: > On 2017/04/17 at 17:42:18, mmenke wrote: > > Why do we need a POSIX-only method? net/ is supposed to be platform > independent. > > Because Windows does not inherit sockets the same way as POSIX systems do, as > far as I can tell. It appears that open sockets are passed on Windows only by > active communication between processes, in which case it would be difficult to > solve my actual use case, as described in crbug.com/624837. I don't think this is relevant to code in net/, which doesn't even know what a process is. > TCPSocketPosix and TCPSocketWin are already different, offering > platform-specific methods used by other code. This change effectively elevates > one such method to TCPServerSocket, because I need access to that functionality > at that level (see https://codereview.chromium.org/2810353003). Regularizing all > the socket-adoption methods to make all of this platform independent would be a > much larger task, and it may not be possible to guarantee the same semantics > across all systems. There's only one extra method in the Windows method (Which should be on the POSIX version, too, in my opinion). I assume the classes aren't virtual to avoid a vtable lookup, which is kinda silly, since we don't do that anywhere else. Ancient code does tend not to follow established norms, though. You're promoting the interface from what is nominally a private POSIX-only class (SocketPosix has no windows equivalent) to a cross-platform class. Why would adding a comparable method to TcpSocketWin be so difficult?
OK. I see your points now. Thanks. I've refactored a bit to ensure that SocketPosix, TCPSocketPosix, and TCPSocketWin all have the following pair of methods: AdoptConnectedSocket(SocketDescriptor socket, <peer address in appropriate form>) AdoptUnconnectedSocke(SocketDescriptor socket) All are carefully documented, including the varying semantics, in the respective .h files. The surprisingly few call points have all been changed. It was easier than I expected. On TcpServerSocket I have only the unconnected call, as it's the only one that makes sense to me in that context. PTAL and thanks for pushing me to do the cleanup. On 2017/04/18 at 16:15:25, mmenke wrote: > https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... > File net/socket/tcp_server_socket.cc (right): > > https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... > net/socket/tcp_server_socket.cc:24: return socket_.AdoptConnectedSocket(socket_fd, IPEndPoint()); > On 2017/04/17 23:51:57, Raul Vera wrote: > > On 2017/04/17 at 17:42:18, mmenke wrote: > > > Using IPEndPoint() seems to violate the API promise, and > > "AdoptConnectedSocket()" is rather inaccurate for this use case, since the > > socket isn't connected. > > > > > > TcpSocketWin has an AdoptListenSocket method, but it expects socket that isn't > > yet listening. I don't suppose we could make a POSIX equivalent, and just use > > that, and start listening on it ourselves? > > > > The AdoptConnectedSocket method on TCPSocketPosix was already there, including > > the misnomer, and used by other code. I have tried to keep my changes to the net > > directory to a minimum. > > > > Expecting that the socket is not yet listening would make this entire change > > pointless, as the whole idea is to provide a mechanism that allows a parent > > process to connect immediately, even though Chrome is still starting up and > > won't reach this call point for some time. See the motivating bug: > > crbug.com/624837, and the corresponding other CL: > > https://codereview.chromium.org/2810353003 > > I'm not seeing a consumer in the chrome repo that passes in a connection that's not connected. Regardless, mind renaming the method (Just AdoptSocket is fine. There aren't a lot of consumers), and documenting it? (Important things: Takes ownership of a socket that may or may not be connected or bound. The IPEndPoint is optional, and is what will be returned by GetPeerAddress(). > > https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... > File net/socket/tcp_server_socket.h (right): > > https://codereview.chromium.org/2815993002/diff/20001/net/socket/tcp_server_s... > net/socket/tcp_server_socket.h:36: #endif > On 2017/04/17 23:51:57, Raul Vera wrote: > > On 2017/04/17 at 17:42:18, mmenke wrote: > > > Why do we need a POSIX-only method? net/ is supposed to be platform > > independent. > > > > Because Windows does not inherit sockets the same way as POSIX systems do, as > > far as I can tell. It appears that open sockets are passed on Windows only by > > active communication between processes, in which case it would be difficult to > > solve my actual use case, as described in crbug.com/624837. > > I don't think this is relevant to code in net/, which doesn't even know what a process is. > > > TCPSocketPosix and TCPSocketWin are already different, offering > > platform-specific methods used by other code. This change effectively elevates > > one such method to TCPServerSocket, because I need access to that functionality > > at that level (see https://codereview.chromium.org/2810353003). Regularizing all > > the socket-adoption methods to make all of this platform independent would be a > > much larger task, and it may not be possible to guarantee the same semantics > > across all systems. > > There's only one extra method in the Windows method (Which should be on the POSIX version, too, in my opinion). I assume the classes aren't virtual to avoid a vtable lookup, which is kinda silly, since we don't do that anywhere else. Ancient code does tend not to follow established norms, though. > > You're promoting the interface from what is nominally a private POSIX-only class (SocketPosix has no windows equivalent) to a cross-platform class. > > Why would adding a comparable method to TcpSocketWin be so difficult?
rvera@chromium.org changed required reviewers: + mmenke@chromium.org
The CQ bit was checked by rvera@chromium.org
PTAL. Longer response below.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
All required reviewers (with asterisk prefixes) have not yet approved this CL. No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== Adds a method to TCPServerSocket to adopt a socket, for Posix only. This is used by headless to adopt a socket opened by a parent process for remote debugging. Also adds a test for the AdoptConnectedSocket method of TCPSocketPosix, as this was missing. The server method is just a single line, so there is no explicit test for it. The Dependent Patchset below uses this new method to adopt the socket as the devtools remote-debug server connection. BUG=624837 ========== to ========== Adds a method to TCPServerSocket to adopt a socket. This is used by headless to adopt a socket opened by a parent process for remote debugging. In order to keep the interfaces as regular as possible, this also renames and adds a few methods to the underlying TCPSocketWin, TCPSocketPosix, and SocketPosix classes. The method rename resulted in bluetooth code changing, so ortuno@ is added as an owner reviewer for the bluetooth code. Also adds tests for the Adopt[C|Unc]onnectedSocket methods. The server method is just a single line, so there is no explicit test for it. The Dependent Patchset below uses this new method to adopt the socket as the devtools remote-debug server connection. BUG=624837 ==========
rvera@chromium.org changed reviewers: + ortuno@chromium.org
rvera@chromium.org changed required reviewers: + ortuno@chromium.org
The CQ bit was checked by rvera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rvera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm. I didn't get an email about this O.O It might have been the '*' before my username.
Looking pretty good, thanks for updating things on Windows, too! https://codereview.chromium.org/2815993002/diff/80001/net/socket/socket_posix.cc File net/socket/socket_posix.cc (right): https://codereview.chromium.org/2815993002/diff/80001/net/socket/socket_posix... net/socket/socket_posix.cc:104: } nit: Don't use braces for one line if bodies. https://codereview.chromium.org/2815993002/diff/80001/net/socket/socket_posix.h File net/socket/socket_posix.h (right): https://codereview.chromium.org/2815993002/diff/80001/net/socket/socket_posix... net/socket/socket_posix.h:40: // listening. The caller must determine the state of the socket based on its Since it may be open (Or may not), maybe just call it AdoptSocket? https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_server_s... File net/socket/tcp_server_socket.h (right): https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_server_s... net/socket/tcp_server_socket.h:31: int AdoptUnconnectedSocket(SocketDescriptor socket); This works for connected sockets, too, right? Maybe just AdoptSocket? https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_socket_u... File net/socket/tcp_socket_unittest.cc (right): https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_socket_u... net/socket/tcp_socket_unittest.cc:14: #include "base/files/file_util.h" I don't think this is used? https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_socket_u... net/socket/tcp_socket_unittest.cc:27: #endif No longer needed, I believe. https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_socket_u... net/socket/tcp_socket_unittest.cc:243: ASSERT_GE(listen(socket_fd, kListenBacklog), 0); This isn't a connected socket, it's a listening socket. Seems like we should use a connected socket for this test. https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_socket_u... net/socket/tcp_socket_unittest.cc:261: ASSERT_EQ(bind(existing_socket, storage.addr, storage.addr_len), 0); Expected should go before actual, for ASSERT_EQ (Yes, it's weird that ASSERT_THAT uses the opposite order) https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_socket_w... File net/socket/tcp_socket_win.h (right): https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_socket_w... net/socket/tcp_socket_win.h:48: const SockaddrStorage& peer_address); IPEndPoint
Also, note that there's a "CQ dry run" button, so you don't have to get that warning about not having the signoff from a reviewer yet. :)
The CQ bit was checked by rvera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
rvera@chromium.org changed reviewers: - dvallet@chromium.org, jri@chromium.org, ortuno@chromium.org
rvera@chromium.org changed required reviewers: - mmenke@chromium.org, ortuno@chromium.org
Hi Matt, Here is my latest version. Have I paid enough technical debt yet? :-) The connected test was a bit more complicated than I would have liked, and necessitated a small tradeoff. See the comments below and in the code. Thanks again, Raul https://codereview.chromium.org/2815993002/diff/80001/net/socket/socket_posix.cc File net/socket/socket_posix.cc (right): https://codereview.chromium.org/2815993002/diff/80001/net/socket/socket_posix... net/socket/socket_posix.cc:104: } On 2017/04/21 at 19:20:38, mmenke wrote: > nit: Don't use braces for one line if bodies. Done. But I'd like to point out that the style guide doesn't say anything about this, so defaults back to the Google style guide, which says you can do it if you prefer. I consider one-line if bodies without braces to be bugs waiting to happen when someone adds a line and doesn't notice that the braces aren't there. Go even eliminated braceless ifs for this reason, IIRC. https://codereview.chromium.org/2815993002/diff/80001/net/socket/socket_posix.h File net/socket/socket_posix.h (right): https://codereview.chromium.org/2815993002/diff/80001/net/socket/socket_posix... net/socket/socket_posix.h:40: // listening. The caller must determine the state of the socket based on its On 2017/04/21 at 19:20:38, mmenke wrote: > Since it may be open (Or may not), maybe just call it AdoptSocket? But that loses the symmetry with AdoptConnectedSocket, and it can't be a connected socket. I've added a further comment to that effect across the board. https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_server_s... File net/socket/tcp_server_socket.h (right): https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_server_s... net/socket/tcp_server_socket.h:31: int AdoptUnconnectedSocket(SocketDescriptor socket); On 2017/04/21 at 19:20:38, mmenke wrote: > This works for connected sockets, too, right? Maybe just AdoptSocket? It doesn't work for connected sockets, as the peer address will not get set properly. The idea is that the server code will subsequently call Accept, which will set the peer address. However, in the case of a server only an unconnected socket makes sense so it doesn't matter much. I've changed the name to what you suggest and added a comment that it should not be connected. https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_socket_u... File net/socket/tcp_socket_unittest.cc (right): https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_socket_u... net/socket/tcp_socket_unittest.cc:14: #include "base/files/file_util.h" On 2017/04/21 at 19:20:38, mmenke wrote: > I don't think this is used? It was, but isn't now, so I've removed it. https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_socket_u... net/socket/tcp_socket_unittest.cc:27: #endif On 2017/04/21 at 19:20:38, mmenke wrote: > No longer needed, I believe. Good catch. I missed that. Thanks. https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_socket_u... net/socket/tcp_socket_unittest.cc:243: ASSERT_GE(listen(socket_fd, kListenBacklog), 0); On 2017/04/21 at 19:20:38, mmenke wrote: > This isn't a connected socket, it's a listening socket. Seems like we should use a connected socket for this test. Indeed. This turned out to require exposing GetSocketDescriptor on TCPSocketWin and TCPSocketPosix, so that I could build the test in a platform-independent way. See the new comments in the code about the resulting error, too. If you can think of a cleaner way to do this, let me know. In particular if you can think of a way to use a nonblocking TCPSocket::Connect(...) and a blocking low-level accept(...) on the single thread without the deadlock I saw, please let me know. My sockets foo just isn't strong enough. https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_socket_u... net/socket/tcp_socket_unittest.cc:261: ASSERT_EQ(bind(existing_socket, storage.addr, storage.addr_len), 0); On 2017/04/21 at 19:20:38, mmenke wrote: > Expected should go before actual, for ASSERT_EQ (Yes, it's weird that ASSERT_THAT uses the opposite order) That code is gone now, but thanks. https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_socket_w... File net/socket/tcp_socket_win.h (right): https://codereview.chromium.org/2815993002/diff/80001/net/socket/tcp_socket_w... net/socket/tcp_socket_win.h:48: const SockaddrStorage& peer_address); On 2017/04/21 at 19:20:38, mmenke wrote: > IPEndPoint I guess you can tell I don't have a Windows machine to compile on. I'll go fix that. Thanks.
One more (hopefully minor) piece of technical debt I'd like to pay down: The double closes. Which should be benign, but we've run into enough issues with them in production that I'd prefer to just avoid them out of paranoia. I hope you don't feel that I'm being too unreasonable on the technical debt front. This is the last thing, honest! https://codereview.chromium.org/2815993002/diff/80001/net/socket/socket_posix.cc File net/socket/socket_posix.cc (right): https://codereview.chromium.org/2815993002/diff/80001/net/socket/socket_posix... net/socket/socket_posix.cc:104: } On 2017/05/01 05:35:25, Raul Vera wrote: > On 2017/04/21 at 19:20:38, mmenke wrote: > > nit: Don't use braces for one line if bodies. > > Done. But I'd like to point out that the style guide doesn't say anything about > this, so defaults back to the Google style guide, which says you can do it if > you prefer. I consider one-line if bodies without braces to be bugs waiting to > happen when someone adds a line and doesn't notice that the braces aren't there. > Go even eliminated braceless ifs for this reason, IIRC. Note that you should generally be consistent with code around you. In this case, net/ does not use braces in this case (Largely because Chromium style used to say not to use braces for single line ifs). https://codereview.chromium.org/2815993002/diff/120001/net/socket/tcp_socket_... File net/socket/tcp_socket_posix.h (right): https://codereview.chromium.org/2815993002/diff/120001/net/socket/tcp_socket_... net/socket/tcp_socket_posix.h:126: SocketDescriptor GetSocketDescriptor() const; GetSocketDescriptorForTesting() (We have some checks that functions with such names are only used in tests) https://codereview.chromium.org/2815993002/diff/120001/net/socket/tcp_socket_... File net/socket/tcp_socket_unittest.cc (right): https://codereview.chromium.org/2815993002/diff/120001/net/socket/tcp_socket_... net/socket/tcp_socket_unittest.cc:266: // message seems the best tradeoff. Can we do better? Shouldn't be hard to make GetSocketDescriptor steal the socket instead (And rename it to ReleaseSocketDescriptorForTesting or ReleaseConnectedSocketForTesting()). On POSIX, we can just use: SocketDescriptor socket_descriptor = socket_->ReleaseConnectedSocket(); socket_.reset(); return socket_descriptor; On Windows, it would be: SocketDescriptor socket_descriptor = socket_; socket_ = INVALID_SOCKET; Close(); return socket_descriptor; And then add a warning to both methods that read/write/accept operations must not be pending when stealing the socket. https://codereview.chromium.org/2815993002/diff/120001/net/socket/tcp_socket_... File net/socket/tcp_socket_win.h (right): https://codereview.chromium.org/2815993002/diff/120001/net/socket/tcp_socket_... net/socket/tcp_socket_win.h:124: SocketDescriptor GetSocketDescriptor() const { return socket_; } GetSocketDescriptorForTesting
rvera@chromium.org changed required reviewers: + mmenke@chromium.org
Thanks Matt, No worries about the technical debt. It's just such a classic Google thing that when you go touch old code that belongs to someone else, they ask you to pay some equally old technical debt before allowing you to make your minor change. (And then you're the last person who touched that code so people sometimes hassle you about random subtle changes in behavior.) Of course it is the right thing to do, so I don't mean to complain, but it's so classic it's funny. So I won't tell you about the other bits of technical debt I've found, but I may surprise you with a further pure-cleanup CL later on, once this tired old bug is put to rest. Anyway, I've implemented your suggestions, which were all great and taught me stuff about the code and about Chromium infrastructure. PTAL and thank you. Raul https://codereview.chromium.org/2815993002/diff/120001/net/socket/tcp_socket_... File net/socket/tcp_socket_posix.h (right): https://codereview.chromium.org/2815993002/diff/120001/net/socket/tcp_socket_... net/socket/tcp_socket_posix.h:126: SocketDescriptor GetSocketDescriptor() const; On 2017/05/01 at 18:44:19, mmenke wrote: > GetSocketDescriptorForTesting() > > (We have some checks that functions with such names are only used in tests) That's very cool. Done. Thanks. https://codereview.chromium.org/2815993002/diff/120001/net/socket/tcp_socket_... File net/socket/tcp_socket_unittest.cc (right): https://codereview.chromium.org/2815993002/diff/120001/net/socket/tcp_socket_... net/socket/tcp_socket_unittest.cc:266: // message seems the best tradeoff. On 2017/05/01 at 18:44:19, mmenke wrote: > Can we do better? Shouldn't be hard to make GetSocketDescriptor steal the socket instead (And rename it to ReleaseSocketDescriptorForTesting or ReleaseConnectedSocketForTesting()). > > On POSIX, we can just use: > > SocketDescriptor socket_descriptor = socket_->ReleaseConnectedSocket(); > socket_.reset(); > return socket_descriptor; > > On Windows, it would be: > > SocketDescriptor socket_descriptor = socket_; > socket_ = INVALID_SOCKET; > Close(); > return socket_descriptor; > > > And then add a warning to both methods that read/write/accept operations must not be pending when stealing the socket. Excellent. All done. Thank you. https://codereview.chromium.org/2815993002/diff/120001/net/socket/tcp_socket_... File net/socket/tcp_socket_win.h (right): https://codereview.chromium.org/2815993002/diff/120001/net/socket/tcp_socket_... net/socket/tcp_socket_win.h:124: SocketDescriptor GetSocketDescriptor() const { return socket_; } On 2017/05/01 at 18:44:19, mmenke wrote: > GetSocketDescriptorForTesting Done.
The CQ bit was checked by rvera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, thanks!
The CQ bit was checked by rvera@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org Link to the patchset: https://codereview.chromium.org/2815993002/#ps140001 (title: "Refined GetSocketDescriptor per review.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493763518778030,
"parent_rev": "cce1f4f3b02e3a65efb572ffdc292724a7ca50b5", "commit_rev":
"26f0a139eaac0d4554cd83c9b736724d2c6553be"}
Message was sent while issue was closed.
Description was changed from ========== Adds a method to TCPServerSocket to adopt a socket. This is used by headless to adopt a socket opened by a parent process for remote debugging. In order to keep the interfaces as regular as possible, this also renames and adds a few methods to the underlying TCPSocketWin, TCPSocketPosix, and SocketPosix classes. The method rename resulted in bluetooth code changing, so ortuno@ is added as an owner reviewer for the bluetooth code. Also adds tests for the Adopt[C|Unc]onnectedSocket methods. The server method is just a single line, so there is no explicit test for it. The Dependent Patchset below uses this new method to adopt the socket as the devtools remote-debug server connection. BUG=624837 ========== to ========== Adds a method to TCPServerSocket to adopt a socket. This is used by headless to adopt a socket opened by a parent process for remote debugging. In order to keep the interfaces as regular as possible, this also renames and adds a few methods to the underlying TCPSocketWin, TCPSocketPosix, and SocketPosix classes. The method rename resulted in bluetooth code changing, so ortuno@ is added as an owner reviewer for the bluetooth code. Also adds tests for the Adopt[C|Unc]onnectedSocket methods. The server method is just a single line, so there is no explicit test for it. The Dependent Patchset below uses this new method to adopt the socket as the devtools remote-debug server connection. BUG=624837 Review-Url: https://codereview.chromium.org/2815993002 Cr-Commit-Position: refs/heads/master@{#468793} Committed: https://chromium.googlesource.com/chromium/src/+/26f0a139eaac0d4554cd83c9b736... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/26f0a139eaac0d4554cd83c9b736... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
