Windows only: Move client socket functionality from TCPClientSocket into TCPSocket.

net/socket/tcp_client_socket.cc

Index: net/socket/tcp_client_socket.cc
diff --git a/net/socket/tcp_client_socket.cc b/net/socket/tcp_client_socket.cc
index a91d33d24c8cb453f9f314e2b980a37d981d7a62..294248466a5ae430e702186b998cc90334b4a5dc 100644
--- a/net/socket/tcp_client_socket.cc
+++ b/net/socket/tcp_client_socket.cc
@@ -7,6 +7,15 @@
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 
+#if defined(OS_WIN)

[wtc, 2013/09/06 21:55:42]

No need to add #if defined(OS_WIN) because these headers will also be used by
OS_POSIX.

[yzshen1, 2013/09/09 23:14:48]

Done.

+
+#include "net/base/io_buffer.h"
+#include "net/base/ip_endpoint.h"
+#include "net/base/net_errors.h"
+#include "net/base/net_util.h"
+
+#endif
+
 namespace net {
 
 namespace {
@@ -56,4 +65,307 @@ bool IsTCPFastOpenEnabled() {
   return g_tcp_fastopen_enabled;
 }
 
+#if defined(OS_WIN)
+
+TCPClientSocket::TCPClientSocket(const AddressList& addresses,
+                                 net::NetLog* net_log,
+                                 const net::NetLog::Source& source)
+    : socket_(new TCPSocket(net_log, source)),
+      addresses_(addresses),
+      current_address_index_(-1),
+      next_connect_state_(CONNECT_STATE_NONE),
+      previously_disconnected_(false) {
+}
+
+TCPClientSocket::TCPClientSocket(scoped_ptr<TCPSocket> connected_socket,
+                                 const IPEndPoint& peer_address)
+    : socket_(connected_socket.Pass()),
+      addresses_(AddressList(peer_address)),
+      current_address_index_(0),
+      next_connect_state_(CONNECT_STATE_NONE),
+      previously_disconnected_(false) {
+  DCHECK(socket_.get());

[akalin, 2013/09/05 22:52:28]

no need for .get()

[yzshen1, 2013/09/06 00:57:44]

Done.

+
+  int result = socket_->SetDefaultOptionsForClient();

[wtc, 2013/09/06 21:55:42]

This should have been called when the passed-in TCPSocket was created.
See my comment about SetDefaultOptionsForClient in tcp_socket.cc.

[yzshen1, 2013/09/09 23:14:48]

Please see my comments below at line 362.


On 2013/09/06 21:55:42, wtc wrote:

+  if (result != OK)
+    PLOG(ERROR) << "TCPSocket::SetDefaultOptionsForClient() returned an error";

[akalin, 2013/09/05 22:52:28]

this is unfortunate. Do we really want to swallow this error? Perhaps we need
two-phase init?

[yzshen1, 2013/09/06 00:57:44]

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:

[akalin, 2013/09/12 23:21:37]

Seems like a good reason to just make SetDefaultOptionsforClient() not return
anything.

[yzshen1, 2013/09/13 05:07:51]

Done.

+
+  use_history_.set_was_ever_connected();
+}
+
+TCPClientSocket::~TCPClientSocket() {
+}
+
+int TCPClientSocket::Bind(const IPEndPoint& address) {
+  if (current_address_index_ >= 0 || bind_address_.get()) {

[akalin, 2013/09/05 22:52:28]

no .get()

[yzshen1, 2013/09/06 00:57:44]

Done.

+    // Cannot bind the socket if we are already connected or connecting.
+    return ERR_UNEXPECTED;

[akalin, 2013/09/05 22:52:28]

ERR_UNEXPECTED is usually found with NOTREACHED()

[yzshen1, 2013/09/06 00:57:44]

Done.

+  }
+
+  int result = OK;
+  if (!socket_->IsValid()) {
+    result = CreateSocket(address.GetFamily());
+    if (result != OK)
+      return result;
+  }
+
+  result = socket_->Bind(address);
+  if (result != OK)
+    return result;
+
+  bind_address_.reset(new IPEndPoint(address));
+  return OK;
+}
+
+int TCPClientSocket::Connect(const CompletionCallback& callback) {
+  DCHECK(!callback.is_null());
+
+  // If connecting or already connected, then just return OK.
+  if (socket_->IsValid() && current_address_index_ >= 0)
+    return OK;
+
+  socket_->StartLoggingMultipleConnectAttempts(addresses_);
+
+  // We will try to connect to each address in addresses_. Start with the
+  // first one in the list.
+  next_connect_state_ = CONNECT_STATE_CONNECT;
+  current_address_index_ = 0;
+
+  int rv = DoConnectLoop(OK);
+  if (rv == ERR_IO_PENDING) {
+    connect_callback_ = callback;
+  } else {
+    socket_->EndLoggingMultipleConnectAttempts(rv);
+  }
+
+  return rv;
+}
+
+int TCPClientSocket::DoConnectLoop(int result) {
+  DCHECK_NE(next_connect_state_, CONNECT_STATE_NONE);
+
+  int rv = result;
+  do {
+    ConnectState state = next_connect_state_;
+    next_connect_state_ = CONNECT_STATE_NONE;
+    switch (state) {
+      case CONNECT_STATE_CONNECT:
+        DCHECK_EQ(OK, rv);
+        rv = DoConnect();
+        break;
+      case CONNECT_STATE_CONNECT_COMPLETE:
+        rv = DoConnectComplete(rv);
+        break;
+      default:
+        LOG(DFATAL) << "bad state " << state;

[akalin, 2013/09/05 22:52:28]

NOTREACHED() instead of LOG(DFATAL)? (They're equivalent for NOTREACHED() seems
clearer)

[yzshen1, 2013/09/06 00:57:44]

Done.

+        rv = ERR_UNEXPECTED;
+        break;
+    }
+  } while (rv != ERR_IO_PENDING && next_connect_state_ != CONNECT_STATE_NONE);
+
+  return rv;
+}
+
+int TCPClientSocket::DoConnect() {
+  DCHECK_GE(current_address_index_, 0);
+  DCHECK_LT(current_address_index_, static_cast<int>(addresses_.size()));
+
+  const IPEndPoint& endpoint = addresses_[current_address_index_];
+
+  if (previously_disconnected_) {
+    use_history_.Reset();
+    previously_disconnected_ = false;
+  }
+
+  next_connect_state_ = CONNECT_STATE_CONNECT_COMPLETE;
+
+  if (socket_->IsValid()) {
+    DCHECK(bind_address_.get());

[akalin, 2013/09/05 22:52:28]

no .get() (also everywhere else)

[yzshen1, 2013/09/06 00:57:44]

Done. Also changed other places.

+  } else {
+    int result = CreateSocket(endpoint.GetFamily());
+    if (result != OK)
+      return result;
+
+    if (bind_address_.get()) {
+      result = socket_->Bind(*bind_address_);
+      if (result != OK) {
+        socket_->Close();
+        return result;
+      }
+    }
+  }
+
+  return socket_->Connect(endpoint,
+                          base::Bind(&TCPClientSocket::DidCompleteConnect,
+                                     base::Unretained(this)));
+}
+
+int TCPClientSocket::DoConnectComplete(int result) {
+  if (result == OK) {
+    use_history_.set_was_ever_connected();
+    return OK;  // Done!
+  }
+
+  // Close whatever partially connected socket we currently have.
+  DoDisconnect();
+
+  // Try to fall back to the next address in the list.
+  if (current_address_index_ + 1 < static_cast<int>(addresses_.size())) {
+    next_connect_state_ = CONNECT_STATE_CONNECT;
+    ++current_address_index_;
+    return OK;
+  }
+
+  // Otherwise there is nothing to fall back to, so give up.
+  return result;
+}
+
+void TCPClientSocket::Disconnect() {
+  DoDisconnect();
+  current_address_index_ = -1;
+  bind_address_.reset();
+}
+
+void TCPClientSocket::DoDisconnect() {
+  // If connecting or already connected, record that the socket has been
+  // disconnected.
+  previously_disconnected_ = socket_->IsValid() && current_address_index_ >= 0;
+  socket_->Close();
+}
+
+bool TCPClientSocket::IsConnected() const {
+  return socket_->IsConnected();
+}
+
+bool TCPClientSocket::IsConnectedAndIdle() const {
+  return socket_->IsConnectedAndIdle();
+}
+
+int TCPClientSocket::GetPeerAddress(IPEndPoint* address) const {
+  return socket_->GetPeerAddress(address);
+}
+
+int TCPClientSocket::GetLocalAddress(IPEndPoint* address) const {
+  DCHECK(address);
+
+  if (!socket_->IsValid()) {
+    if (bind_address_.get()) {
+      *address = *bind_address_;
+      return OK;
+    }
+    return ERR_SOCKET_NOT_CONNECTED;
+  }
+
+  return socket_->GetLocalAddress(address);
+}
+
+void TCPClientSocket::SetSubresourceSpeculation() {
+  use_history_.set_subresource_speculation();
+}
+
+void TCPClientSocket::SetOmniboxSpeculation() {
+  use_history_.set_omnibox_speculation();
+}
+
+bool TCPClientSocket::WasEverUsed() const {
+  return use_history_.was_used_to_convey_data();
+}
+
+bool TCPClientSocket::UsingTCPFastOpen() const {
+  return socket_->UsingTCPFastOpen();
+}
+
+bool TCPClientSocket::WasNpnNegotiated() const {
+  return false;
+}
+
+NextProto TCPClientSocket::GetNegotiatedProtocol() const {
+  return kProtoUnknown;
+}
+
+bool TCPClientSocket::GetSSLInfo(SSLInfo* ssl_info) {
+  return false;
+}
+
+int TCPClientSocket::Read(IOBuffer* buf,
+                          int buf_len,
+                          const CompletionCallback& callback) {
+  DCHECK(!callback.is_null());
+
+  CompletionCallback read_callback = base::Bind(
+      &TCPClientSocket::DidCompleteReadWrite, base::Unretained(this), callback);

[akalin, 2013/09/05 22:52:28]

comment as to why Unretained() can be used safely here

[yzshen1, 2013/09/06 00:57:44]

Done.

+  int result = socket_->Read(buf, buf_len, read_callback);
+  if (result > 0)
+    use_history_.set_was_used_to_convey_data();
+
+  return result;
+}
+
+int TCPClientSocket::Write(IOBuffer* buf,
+                           int buf_len,
+                           const CompletionCallback& callback) {
+  DCHECK(!callback.is_null());
+
+  CompletionCallback write_callback = base::Bind(
+      &TCPClientSocket::DidCompleteReadWrite, base::Unretained(this), callback);
+  int result = socket_->Write(buf, buf_len, write_callback);
+  if (result > 0)
+    use_history_.set_was_used_to_convey_data();
+
+  return result;
+}
+
+bool TCPClientSocket::SetReceiveBufferSize(int32 size) {
+  return socket_->SetReceiveBufferSize(size);
+}
+
+bool TCPClientSocket::SetSendBufferSize(int32 size) {
+    return socket_->SetSendBufferSize(size);
+}
+
+bool TCPClientSocket::SetKeepAlive(bool enable, int delay) {
+  return socket_->SetKeepAlive(enable, delay);
+}
+
+bool TCPClientSocket::SetNoDelay(bool no_delay) {
+  return socket_->SetNoDelay(no_delay);
+}
+
+void TCPClientSocket::DidCompleteConnect(int result) {
+  DCHECK_EQ(next_connect_state_, CONNECT_STATE_CONNECT_COMPLETE);

[wtc, 2013/09/06 21:55:42]

You can also DCHECK these:

  DCHECK_NE(result, ERR_IO_PENDING);
  DCHECK(!connect_callback_.is_null());

[yzshen1, 2013/09/09 23:14:48]

Done.

+
+  result = DoConnectLoop(result);
+  if (result != ERR_IO_PENDING) {
+    socket_->EndLoggingMultipleConnectAttempts(result);
+    CompletionCallback callback = connect_callback_;
+    connect_callback_.Reset();
+    callback.Run(result);

[wtc, 2013/09/06 21:55:42]

I remember there is a new CompletionCallback method (seems to be called
ResetAndRun) that does these three lines in one shot.

[yzshen1, 2013/09/09 23:14:48]

Done.

+  }
+}
+
+void TCPClientSocket::DidCompleteReadWrite(
+    const CompletionCallback& forward_callback,

[wtc, 2013/09/06 21:55:42]

Why is this argument named "forward_callback"? Why not simply "callback"?

[yzshen1, 2013/09/09 23:14:48]

Done.

+    int result) {
+  if (result > 0)
+    use_history_.set_was_used_to_convey_data();
+
+  forward_callback.Run(result);
+}
+
+int TCPClientSocket::CreateSocket(AddressFamily family) {
+  DCHECK(!socket_->IsValid());
+
+  int result = socket_->Create(family);
+  if (result != OK)
+    return result;
+
+  result = socket_->SetDefaultOptionsForClient();

[wtc, 2013/09/06 21:55:42]

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).

[yzshen1, 2013/09/09 23:14:48]

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:

[wtc, 2013/09/12 01:27:16]

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.

[yzshen1, 2013/09/12 20:07:50]

Thanks! I will leave it as it is.

+  if (result != OK)
+    socket_->Close();
+
+  return result;
+}
+
+#endif
+
 }  // namespace net