Use TCPServerSocket instead of TCPListenSocket in transport_client_socket_unittest.cc

net/socket/transport_client_socket_unittest.cc

Index: net/socket/transport_client_socket_unittest.cc
diff --git a/net/socket/transport_client_socket_unittest.cc b/net/socket/transport_client_socket_unittest.cc
index 0e2f11d06d5bd5663311f2badbb24279742b961a..e95f6540f177e56dad64d2ed605a95b2e1ee2209 100644
--- a/net/socket/transport_client_socket_unittest.cc
+++ b/net/socket/transport_client_socket_unittest.cc
@@ -5,8 +5,10 @@
 #include "net/socket/tcp_client_socket.h"
 
 #include "base/basictypes.h"
+#include "base/bind.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/run_loop.h"
 #include "net/base/address_list.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
@@ -16,7 +18,7 @@
 #include "net/log/net_log.h"
 #include "net/log/net_log_unittest.h"
 #include "net/socket/client_socket_factory.h"
-#include "net/socket/tcp_listen_socket.h"
+#include "net/socket/tcp_server_socket.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 
@@ -34,8 +36,7 @@ enum ClientSocketTestTypes {
 }  // namespace
 
 class TransportClientSocketTest
-    : public StreamListenSocket::Delegate,
-      public ::testing::TestWithParam<ClientSocketTestTypes> {
+    : public ::testing::TestWithParam<ClientSocketTestTypes> {
  public:
   TransportClientSocketTest()
       : listen_port_(0),
@@ -46,21 +47,6 @@ class TransportClientSocketTest
   virtual ~TransportClientSocketTest() {
   }
 
-  // Implement StreamListenSocket::Delegate methods
-  void DidAccept(StreamListenSocket* server,
-                 scoped_ptr<StreamListenSocket> connection) override {
-    connected_sock_.reset(
-        static_cast<TCPListenSocket*>(connection.release()));
-  }
-  void DidRead(StreamListenSocket*, const char* str, int len) override {
-    // TODO(dkegel): this might not be long enough to tickle some bugs.
-    connected_sock_->Send(kServerReply, arraysize(kServerReply) - 1,
-                          false /* Don't append line feed */);
-    if (close_server_socket_on_next_send_)
-      CloseServerSocket();
-  }
-  void DidClose(StreamListenSocket* sock) override {}
-
   // Testcase hooks
   void SetUp() override;
 
@@ -69,12 +55,10 @@ class TransportClientSocketTest
     connected_sock_.reset();
   }
 
-  void PauseServerReads() {
-    connected_sock_->PauseReads();
-  }
-
-  void ResumeServerReads() {
-    connected_sock_->ResumeReads();
+  void AcceptCallback(int res) {
+    ASSERT_TRUE(loop_.running());

[mmenke, 2015/04/09 15:16:15]

I don't think we need this check - this basically requires we always call
loop_.Run before callback.GetResult, which seems like an unintuitive
requirement.

RunLoops handle Quit before Run fine (That is, Run will do nothing if the loop
has already been quit).

[xunjieli, 2015/04/21 19:50:24]

Done.

+    ASSERT_EQ(OK, res);
+    loop_.Quit();
   }
 
   int DrainClientSocket(IOBuffer* buf,
@@ -82,44 +66,43 @@ class TransportClientSocketTest
                         uint32 bytes_to_read,
                         TestCompletionCallback* callback);
 
-  void SendClientRequest();
+  void ProcessRequest();

[mmenke, 2015/04/09 15:16:15]

Think this is worth a comment (Maybe something like "Send request from client to
server socket, have the server read it, and send the response"?)

[mmenke, 2015/04/09 15:16:15]

Also should probably rename it.  SendRequestAndResponse?

[xunjieli, 2015/04/21 19:50:23]

Done.

[xunjieli, 2015/04/21 19:50:24]

Done.

+
+  // Make |connected_sock_| to read |expected_bytes_read| bytes.
+  void StartServerReads(int expected_bytes_read);
 
   void set_close_server_socket_on_next_send(bool close) {
     close_server_socket_on_next_send_ = close;
   }
 
  protected:
+  base::RunLoop loop_;
   uint16 listen_port_;
   CapturingNetLog net_log_;
   ClientSocketFactory* const socket_factory_;
   scoped_ptr<StreamSocket> sock_;
 
  private:
-  scoped_ptr<TCPListenSocket> listen_sock_;
-  scoped_ptr<TCPListenSocket> connected_sock_;
+  void SendServerResponse();
+
+  scoped_ptr<TCPServerSocket> listen_sock_;
+  scoped_ptr<StreamSocket> connected_sock_;
   bool close_server_socket_on_next_send_;
 };
 
 void TransportClientSocketTest::SetUp() {
   ::testing::TestWithParam<ClientSocketTestTypes>::SetUp();
 
-  // Find a free port to listen on
-  scoped_ptr<TCPListenSocket> sock;
-  uint16 port;
-  // Range of ports to listen on.  Shouldn't need to try many.
-  const uint16 kMinPort = 10100;
-  const uint16 kMaxPort = 10200;
-#if defined(OS_WIN)
-  EnsureWinsockInit();
-#endif
-  for (port = kMinPort; port < kMaxPort; port++) {
-    sock = TCPListenSocket::CreateAndListen("127.0.0.1", port, this);
-    if (sock.get())
-      break;
-  }
-  ASSERT_TRUE(sock.get() != NULL);
-  listen_sock_ = sock.Pass();
-  listen_port_ = port;
+  listen_sock_.reset(new TCPServerSocket(NULL, NetLog::Source()));
+  IPAddressNumber address;
+  ParseIPLiteralToNumber("127.0.0.1", &address);
+  IPEndPoint local_address(address, 0);
+  ASSERT_EQ(OK, listen_sock_->Listen(local_address, 1));
+  ASSERT_EQ(OK, listen_sock_->GetLocalAddress(&local_address));

[mmenke, 2015/04/09 15:16:15]

nit:  Think this is worth a comment or two, to make it clear why we need to get
the address we just set after listening.  Just something like:

Just above the "listen_sock_.reset" line (Or the listen line).
// Open a server socket on an ephemeral port.

And just above this line:

// Get the server's address (including actual port number).

[xunjieli, 2015/04/21 19:50:24]

Done.

+  listen_port_ = local_address.port();
+  listen_sock_->Accept(&connected_sock_,
+                       base::Bind(&TransportClientSocketTest::AcceptCallback,
+                                  base::Unretained(this)));
 
   AddressList addr;
   // MockHostResolver resolves everything to 127.0.0.1.
@@ -146,10 +129,7 @@ int TransportClientSocketTest::DrainClientSocket(
   while (bytes_read < bytes_to_read) {
     rv = sock_->Read(buf, buf_len, callback->callback());
     EXPECT_TRUE(rv >= 0 || rv == ERR_IO_PENDING);
-
-    if (rv == ERR_IO_PENDING)
-      rv = callback->WaitForResult();
-
+    rv = callback->GetResult(rv);
     EXPECT_GE(rv, 0);

[mmenke, 2015/04/09 15:16:15]

While you're here, this can actually be ASSERT_GT

[xunjieli, 2015/04/21 19:50:24]

Hmm, tried to use ASSERT_GT, but it complains that "cannot initialize return
object of type 'int' with an rvalue of type 'void". 

[mmenke, 2015/04/23 15:43:07]

Sorry, meant EXPECT_GT

     bytes_read += rv;
   }
@@ -157,21 +137,90 @@ int TransportClientSocketTest::DrainClientSocket(
   return static_cast<int>(bytes_read);
 }
 
-void TransportClientSocketTest::SendClientRequest() {
+void TransportClientSocketTest::ProcessRequest() {
+  // Send client request.
   const char request_text[] = "GET / HTTP/1.0\r\n\r\n";
-  scoped_refptr<IOBuffer> request_buffer(
-      new IOBuffer(arraysize(request_text) - 1));
-  TestCompletionCallback callback;
-  int rv;
+  int request_len = strlen(request_text);

[mmenke, 2015/04/09 15:16:15]

Sorry, I meant the header for strlen (Which is just <string>)

[xunjieli, 2015/04/21 19:50:24]

Done. Ah, I see! thanks!

+  scoped_refptr<DrainableIOBuffer> request_buffer(
+      new DrainableIOBuffer(new IOBuffer(request_len), request_len));
+  memcpy(request_buffer->data(), request_text, request_len);
+
+  int bytes_written = 0;
+  while (request_buffer->BytesRemaining() > 0) {
+    TestCompletionCallback write_callback;
+    int write_result =
+        sock_->Write(request_buffer.get(), request_buffer->BytesRemaining(),
+                     write_callback.callback());
+    write_result = write_callback.GetResult(write_result);
+    ASSERT_GE(write_result, 0);

[mmenke, 2015/04/09 15:16:15]

nit:  This can actually be ASSERT_GT

[xunjieli, 2015/04/21 19:50:23]

Done.

+    ASSERT_LE(bytes_written + write_result, request_len);
+    request_buffer->DidConsume(write_result);
+    bytes_written += write_result;
+  }
 
-  memcpy(request_buffer->data(), request_text, arraysize(request_text) - 1);
-  rv = sock_->Write(
-      request_buffer.get(), arraysize(request_text) - 1, callback.callback());
-  EXPECT_TRUE(rv >= 0 || rv == ERR_IO_PENDING);
+  // Confirms that the server receives what client sent.

[mmenke, 2015/04/09 15:16:15]

nit:  Confirm, to be consistent with other comments.

[xunjieli, 2015/04/21 19:50:24]

Done.

+  std::vector<char> buffer(request_len);
+  int bytes_read = 0;
+  while (bytes_read < request_len) {
+    scoped_refptr<IOBufferWithSize> read_buffer(
+        new IOBufferWithSize(request_len - bytes_read));
+    TestCompletionCallback read_callback;
+    int read_result = connected_sock_->Read(
+        read_buffer.get(), read_buffer->size(), read_callback.callback());
+    read_result = read_callback.GetResult(read_result);
+    ASSERT_GE(read_result, 0);
+    ASSERT_LE(bytes_read + read_result, request_len);
+    memmove(&buffer[bytes_read], read_buffer->data(), read_result);
+    bytes_read += read_result;
+  }
+
+  ASSERT_EQ(request_len, bytes_read);
+  std::string message(request_text);
+  std::string received_message(buffer.begin(), buffer.end());
+  ASSERT_EQ(message, received_message);

[mmenke, 2015/04/09 15:16:15]

nit:  Think you can just inline request_text - ASSERT_EQ is smart enough to
compare a string and a char*.

[xunjieli, 2015/04/21 19:50:24]

Done.

+
+  // Write server response.
+  SendServerResponse();
+}
 
-  if (rv == ERR_IO_PENDING)
-    rv = callback.WaitForResult();
-  EXPECT_EQ(rv, static_cast<int>(arraysize(request_text) - 1));
+void TransportClientSocketTest::SendServerResponse() {
+  // TODO(dkegel): this might not be long enough to tickle some bugs.
+  int reply_len = strlen(kServerReply);
+  scoped_refptr<DrainableIOBuffer> write_buffer(
+      new DrainableIOBuffer(new IOBuffer(reply_len), reply_len));
+  memcpy(write_buffer->data(), kServerReply, reply_len);
+  int bytes_written = 0;
+  while (write_buffer->BytesRemaining() > 0) {
+    TestCompletionCallback write_callback;
+    int write_result = connected_sock_->Write(write_buffer.get(),
+                                              write_buffer->BytesRemaining(),
+                                              write_callback.callback());
+    write_result = write_callback.GetResult(write_result);
+    ASSERT_GE(write_result, 0);
+    ASSERT_LE(bytes_written + write_result, reply_len);
+    write_buffer->DidConsume(write_result);
+    bytes_written += write_result;
+  }
+  if (close_server_socket_on_next_send_)
+    CloseServerSocket();
+}
+
+void TransportClientSocketTest::StartServerReads(int expected_bytes_read) {

[mmenke, 2015/04/09 15:16:15]

Suggestion:  Modify StartServerReads to just read expected_bytes_read bytes, and
return it as a string (Can then use it in 
TransportClientSocketTest::ProcessRequest).  Also don't have if call
SendServerResponse() - just have tests call it manually when needed.

Most tests should probably also check that connected_sock_ IsConnnectedAndIdle
after calling StartServerReads.

Also suggest renaming StartServerReads to ReadServerData or somesuch.

[xunjieli, 2015/04/21 19:50:24]

Done.

+  int bytes_read = 0;
+  scoped_refptr<IOBufferWithSize> read_buffer(
+      new IOBufferWithSize(expected_bytes_read));
+  while (bytes_read < expected_bytes_read) {
+    TestCompletionCallback read_callback;
+    int rv = connected_sock_->Read(read_buffer.get(),
+                                   expected_bytes_read - bytes_read,
+                                   read_callback.callback());
+    EXPECT_TRUE(rv >= 0 || rv == ERR_IO_PENDING);
+    rv = read_callback.GetResult(rv);
+    EXPECT_GE(rv, 0);
+    bytes_read += rv;
+  }
+  ASSERT_EQ(expected_bytes_read, bytes_read);
+  SendServerResponse();
 }
 
 // TODO(leighton):  Add SCTP to this list when it is ready.
@@ -184,6 +233,8 @@ TEST_P(TransportClientSocketTest, Connect) {
   EXPECT_FALSE(sock_->IsConnected());
 
   int rv = sock_->Connect(callback.callback());
+  loop_.Run();
+  EXPECT_EQ(OK, callback.GetResult(rv));

[mmenke, 2015/04/09 15:16:15]

Suggest leaving this where it was.  Serves largely to make sure the right netlog
events are emitted before and after connection.  Not exactly critical, but
moving it does somewhat change what the test is testing.

[xunjieli, 2015/04/21 19:50:24]

Done. I see. Makes sense. Thanks!

 
   net::CapturingNetLog::CapturedEntryList net_log_entries;
   net_log_.GetEntries(&net_log_entries);
@@ -191,11 +242,6 @@ TEST_P(TransportClientSocketTest, Connect) {
       net_log_entries, 0, net::NetLog::TYPE_SOCKET_ALIVE));
   EXPECT_TRUE(net::LogContainsBeginEvent(
       net_log_entries, 1, net::NetLog::TYPE_TCP_CONNECT));
-  if (rv != OK) {
-    ASSERT_EQ(rv, ERR_IO_PENDING);
-    rv = callback.WaitForResult();
-    EXPECT_EQ(rv, OK);
-  }
 
   EXPECT_TRUE(sock_->IsConnected());
   net_log_.GetEntries(&net_log_entries);
@@ -214,16 +260,14 @@ TEST_P(TransportClientSocketTest, IsConnected) {
   EXPECT_FALSE(sock_->IsConnected());
   EXPECT_FALSE(sock_->IsConnectedAndIdle());
   int rv = sock_->Connect(callback.callback());
-  if (rv != OK) {
-    ASSERT_EQ(rv, ERR_IO_PENDING);
-    rv = callback.WaitForResult();
-    EXPECT_EQ(rv, OK);
-  }
+  loop_.Run();
+  EXPECT_EQ(OK, callback.GetResult(rv));
+
   EXPECT_TRUE(sock_->IsConnected());
   EXPECT_TRUE(sock_->IsConnectedAndIdle());
 
   // Send the request and wait for the server to respond.
-  SendClientRequest();
+  ProcessRequest();
 
   // Drain a single byte so we know we've received some data.
   bytes_read = DrainClientSocket(buf.get(), 1, 1, &callback);
@@ -234,9 +278,9 @@ TEST_P(TransportClientSocketTest, IsConnected) {
   EXPECT_TRUE(sock_->IsConnected());
   EXPECT_FALSE(sock_->IsConnectedAndIdle());
 
-  bytes_read = DrainClientSocket(
-      buf.get(), 4096, arraysize(kServerReply) - 2, &callback);
-  ASSERT_EQ(bytes_read, arraysize(kServerReply) - 2);
+  bytes_read =
+      DrainClientSocket(buf.get(), 4096, strlen(kServerReply) - 1, &callback);
+  ASSERT_EQ(bytes_read, strlen(kServerReply) - 1);
 
   // After draining the data, the socket should be back to connected
   // and idle.
@@ -245,7 +289,7 @@ TEST_P(TransportClientSocketTest, IsConnected) {
 
   // This time close the server socket immediately after the server response.
   set_close_server_socket_on_next_send(true);
-  SendClientRequest();
+  ProcessRequest();
 
   bytes_read = DrainClientSocket(buf.get(), 1, 1, &callback);
   ASSERT_EQ(bytes_read, 1u);
@@ -254,9 +298,9 @@ TEST_P(TransportClientSocketTest, IsConnected) {
   EXPECT_TRUE(sock_->IsConnected());
   EXPECT_FALSE(sock_->IsConnectedAndIdle());
 
-  bytes_read = DrainClientSocket(
-      buf.get(), 4096, arraysize(kServerReply) - 2, &callback);
-  ASSERT_EQ(bytes_read, arraysize(kServerReply) - 2);
+  bytes_read =
+      DrainClientSocket(buf.get(), 4096, strlen(kServerReply) - 1, &callback);
+  ASSERT_EQ(bytes_read, strlen(kServerReply) - 1);
 
   // Once the data is drained, the socket should now be seen as not
   // connected.
@@ -273,18 +317,16 @@ TEST_P(TransportClientSocketTest, IsConnected) {
 TEST_P(TransportClientSocketTest, Read) {
   TestCompletionCallback callback;
   int rv = sock_->Connect(callback.callback());
-  if (rv != OK) {
-    ASSERT_EQ(rv, ERR_IO_PENDING);
+  loop_.Run();
+  EXPECT_EQ(OK, callback.GetResult(rv));
 
-    rv = callback.WaitForResult();
-    EXPECT_EQ(rv, OK);
-  }
-  SendClientRequest();
+  ProcessRequest();
 
   scoped_refptr<IOBuffer> buf(new IOBuffer(4096));
-  uint32 bytes_read = DrainClientSocket(
-      buf.get(), 4096, arraysize(kServerReply) - 1, &callback);
-  ASSERT_EQ(bytes_read, arraysize(kServerReply) - 1);
+  uint32 bytes_read =
+      DrainClientSocket(buf.get(), 4096, strlen(kServerReply), &callback);
+  ASSERT_EQ(bytes_read, strlen(kServerReply));
+  ASSERT_EQ(std::string(kServerReply), std::string(buf->data(), bytes_read));
 
   // All data has been read now.  Read once more to force an ERR_IO_PENDING, and
   // then close the server socket, and note the close.
@@ -298,22 +340,18 @@ TEST_P(TransportClientSocketTest, Read) {
 TEST_P(TransportClientSocketTest, Read_SmallChunks) {
   TestCompletionCallback callback;
   int rv = sock_->Connect(callback.callback());
-  if (rv != OK) {
-    ASSERT_EQ(rv, ERR_IO_PENDING);
+  loop_.Run();
+  EXPECT_EQ(OK, callback.GetResult(rv));
 
-    rv = callback.WaitForResult();
-    EXPECT_EQ(rv, OK);
-  }
-  SendClientRequest();
+  ProcessRequest();
 
   scoped_refptr<IOBuffer> buf(new IOBuffer(1));
   uint32 bytes_read = 0;
-  while (bytes_read < arraysize(kServerReply) - 1) {
+  while (bytes_read < strlen(kServerReply)) {
     rv = sock_->Read(buf.get(), 1, callback.callback());
     EXPECT_TRUE(rv >= 0 || rv == ERR_IO_PENDING);
 
-    if (rv == ERR_IO_PENDING)
-      rv = callback.WaitForResult();
+    rv = callback.GetResult(rv);
 
     ASSERT_EQ(1, rv);
     bytes_read += rv;
@@ -331,34 +369,26 @@ TEST_P(TransportClientSocketTest, Read_SmallChunks) {
 TEST_P(TransportClientSocketTest, Read_Interrupted) {
   TestCompletionCallback callback;
   int rv = sock_->Connect(callback.callback());
-  if (rv != OK) {
-    ASSERT_EQ(ERR_IO_PENDING, rv);
+  loop_.Run();
+  EXPECT_EQ(OK, callback.GetResult(rv));
 
-    rv = callback.WaitForResult();
-    EXPECT_EQ(rv, OK);
-  }
-  SendClientRequest();
+  ProcessRequest();
 
   // Do a partial read and then exit.  This test should not crash!
   scoped_refptr<IOBuffer> buf(new IOBuffer(16));
   rv = sock_->Read(buf.get(), 16, callback.callback());
   EXPECT_TRUE(rv >= 0 || rv == ERR_IO_PENDING);
 
-  if (rv == ERR_IO_PENDING)
-    rv = callback.WaitForResult();
+  rv = callback.GetResult(rv);
 
   EXPECT_NE(0, rv);
 }
 
-TEST_P(TransportClientSocketTest, DISABLED_FullDuplex_ReadFirst) {
+TEST_P(TransportClientSocketTest, FullDuplex_ReadFirst) {
   TestCompletionCallback callback;
   int rv = sock_->Connect(callback.callback());
-  if (rv != OK) {
-    ASSERT_EQ(rv, ERR_IO_PENDING);
-
-    rv = callback.WaitForResult();
-    EXPECT_EQ(rv, OK);
-  }
+  loop_.Run();
+  EXPECT_EQ(OK, callback.GetResult(rv));
 
   // Read first.  There's no data, so it should return ERR_IO_PENDING.
   const int kBufLen = 4096;
@@ -366,23 +396,23 @@ TEST_P(TransportClientSocketTest, DISABLED_FullDuplex_ReadFirst) {
   rv = sock_->Read(buf.get(), kBufLen, callback.callback());
   EXPECT_EQ(ERR_IO_PENDING, rv);
 
-  PauseServerReads();
   const int kWriteBufLen = 64 * 1024;
   scoped_refptr<IOBuffer> request_buffer(new IOBuffer(kWriteBufLen));
   char* request_data = request_buffer->data();
   memset(request_data, 'A', kWriteBufLen);
   TestCompletionCallback write_callback;
 
+  int bytes_written = 0;
   while (true) {
     rv = sock_->Write(
         request_buffer.get(), kWriteBufLen, write_callback.callback());
     ASSERT_TRUE(rv >= 0 || rv == ERR_IO_PENDING);
-
     if (rv == ERR_IO_PENDING) {
-      ResumeServerReads();
+      StartServerReads(bytes_written);
       rv = write_callback.WaitForResult();
       break;
     }
+    bytes_written += rv;
   }
 
   // At this point, both read and write have returned ERR_IO_PENDING, and the
@@ -393,23 +423,20 @@ TEST_P(TransportClientSocketTest, DISABLED_FullDuplex_ReadFirst) {
   EXPECT_GE(rv, 0);
 }
 
-TEST_P(TransportClientSocketTest, DISABLED_FullDuplex_WriteFirst) {
+TEST_P(TransportClientSocketTest, FullDuplex_WriteFirst) {
   TestCompletionCallback callback;
   int rv = sock_->Connect(callback.callback());
-  if (rv != OK) {
-    ASSERT_EQ(ERR_IO_PENDING, rv);
-
-    rv = callback.WaitForResult();
-    EXPECT_EQ(OK, rv);
-  }
+  loop_.Run();
+  EXPECT_EQ(OK, callback.GetResult(rv));
 
-  PauseServerReads();
+  //  PauseServerReads();
   const int kWriteBufLen = 64 * 1024;
   scoped_refptr<IOBuffer> request_buffer(new IOBuffer(kWriteBufLen));
   char* request_data = request_buffer->data();
   memset(request_data, 'A', kWriteBufLen);
   TestCompletionCallback write_callback;
 
+  int bytes_written = 0;
   while (true) {
     rv = sock_->Write(
         request_buffer.get(), kWriteBufLen, write_callback.callback());
@@ -417,6 +444,7 @@ TEST_P(TransportClientSocketTest, DISABLED_FullDuplex_WriteFirst) {
 
     if (rv == ERR_IO_PENDING)
       break;
+    bytes_written += rv;
   }
 
   // Now we have the Write() blocked on ERR_IO_PENDING.  It's time to force the
@@ -435,7 +463,7 @@ TEST_P(TransportClientSocketTest, DISABLED_FullDuplex_WriteFirst) {
   // run the write and read callbacks to make sure they can handle full duplex
   // communications.
 
-  ResumeServerReads();
+  StartServerReads(bytes_written);
   rv = write_callback.WaitForResult();
   EXPECT_GE(rv, 0);