Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23881002:
Windows only: Move client socket functionality from TCPClientSocket into TCPSocket. (Closed)
|
Created:
7 years, 3 months ago by yzshen1 Modified:
7 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Wez Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWindows only: Move client socket functionality from TCPClientSocket into TCPSocket.
TCPClientSocket becomes a wrapper around TCPSocket to expose a client-only interface.
The corresponding change for POSIX will be in a separate CL.
BUG=262601
TEST=new tests in tcp_socket_unittest.cc
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223286
Patch Set 1 #Patch Set 2 : #
Total comments: 82
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 10
Patch Set 7 : #Patch Set 8 : #
Total comments: 11
Patch Set 9 : #Patch Set 10 : #
Total comments: 8
Messages
Total messages: 20 (0 generated)
Hi, Fred and Wan-Teh. Would you please take a look? Thanks!
On 2013/09/03 16:32:05, yzshen1 wrote: > Hi, Fred and Wan-Teh. > > Would you please take a look? Thanks! A few notes: - Unlike TCPClientSocket::Connect() which could try multiple addresses, TCPSocket::Connect() takes a single IP endpoint. Therefore, the state machine of doing multiple connect attempts stays in TCPClientSocket. - UseHistory also stays in TCPClientSocket. (I did this because UseHistory is defined in StreamSocket which TCPSocket doesn't inherit.) I am not quite sure how the log is used. Please let me know if there is any problem. Thanks!
will look by today
mostly nits https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... File net/socket/tcp_client_socket.cc (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:87: DCHECK(socket_.get()); no need for .get() https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:91: PLOG(ERROR) << "TCPSocket::SetDefaultOptionsForClient() returned an error"; this is unfortunate. Do we really want to swallow this error? Perhaps we need two-phase init? https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:100: if (current_address_index_ >= 0 || bind_address_.get()) { no .get() https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:102: return ERR_UNEXPECTED; ERR_UNEXPECTED is usually found with NOTREACHED() https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:160: LOG(DFATAL) << "bad state " << state; NOTREACHED() instead of LOG(DFATAL)? (They're equivalent for NOTREACHED() seems clearer) https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:183: DCHECK(bind_address_.get()); no .get() (also everywhere else) https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:296: &TCPClientSocket::DidCompleteReadWrite, base::Unretained(this), callback); comment as to why Unretained() can be used safely here https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... File net/socket/tcp_client_socket.h (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.h:24: #elif defined(OS_POSIX) test for OS_POSIX first to match block below? https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.h:52: // given socket and then acts as if Connect() had been called. This function No need to mention ownership, since scoped_ptr<> is used. Perhaps: Adopts the given connected socket and then acts as if Connect() has been called. This constructor's main user is TCPServerSocket. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.h:54: explicit TCPClientSocket(scoped_ptr<TCPSocket> connected_socket, no need for explicit https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.h:80: // Full duplex mode (reading and writing at the same time) is supported period at end https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.h:117: // was't called (in that cases OS chooses address/port). was't -> wasn't in that cases -> in that case https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win.cc File net/socket/tcp_socket_win.cc (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:26: const int kTCPKeepAliveSeconds = 45; i'm assuming most of the new code here was just moved from tcp_client_socket_win.cc? https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:304: DCHECK(!core_.get()); no .get()
Thanks Fred and Wan-Teh! https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... File net/socket/tcp_client_socket.cc (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:87: DCHECK(socket_.get()); On 2013/09/05 22:52:28, akalin wrote: > no need for .get() Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:91: PLOG(ERROR) << "TCPSocket::SetDefaultOptionsForClient() returned an error"; I think this is not a fatal issue, maybe we can swallow it? Actually SetDefaultOptionsForClient() never returns failure. It behaves the same as the previous code: https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/tcp_cli... One minor annoyance about two-phase init is that either (1) I need to set |socket_| to NULL before Init(connected_socket). In this case, I need to check NULL for |socket_| in other places; or (2) I create a |socket_| and later in Init(connected_socket) replace it with |connected_socket|. In this case, two TCPSocket are created, and therefore we log two NetLog::TYPE_SOCKET_ALIVE events. Comparing with two-phase init, it seems easier to have a static Create() methods which returns a valid TCPSocket pointer on success. What do you think? On 2013/09/05 22:52:28, akalin wrote: > this is unfortunate. Do we really want to swallow this error? Perhaps we need > two-phase init? https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:100: if (current_address_index_ >= 0 || bind_address_.get()) { On 2013/09/05 22:52:28, akalin wrote: > no .get() Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:102: return ERR_UNEXPECTED; On 2013/09/05 22:52:28, akalin wrote: > ERR_UNEXPECTED is usually found with NOTREACHED() Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:160: LOG(DFATAL) << "bad state " << state; On 2013/09/05 22:52:28, akalin wrote: > NOTREACHED() instead of LOG(DFATAL)? (They're equivalent for NOTREACHED() seems > clearer) Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:183: DCHECK(bind_address_.get()); On 2013/09/05 22:52:28, akalin wrote: > no .get() (also everywhere else) Done. Also changed other places. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:296: &TCPClientSocket::DidCompleteReadWrite, base::Unretained(this), callback); On 2013/09/05 22:52:28, akalin wrote: > comment as to why Unretained() can be used safely here Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... File net/socket/tcp_client_socket.h (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.h:24: #elif defined(OS_POSIX) On 2013/09/05 22:52:28, akalin wrote: > test for OS_POSIX first to match block below? Done. It will be removed in the next CL. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.h:52: // given socket and then acts as if Connect() had been called. This function On 2013/09/05 22:52:28, akalin wrote: > No need to mention ownership, since scoped_ptr<> is used. Perhaps: > > Adopts the given connected socket and then acts as if Connect() has been called. > This constructor's main user is TCPServerSocket. Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.h:54: explicit TCPClientSocket(scoped_ptr<TCPSocket> connected_socket, On 2013/09/05 22:52:28, akalin wrote: > no need for explicit Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.h:80: // Full duplex mode (reading and writing at the same time) is supported On 2013/09/05 22:52:28, akalin wrote: > period at end Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.h:117: // was't called (in that cases OS chooses address/port). On 2013/09/05 22:52:28, akalin wrote: > was't -> wasn't > in that cases -> in that case Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win.cc File net/socket/tcp_socket_win.cc (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:26: const int kTCPKeepAliveSeconds = 45; On 2013/09/05 22:52:28, akalin wrote: > i'm assuming most of the new code here was just moved from > tcp_client_socket_win.cc? Yes. A few major changes include: - the mutiple-connect-attempts logic is split out and lives in tcp_client_socket. (Therefore, maybe you want to look at those *Connect*() methods.) - the UseHistory logging logic is also split out and lives in tcp_client_socket. - Close() is made to reset most of the internal state (except logging), so that it is okay to reuse the object and call Create() again. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:304: DCHECK(!core_.get()); On 2013/09/05 22:52:28, akalin wrote: > no .get() Done.
Review comments on patch set 2: I was in the middle of reviewing patch set 2 when you uploaded patch set 3, so I didn't switch to patch set 3. I am sorry if some of the issues I found are already fixed in patch set 3. I did a rather careful review. The CL seems good. The only things I don't quite like are: 1. How multiple TCP connect attempts of TCPClientSocket are logged. It seems to be partially done in TCPSocket and partially done in TCPClientSocket. This is confusing. Unfortunately I could not suggest a solution. 2) The SetDefaultOptionsForClient method: I think it should be automatically called internally by TCPSocket at the right time. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... File net/socket/tcp_client_socket.cc (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:10: #if defined(OS_WIN) No need to add #if defined(OS_WIN) because these headers will also be used by OS_POSIX. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:89: int result = socket_->SetDefaultOptionsForClient(); This should have been called when the passed-in TCPSocket was created. See my comment about SetDefaultOptionsForClient in tcp_socket.cc. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:335: DCHECK_EQ(next_connect_state_, CONNECT_STATE_CONNECT_COMPLETE); You can also DCHECK these: DCHECK_NE(result, ERR_IO_PENDING); DCHECK(!connect_callback_.is_null()); https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:342: callback.Run(result); I remember there is a new CompletionCallback method (seems to be called ResetAndRun) that does these three lines in one shot. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:347: const CompletionCallback& forward_callback, Why is this argument named "forward_callback"? Why not simply "callback"? https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:362: result = socket_->SetDefaultOptionsForClient(); SetDefaultOptionsForClient can be called by TCPSocket, either in the constructor, or when the socket is successfully connected. Then we don't need the CreateSocket() method in this class; we can just call socket_->Create(family). https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_uni... File net/socket/tcp_socket_unittest.cc (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_uni... net/socket/tcp_socket_unittest.cc:7: #include <cstring> Nit: can you <string.h>? https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_uni... net/socket/tcp_socket_unittest.cc:235: scoped_refptr<net::IOBufferWithSize> write_buffer( Remove all net:: because this file is in the net namespace. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win.cc File net/socket/tcp_socket_win.cc (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:24: namespace { Nit: add a blank line before this line. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:308: if (SetNonBlocking(socket_)) { Should we also call SetupSocket(socket_)? https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:315: peer_address_ = peer_address; Is it necessary to have the peer_address_ member? It can be looked up with a getpeername() call. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:359: return ERR_FAILED; We should return MapSystemError(WSAGetLastError()). https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:404: // If in the process of connecting or already connected, then just return OK. This seems wrong. I think we should return an error code if waiting_connect_ is true. Also, IsConnected() means something different -- it means the socket has been connected AND the TCP connection is still good. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:417: // TODO(ajwong): Is setting read_callback_ the right thing to do here?? Remove this TODO comment. It is correct to use read_callback_ for connect or read. This is documented in tcp_socket_win.h. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:548: int TCPSocketWin::SetDefaultOptionsForClient() { 1. Please copy (and update as appropriate) the original comment: // Sets socket parameters. Returns the OS error code (or 0 on // success). 2. IMPORTANT: This function is also suitable for an accepted socket returned by a listening server socket. So perhaps we should not have "ForClient" in the function's name. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:630: socket_ = INVALID_SOCKET; I think most of the following code (except the accept_xxx code) should be moved inside this "if (socket_ != INVALID_SOCKET)" block. That's what the original code in tcp_client_socket_win.cc is like. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:643: } This code is not in the original code. Is that a bug? https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:663: write_callback_.Reset(); Are these two lines necessary? They are not in the original tcp_client_socket_win.cc code. Perhaps that was a bug? https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:802: void TCPSocketWin::LogConnectStart(const AddressList& addresses) { I think this function and LogConnectCompletion belong in TCPClientSocketWin. It is confusing how to use the two LogXXX functions. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win.h File net/socket/tcp_socket_win.h (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.h:33: int Create(AddressFamily family); "Open" is a better name. "Create" is usually a method that creates an object and returns it. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.h:48: int GetPeerAddress(IPEndPoint* address) const; I suggest declaring GetPeerAddress next to the related GetLocalAddress function. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.h:66: bool IsValid() const { return socket_ != INVALID_SOCKET; } You can consider renaming this method "IsOpen". https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.h:70: // TCPClientSocket may attempt to connect to multiple addresses until it TCPClientSocket => TCPSocket or did you really mean TCPClientSocket? Do the StartLoggingMultipleConnectAttempts and EndLoggingMultipleConnectAttempts methods belong in this class? https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.h:71: // succeeds in establishing a connection. The corresponding log will be Nit: will be => will have ?
Thanks Wan-Teh. Please take another look. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... File net/socket/tcp_client_socket.cc (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:10: #if defined(OS_WIN) On 2013/09/06 21:55:42, wtc wrote: > > No need to add #if defined(OS_WIN) because these headers will also be used by > OS_POSIX. Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:89: int result = socket_->SetDefaultOptionsForClient(); Please see my comments below at line 362. On 2013/09/06 21:55:42, wtc wrote: > > This should have been called when the passed-in TCPSocket was created. > See my comment about SetDefaultOptionsForClient in tcp_socket.cc. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:335: DCHECK_EQ(next_connect_state_, CONNECT_STATE_CONNECT_COMPLETE); On 2013/09/06 21:55:42, wtc wrote: > > You can also DCHECK these: > > DCHECK_NE(result, ERR_IO_PENDING); > DCHECK(!connect_callback_.is_null()); Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:342: callback.Run(result); On 2013/09/06 21:55:42, wtc wrote: > > I remember there is a new CompletionCallback method (seems to be called > ResetAndRun) that does these three lines in one shot. Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:347: const CompletionCallback& forward_callback, On 2013/09/06 21:55:42, wtc wrote: > > Why is this argument named "forward_callback"? Why not simply "callback"? Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:362: result = socket_->SetDefaultOptionsForClient(); IMO, this seems to cause inconsistency: Do we also want to let TCPSocket itself calls SetDefaultOptionsForServer()? It seems we cannot easily do that: SetDefaultOptionsForServer() requires to be called before Bind(), I think. And at that point, TCPSocket doesn't really know whether it is going to be used as server or client. I think it is the users of TCPSocket to decide what options to use. SetDefaultOptionsFor(Client|Server) are convenient helpers for the common cases, but in some cases maybe the users of TCPSocket want to set different options? What do you think? On 2013/09/06 21:55:42, wtc wrote: > > SetDefaultOptionsForClient can be called by TCPSocket, either in the > constructor, or when the socket is successfully connected. Then we don't > need the CreateSocket() method in this class; we can just call > socket_->Create(family). https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_uni... File net/socket/tcp_socket_unittest.cc (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_uni... net/socket/tcp_socket_unittest.cc:7: #include <cstring> On 2013/09/06 21:55:42, wtc wrote: > > Nit: can you <string.h>? Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_uni... net/socket/tcp_socket_unittest.cc:235: scoped_refptr<net::IOBufferWithSize> write_buffer( On 2013/09/06 21:55:42, wtc wrote: > > Remove all net:: because this file is in the net namespace. Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win.cc File net/socket/tcp_socket_win.cc (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:24: namespace { On 2013/09/06 21:55:42, wtc wrote: > > Nit: add a blank line before this line. Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:308: if (SetNonBlocking(socket_)) { IMO, what options to set is the decision of TCPSocket users. What do you think? On 2013/09/06 21:55:42, wtc wrote: > > Should we also call SetupSocket(socket_)? https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:315: peer_address_ = peer_address; This is the same behavior as the previous code, which also stored the peer address and returned it directly in GetPeerAddress(). I agree that getpeername() could be used. On 2013/09/06 21:55:42, wtc wrote: > > Is it necessary to have the peer_address_ member? It can be looked up with a > getpeername() call. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:359: return ERR_FAILED; On 2013/09/06 21:55:42, wtc wrote: > > We should return MapSystemError(WSAGetLastError()). Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:404: // If in the process of connecting or already connected, then just return OK. On 2013/09/06 21:55:42, wtc wrote: > > This seems wrong. I think we should return an error code if waiting_connect_ > is true. Also, IsConnected() means something different -- it means the socket > has been connected AND the TCP connection is still good. Returning OK is the behavior of the previous code. I agree that an error makes more sense. And you are right about the IsConnected() call. Thanks! I changed the code and added more comments. (I decided to do this purely based on what I read from MSDN and POSIX documents. Please take a close look and let me know if it doesn't make sense.) https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:417: // TODO(ajwong): Is setting read_callback_ the right thing to do here?? On 2013/09/06 21:55:42, wtc wrote: > > Remove this TODO comment. It is correct to use read_callback_ for connect or > read. This is documented in tcp_socket_win.h. Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:548: int TCPSocketWin::SetDefaultOptionsForClient() { On 2013/09/06 21:55:42, wtc wrote: > > 1. Please copy (and update as appropriate) the original comment: > > // Sets socket parameters. Returns the OS error code (or 0 on > // success). Updated comments in .h file. > > 2. IMPORTANT: This function is also suitable for an accepted socket > returned by a listening server socket. So perhaps we should not have "ForClient" > in the function's name. But these options are not needed for server sockets, right? The "accepted" sockets are still client sockets. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:630: socket_ = INVALID_SOCKET; On 2013/09/06 21:55:42, wtc wrote: > > I think most of the following code (except the accept_xxx code) should be moved > inside this "if (socket_ != INVALID_SOCKET)" block. That's what the original > code in tcp_client_socket_win.cc is like. I was thinking that it is more clear this way that all state (except logging) is reset by Close(). Do you concern about the performance? https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:643: } On 2013/09/06 21:55:42, wtc wrote: > > This code is not in the original code. Is that a bug? This is intended. I would like this Close() to reset all internal state except logging-related things. So that we can do: socket.Create(); socket.Connect(); socket.Close(); // |socket| is reset completely so that we can reuse it. socket.Create(); ... https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:663: write_callback_.Reset(); Please see my comments above. On 2013/09/06 21:55:42, wtc wrote: > > Are these two lines necessary? They are not in the original > tcp_client_socket_win.cc code. Perhaps that was a bug? https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:802: void TCPSocketWin::LogConnectStart(const AddressList& addresses) { I agree that this is some kind of layer violation. The best way is to re-define the logging format for connect. I wish that could be done in a separate CL later. I will try to talk with you in person and listen to your suggestions. On 2013/09/06 21:55:42, wtc wrote: > > I think this function and LogConnectCompletion belong in TCPClientSocketWin. > It is confusing how to use the two LogXXX functions. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win.h File net/socket/tcp_socket_win.h (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.h:33: int Create(AddressFamily family); I slightly prefer Create(). The document of OS API socket() uses "create a socket" instead of "open". What do you think? On 2013/09/06 21:55:42, wtc wrote: > > "Open" is a better name. "Create" is usually a method that creates an > object and returns it. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.h:48: int GetPeerAddress(IPEndPoint* address) const; On 2013/09/06 21:55:42, wtc wrote: > > I suggest declaring GetPeerAddress next to the related GetLocalAddress function. Done. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.h:66: bool IsValid() const { return socket_ != INVALID_SOCKET; } Please see my comment about Create(). Thanks! On 2013/09/06 21:55:42, wtc wrote: > > You can consider renaming this method "IsOpen". https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.h:70: // TCPClientSocket may attempt to connect to multiple addresses until it I did really mean TCPClientSocket. I agree that it seems awkward. I would like to talk with you in person and listen to your suggestions. On 2013/09/06 21:55:42, wtc wrote: > > TCPClientSocket => TCPSocket > > or did you really mean TCPClientSocket? > > Do the StartLoggingMultipleConnectAttempts and EndLoggingMultipleConnectAttempts > methods belong in this class? https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.h:71: // succeeds in establishing a connection. The corresponding log will be On 2013/09/06 21:55:42, wtc wrote: > > Nit: will be => will have ? Done.
Hi, Wan-Teh. I have made changes according to our discussion. Please take another look. Thanks!
On 2013/09/10 00:52:23, yzshen1 wrote: > Hi, Wan-Teh. > > I have made changes according to our discussion. Please take another look. > > Thanks! Friendly ping, Wan-Teh and Fred. If possible, I wish the refactoring could be landed before M31 branching(1.5 week from now), because some Pepper API changes are based on this and we have promised to ship in this milestone. Thanks!
Patch set 6 LGTM. I didn't review TCPSocketWin::Close() carefully. I will review it again tomorrow. Everything else looks good. Some comments and suggested changes below. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... File net/socket/tcp_client_socket.cc (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:362: result = socket_->SetDefaultOptionsForClient(); Another idea is to just call SetDefaultOptionsForClient() for all sockets, including listening sockets. I think the options set by SetDefaultOptionsForClient() should be harmless to listening sockets. In any case, your current design is fine by me. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win.h File net/socket/tcp_socket_win.h (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.h:33: int Create(AddressFamily family); On 2013/09/09 23:14:48, yzshen1 wrote: > I slightly prefer Create(). The document of OS API socket() uses "create a > socket" instead of "open". What do you think? The socket() function is more similar to a static Create method: class TCPSocketWin { public: static TCPSocketWin* Create(AddressFamily family); ... But the function you specified here is invoked on an existing TCPSocketWin object, so "Create" sounds strange. Also, "Open" would match the "Close" method, which performs the reverse operation. https://codereview.chromium.org/23881002/diff/93001/net/socket/tcp_socket_win.cc File net/socket/tcp_socket_win.cc (right): https://codereview.chromium.org/23881002/diff/93001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:536: return ERR_FAILED; Can you find a better error code? https://codereview.chromium.org/23881002/diff/93001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:902: base::ResetAndReturn(&read_callback_).Run(result); Seems better to put DCHECK_NE(result, ERR_IO_PENDING); immediately before this line. https://codereview.chromium.org/23881002/diff/93001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:940: base::ResetAndReturn(&write_callback_).Run(rv); Seems better to put DCHECK_NE(rv, ERR_IO_PENDING); immediately before this line. https://codereview.chromium.org/23881002/diff/93001/net/socket/tcp_socket_win.h File net/socket/tcp_socket_win.h (right): https://codereview.chromium.org/23881002/diff/93001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.h:60: // The commonly used options for client sockets (including accepted sockets): This should say "client sockets and accepted sockets". https://codereview.chromium.org/23881002/diff/93001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.h:84: // TODO(yzshen): Change logging format and let TCPClientSocket log start/end Nit: start/end => the start/end
Thanks, Wan-Teh. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... File net/socket/tcp_client_socket.cc (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:362: result = socket_->SetDefaultOptionsForClient(); On 2013/09/12 01:27:16, wtc wrote: > > Another idea is to just call SetDefaultOptionsForClient() > for all sockets, including listening sockets. I think > the options set by SetDefaultOptionsForClient() should be > harmless to listening sockets. > > In any case, your current design is fine by me. Thanks! I will leave it as it is. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win.h File net/socket/tcp_socket_win.h (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.h:33: int Create(AddressFamily family); Done. I have changed the name to Open. On 2013/09/12 01:27:16, wtc wrote: > > On 2013/09/09 23:14:48, yzshen1 wrote: > > I slightly prefer Create(). The document of OS API socket() uses "create a > > socket" instead of "open". What do you think? > > The socket() function is more similar to a static Create > method: > > class TCPSocketWin { > public: > static TCPSocketWin* Create(AddressFamily family); > ... > > But the function you specified here is invoked on an > existing TCPSocketWin object, so "Create" sounds strange. > Also, "Open" would match the "Close" method, which performs > the reverse operation. https://codereview.chromium.org/23881002/diff/93001/net/socket/tcp_socket_win.cc File net/socket/tcp_socket_win.cc (right): https://codereview.chromium.org/23881002/diff/93001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:536: return ERR_FAILED; Done. I changed it to ERR_ADDRESS_INVALID. On 2013/09/12 01:27:17, wtc wrote: > > Can you find a better error code? https://codereview.chromium.org/23881002/diff/93001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:902: base::ResetAndReturn(&read_callback_).Run(result); On 2013/09/12 01:27:17, wtc wrote: > > Seems better to put DCHECK_NE(result, ERR_IO_PENDING); > immediately before this line. Done. https://codereview.chromium.org/23881002/diff/93001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.cc:940: base::ResetAndReturn(&write_callback_).Run(rv); On 2013/09/12 01:27:17, wtc wrote: > > Seems better to put DCHECK_NE(rv, ERR_IO_PENDING); > immediately before this line. Done. https://codereview.chromium.org/23881002/diff/93001/net/socket/tcp_socket_win.h File net/socket/tcp_socket_win.h (right): https://codereview.chromium.org/23881002/diff/93001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.h:60: // The commonly used options for client sockets (including accepted sockets): On 2013/09/12 01:27:17, wtc wrote: > > This should say "client sockets and accepted sockets". Done. https://codereview.chromium.org/23881002/diff/93001/net/socket/tcp_socket_win... net/socket/tcp_socket_win.h:84: // TODO(yzshen): Change logging format and let TCPClientSocket log start/end On 2013/09/12 01:27:17, wtc wrote: > > Nit: start/end => the start/end Done.
Patch set 8 LGTM. I also reviewed TCPSocketWin::Close(). I suggest two changes below to keep the creation and destruction of socket_ and core_ in sync. https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_socket_wi... File net/socket/tcp_socket_win.cc (right): https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_socket_wi... net/socket/tcp_socket_win.cc:297: } I think we should move the creation of Core (core_ = new Core(this);) from DoConnect() to this function, so that whenever we set socket_, we also set core_. https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_socket_wi... net/socket/tcp_socket_win.cc:661: core_ = NULL; I suggest moving this block of code (lines 652-661) righjt after line 636, so that whenever we close socket_, we destroy core_. https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_socket_wi... net/socket/tcp_socket_win.cc:669: write_callback_.Reset(); It is a little confusing that we seem to clean up read_callback_ and write_callback_ in a different way from how we clean up accept_callback_. The StopWatching() calls for read_watcher_ and write_watcher_ are actually done when we destroy core_ (line 661). I am also wondering if we should move accept_watcher_ to the Core object.
https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_socket_wi... File net/socket/tcp_socket_win.cc (right): https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_socket_wi... net/socket/tcp_socket_win.cc:297: } Then we will need an additional boolean flag to keep track of whether Connect has been called. (Please see comment on line 392.) On 2013/09/12 21:31:07, wtc wrote: > > I think we should move the creation of Core (core_ = new Core(this);) > from DoConnect() to this function, so that whenever we set socket_, > we also set core_. https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_socket_wi... net/socket/tcp_socket_win.cc:661: core_ = NULL; (I am happy to make the change, just try to make sure I understand it correctly.) If we decide not to move core_ creation to Open(), do you still want to move this code? On 2013/09/12 21:31:07, wtc wrote: > > I suggest moving this block of code (lines 652-661) righjt after line 636, > so that whenever we close socket_, we destroy core_. https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_socket_wi... net/socket/tcp_socket_win.cc:669: write_callback_.Reset(); Yeah, I think we could do more refactoring to move accept into Core. But I wish I could do it in a separate CL, if you think that is okay. This CL is already quite big. On 2013/09/12 21:31:07, wtc wrote: > > It is a little confusing that we seem to clean up read_callback_ and > write_callback_ in a different way from how we clean up accept_callback_. > > The StopWatching() calls for read_watcher_ and write_watcher_ are actually > done when we destroy core_ (line 661). > > I am also wondering if we should move accept_watcher_ to the Core object.
LGTM after nits below I just took a quick pass over; looks like wtc@ did a detailed review, so I'll just defer to him. https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... File net/socket/tcp_client_socket.cc (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:91: PLOG(ERROR) << "TCPSocket::SetDefaultOptionsForClient() returned an error"; On 2013/09/06 00:57:44, yzshen1 wrote: > I think this is not a fatal issue, maybe we can swallow it? > Actually SetDefaultOptionsForClient() never returns failure. It behaves the same > as the previous code: > https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/tcp_cli... > > One minor annoyance about two-phase init is that either (1) I need to set > |socket_| to NULL before Init(connected_socket). In this case, I need to check > NULL for |socket_| in other places; or (2) I create a |socket_| and later in > Init(connected_socket) replace it with |connected_socket|. In this case, two > TCPSocket are created, and therefore we log two NetLog::TYPE_SOCKET_ALIVE > events. > > Comparing with two-phase init, it seems easier to have a static Create() methods > which returns a valid TCPSocket pointer on success. What do you think? > > On 2013/09/05 22:52:28, akalin wrote: > > this is unfortunate. Do we really want to swallow this error? Perhaps we need > > two-phase init? > Seems like a good reason to just make SetDefaultOptionsforClient() not return anything. https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_client_so... File net/socket/tcp_client_socket.cc (right): https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_client_so... net/socket/tcp_client_socket.cc:8: #include "base/file_util.h" #include "base/logging.h", too https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_client_so... File net/socket/tcp_client_socket.h (right): https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_client_so... net/socket/tcp_client_socket.h:64: virtual int Connect(const CompletionCallback& callback); can you mark these functions as OVERRIDE here? (I know you do it in your other CL)
Thanks, Fred! https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... File net/socket/tcp_client_socket.cc (right): https://codereview.chromium.org/23881002/diff/17001/net/socket/tcp_client_soc... net/socket/tcp_client_socket.cc:91: PLOG(ERROR) << "TCPSocket::SetDefaultOptionsForClient() returned an error"; On 2013/09/12 23:21:37, akalin wrote: > On 2013/09/06 00:57:44, yzshen1 wrote: > > I think this is not a fatal issue, maybe we can swallow it? > > Actually SetDefaultOptionsForClient() never returns failure. It behaves the > same > > as the previous code: > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/tcp_cli... > > > > One minor annoyance about two-phase init is that either (1) I need to set > > |socket_| to NULL before Init(connected_socket). In this case, I need to check > > NULL for |socket_| in other places; or (2) I create a |socket_| and later in > > Init(connected_socket) replace it with |connected_socket|. In this case, two > > TCPSocket are created, and therefore we log two NetLog::TYPE_SOCKET_ALIVE > > events. > > > > Comparing with two-phase init, it seems easier to have a static Create() > methods > > which returns a valid TCPSocket pointer on success. What do you think? > > > > On 2013/09/05 22:52:28, akalin wrote: > > > this is unfortunate. Do we really want to swallow this error? Perhaps we > need > > > two-phase init? > > > > Seems like a good reason to just make SetDefaultOptionsforClient() not return > anything. Done. https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_client_so... File net/socket/tcp_client_socket.cc (right): https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_client_so... net/socket/tcp_client_socket.cc:8: #include "base/file_util.h" On 2013/09/12 23:21:37, akalin wrote: > #include "base/logging.h", too Done. https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_client_so... File net/socket/tcp_client_socket.h (right): https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_client_so... net/socket/tcp_client_socket.h:64: virtual int Connect(const CompletionCallback& callback); On 2013/09/12 23:21:37, akalin wrote: > can you mark these functions as OVERRIDE here? (I know you do it in your other > CL) Done.
Hi, Wan-Teh. https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_socket_wi... File net/socket/tcp_socket_win.cc (right): https://codereview.chromium.org/23881002/diff/117001/net/socket/tcp_socket_wi... net/socket/tcp_socket_win.cc:297: } I realized that we could still use |peer_address_| to keep track of whether Connect() has been called. However, I still prefer to lazily create |core_|: if TCPSocket is used as server, there is no need to have a |core_| object. If later we let |core_| also watch Accept, then I will move |core_| creation into Open(). What do you think? On 2013/09/12 21:45:10, yzshen1 wrote: > Then we will need an additional boolean flag to keep track of whether Connect > has been called. > (Please see comment on line 392.) > > On 2013/09/12 21:31:07, wtc wrote: > > > > I think we should move the creation of Core (core_ = new Core(this);) > > from DoConnect() to this function, so that whenever we set socket_, > > we also set core_. >
Patch set 10 LGTM. https://codereview.chromium.org/23881002/diff/139001/net/socket/tcp_socket_wi... File net/socket/tcp_socket_win.cc (right): https://codereview.chromium.org/23881002/diff/139001/net/socket/tcp_socket_wi... net/socket/tcp_socket_win.cc:620: void TCPSocketWin::Close() { The property I want from Close() is that members that are created at the same time should be destroyed together. You know which members are created at the same time much better than I do, so I trust your judgement. Also, ideally Close() should detect that Close() has already been called and return an error (which is a form of a double-free error). I know you use if statements to allow the second Close() to skip work. You can consider making this change in a future CL. https://codereview.chromium.org/23881002/diff/139001/net/socket/tcp_socket_wi... net/socket/tcp_socket_win.cc:701: int new_socket = accept(socket_, storage.addr, &storage.addr_len); This shows our listening sockets are calling accept() in non-blocking mode rather than async mode. This means they don't need to use a Core object. The client sockets and accepted sockets do Read() and Write() in async mode, so they need a Core object. https://codereview.chromium.org/23881002/diff/139001/net/socket/tcp_socket_wi... net/socket/tcp_socket_win.cc:755: core_ = new Core(this); It is fine to create core_ lazily here. I forgot that a listening socket doesn't need Core. https://codereview.chromium.org/23881002/diff/139001/net/socket/tcp_socket_win.h File net/socket/tcp_socket_win.h (right): https://codereview.chromium.org/23881002/diff/139001/net/socket/tcp_socket_wi... net/socket/tcp_socket_win.h:136: scoped_ptr<IPEndPoint> peer_address_; It is worth removing this member and just calling getpeername in a future CL.
https://codereview.chromium.org/23881002/diff/139001/net/socket/tcp_socket_wi... File net/socket/tcp_socket_win.cc (right): https://codereview.chromium.org/23881002/diff/139001/net/socket/tcp_socket_wi... net/socket/tcp_socket_win.cc:620: void TCPSocketWin::Close() { On 2013/09/13 16:33:05, wtc wrote: > > The property I want from Close() is that members that are created at the same > time should be destroyed together. You know which members are created at the > same time much better than I do, so I trust your judgement. > > Also, ideally Close() should detect that Close() has already been called and > return an error (which is a form of a double-free error). I know you use if > statements to allow the second Close() to skip work. You can consider making > this change in a future CL. I tried to make it simple: Close() is always safe to call. For example, the destructor calls it without detecting whether it is really necessary. Is performance your main concern? IMO the performance impact is negligible. https://codereview.chromium.org/23881002/diff/139001/net/socket/tcp_socket_wi... net/socket/tcp_socket_win.cc:701: int new_socket = accept(socket_, storage.addr, &storage.addr_len); Ah, I didn't realize that. Thanks for telling me. :) On 2013/09/13 16:33:05, wtc wrote: > > This shows our listening sockets are calling accept() in non-blocking mode > rather than async mode. This means they don't need to use a Core object. > > The client sockets and accepted sockets do Read() and Write() in async mode, > so they need a Core object. https://codereview.chromium.org/23881002/diff/139001/net/socket/tcp_socket_wi... net/socket/tcp_socket_win.cc:755: core_ = new Core(this); On 2013/09/13 16:33:05, wtc wrote: > > It is fine to create core_ lazily here. I forgot that a listening socket > doesn't need Core. Done. https://codereview.chromium.org/23881002/diff/139001/net/socket/tcp_socket_win.h File net/socket/tcp_socket_win.h (right): https://codereview.chromium.org/23881002/diff/139001/net/socket/tcp_socket_wi... net/socket/tcp_socket_win.h:136: scoped_ptr<IPEndPoint> peer_address_; A few nice things about keeping this member: - The libevent implementation needs it anyway, because of the fast open mode. - It also indicates whether Connect() has been called. (I think it is a better indicator than |core_|.) But I don't have a strong opinion. I could remove it if you prefer so. Thanks! On 2013/09/13 16:33:05, wtc wrote: > > It is worth removing this member and just calling getpeername > in a future CL.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yzshen@chromium.org/23881002/139001
Message was sent while issue was closed.
Change committed as 223286 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
