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..5027509323d86c9cb4ef53ff1bc90732b6103ffd 100644
--- a/net/socket/transport_client_socket_unittest.cc
+++ b/net/socket/transport_client_socket_unittest.cc
@@ -5,6 +5,7 @@
 #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 "net/base/address_list.h"
@@ -16,7 +17,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 +35,7 @@ enum ClientSocketTestTypes {
 }  // namespace
 
 class TransportClientSocketTest
-    : public StreamListenSocket::Delegate,
-      public ::testing::TestWithParam<ClientSocketTestTypes> {
+    : public ::testing::TestWithParam<ClientSocketTestTypes> {
  public:
   TransportClientSocketTest()
       : listen_port_(0),
@@ -46,21 +46,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,20 +54,14 @@ class TransportClientSocketTest
     connected_sock_.reset();
   }
 
-  void PauseServerReads() {
-    connected_sock_->PauseReads();
-  }
-
-  void ResumeServerReads() {
-    connected_sock_->ResumeReads();
-  }
+  void AcceptCallback(int res) {}
 
   int DrainClientSocket(IOBuffer* buf,
                         uint32 buf_len,
                         uint32 bytes_to_read,
                         TestCompletionCallback* callback);
 
-  void SendClientRequest();
+  void ProcessRequest();
 
   void set_close_server_socket_on_next_send(bool close) {
     close_server_socket_on_next_send_ = close;
@@ -95,16 +74,16 @@ class TransportClientSocketTest
   scoped_ptr<StreamSocket> sock_;
 
  private:
-  scoped_ptr<TCPListenSocket> listen_sock_;
-  scoped_ptr<TCPListenSocket> connected_sock_;
+  scoped_ptr<TCPServerSocket> listen_sock_;
+  scoped_ptr<StreamSocket> connected_sock_;
   bool close_server_socket_on_next_send_;
 };
 
 void TransportClientSocketTest::SetUp() {
   ::testing::TestWithParam<ClientSocketTestTypes>::SetUp();
 
+  listen_sock_.reset(new TCPServerSocket(NULL, NetLog::Source()));
   // 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;
@@ -112,14 +91,16 @@ void TransportClientSocketTest::SetUp() {
 #if defined(OS_WIN)
   EnsureWinsockInit();
 #endif

[mmenke, 2015/04/06 18:37:33]

Also, we can remove this #if block - creating the TCPServerSocket creates a
TCPSocketWin, which calls this for us.

[xunjieli, 2015/04/08 14:47:19]

Done.

+  net::IPAddressNumber address;
+  net::ParseIPLiteralToNumber("127.0.0.1", &address);
   for (port = kMinPort; port < kMaxPort; port++) {

[mmenke, 2015/04/06 18:30:03]

While we're here, can we get rid of this loop?

I believe we can just use:

ASSERT_EQ(OK, listen_sock_->Listen(IPEndPoint(address, 0), 1));

And then grab the port we actually got using listen_sock_->GetLocalAddress.

[xunjieli, 2015/04/08 14:47:19]

Done.

-    sock = TCPListenSocket::CreateAndListen("127.0.0.1", port, this);
-    if (sock.get())
+    if (listen_sock_->Listen(IPEndPoint(address, port), 1) == net::OK)
       break;
   }
-  ASSERT_TRUE(sock.get() != NULL);
-  listen_sock_ = sock.Pass();
   listen_port_ = port;
+  listen_sock_->Accept(&connected_sock_,
+                       base::Bind(&TransportClientSocketTest::AcceptCallback,

[xunjieli, 2015/04/03 20:19:23]

Using a dummy callback here, not sure if there's a better way. When I use
"TestCompletionCallback", the object is already deallocated when the callback is
called.

[mmenke, 2015/04/06 18:30:03]

How about having AcceptCallback quite a base::RunLoop, which you can wait on
whenever you call Connect()?  Makes sure both sides know they have been
connected before we continue with the test.

Should also be checking the result passed to the callback.

[xunjieli, 2015/04/08 14:47:19]

Done. Good idea! thanks

+                                  base::Unretained(this)));
 
   AddressList addr;
   // MockHostResolver resolves everything to 127.0.0.1.
@@ -157,21 +138,47 @@ 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));
+  int request_len = arraysize(request_text) - 1;

[mmenke, 2015/04/06 18:30:03]

Much cleaner to have this in a variable.  Maybe just use strlen, though (And
include the appropriate header)?

[xunjieli, 2015/04/08 14:47:19]

Partially done. Sorry, I am not sure what you meant by "include the appropriate
header." Should I make |request_test| contain some request headers?

+  scoped_refptr<IOBuffer> request_buffer(new IOBuffer(request_len));
   TestCompletionCallback callback;
   int rv;
 
-  memcpy(request_buffer->data(), request_text, arraysize(request_text) - 1);
-  rv = sock_->Write(
-      request_buffer.get(), arraysize(request_text) - 1, callback.callback());
+  memcpy(request_buffer->data(), request_text, request_len);
+  rv = sock_->Write(request_buffer.get(), request_len, callback.callback());
   EXPECT_TRUE(rv >= 0 || rv == ERR_IO_PENDING);
 
   if (rv == ERR_IO_PENDING)
     rv = callback.WaitForResult();
-  EXPECT_EQ(rv, static_cast<int>(arraysize(request_text) - 1));
+  EXPECT_EQ(rv, request_len);

[mmenke, 2015/04/06 18:30:03]

nit:  Flip order, expected should go first (Know this was wrong before)

[xunjieli, 2015/04/08 14:47:19]

Done. Right!

+
+  // Confirms that the server receives what client sent.
+  scoped_refptr<IOBufferWithSize> read_buffer(
+      new IOBufferWithSize(request_len + 1));

[mmenke, 2015/04/06 18:30:03]

Is the +1 needed?

[xunjieli, 2015/04/08 14:47:19]

I am not sure. I thought we need to have 1 byte extra for the EOF. I guess not.
Done.

+  TestCompletionCallback read_callback;
+  int read_result = connected_sock_->Read(
+      read_buffer.get(), read_buffer->size(), read_callback.callback());

[mmenke, 2015/04/06 18:30:03]

I don't believe can actually be completely sure we read the entire thing in one
go (Actually, the same is true for the Write call above, too).

While in practice, the text is short enough that it should generally be taken
care in one call in either direct, I'd be more comfortable with loops on both
sides of things.

[xunjieli, 2015/04/08 14:47:19]

Done.

+  read_result = read_callback.GetResult(read_result);
+  ASSERT_EQ(request_len, read_result);
+  std::string message(request_text);
+  std::string received_message(read_buffer->data(), read_result);
+  ASSERT_EQ(message, received_message);
+
+  // Write response.
+  // TODO(dkegel): this might not be long enough to tickle some bugs.
+  TestCompletionCallback write_callback;
+  scoped_refptr<IOBufferWithSize> write_buffer(
+      new IOBufferWithSize(arraysize(kServerReply)));
+  memmove(write_buffer->data(), kServerReply, arraysize(kServerReply) - 1);
+  int write_result =
+      connected_sock_->Write(write_buffer.get(), arraysize(kServerReply) - 1,
+                             write_callback.callback());
+  write_result = write_callback.GetResult(write_result);

[mmenke, 2015/04/06 18:30:03]

Should handle ERR_IO_PENDING, like above, and I also suggest a for loop, for
partial writes.

[xunjieli, 2015/04/08 14:47:19]

But GetResult will wait if it is ERR_IO_PENDING right?

+  ASSERT_EQ((int)(arraysize(kServerReply) - 1), write_result);
+  if (close_server_socket_on_next_send_)
+    CloseServerSocket();
 }
 
 // TODO(leighton):  Add SCTP to this list when it is ready.
@@ -223,7 +230,7 @@ TEST_P(TransportClientSocketTest, 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);
@@ -245,7 +252,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);
@@ -279,7 +286,7 @@ TEST_P(TransportClientSocketTest, Read) {
     rv = callback.WaitForResult();
     EXPECT_EQ(rv, OK);
   }
-  SendClientRequest();
+  ProcessRequest();
 
   scoped_refptr<IOBuffer> buf(new IOBuffer(4096));
   uint32 bytes_read = DrainClientSocket(
@@ -304,7 +311,7 @@ TEST_P(TransportClientSocketTest, Read_SmallChunks) {
     rv = callback.WaitForResult();
     EXPECT_EQ(rv, OK);
   }
-  SendClientRequest();
+  ProcessRequest();
 
   scoped_refptr<IOBuffer> buf(new IOBuffer(1));
   uint32 bytes_read = 0;
@@ -337,7 +344,7 @@ TEST_P(TransportClientSocketTest, Read_Interrupted) {
     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));
@@ -366,7 +373,7 @@ TEST_P(TransportClientSocketTest, DISABLED_FullDuplex_ReadFirst) {
   rv = sock_->Read(buf.get(), kBufLen, callback.callback());
   EXPECT_EQ(ERR_IO_PENDING, rv);
 
-  PauseServerReads();
+  // PauseServerReads();
   const int kWriteBufLen = 64 * 1024;
   scoped_refptr<IOBuffer> request_buffer(new IOBuffer(kWriteBufLen));
   char* request_data = request_buffer->data();
@@ -379,7 +386,7 @@ TEST_P(TransportClientSocketTest, DISABLED_FullDuplex_ReadFirst) {
     ASSERT_TRUE(rv >= 0 || rv == ERR_IO_PENDING);
 
     if (rv == ERR_IO_PENDING) {
-      ResumeServerReads();
+      // ResumeServerReads();
       rv = write_callback.WaitForResult();
       break;
     }
@@ -403,7 +410,7 @@ TEST_P(TransportClientSocketTest, DISABLED_FullDuplex_WriteFirst) {
     EXPECT_EQ(OK, rv);
   }
 
-  PauseServerReads();
+  //  PauseServerReads();
   const int kWriteBufLen = 64 * 1024;
   scoped_refptr<IOBuffer> request_buffer(new IOBuffer(kWriteBufLen));
   char* request_data = request_buffer->data();
@@ -435,7 +442,7 @@ TEST_P(TransportClientSocketTest, DISABLED_FullDuplex_WriteFirst) {
   // run the write and read callbacks to make sure they can handle full duplex
   // communications.
 
-  ResumeServerReads();
+  // ResumeServerReads();
   rv = write_callback.WaitForResult();
   EXPECT_GE(rv, 0);