Use TCPServerSocket instead of TCPListenSocket in transport_client_socket_unittest.cc

net/socket/transport_client_socket_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/base/test_completion_callback.h"
 #include "net/base/winsock_init.h"
 #include "net/dns/mock_host_resolver.h"
 #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"
 
 namespace net {
 
 namespace {
 
 const char kServerReply[] = "HTTP/1.1 404 Not Found";
 
 enum ClientSocketTestTypes {
   TCP,
   SCTP
 };
 
 }  // namespace
 
 class TransportClientSocketTest
-    : public StreamListenSocket::Delegate,
-      public ::testing::TestWithParam<ClientSocketTestTypes> {
+    : public ::testing::TestWithParam<ClientSocketTestTypes> {
  public:
   TransportClientSocketTest()
       : listen_port_(0),
         socket_factory_(ClientSocketFactory::GetDefaultFactory()),
         close_server_socket_on_next_send_(false) {
   }
 
   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;
 
   void CloseServerSocket() {
     // delete the connected_sock_, which will close it.
     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;
   }
 
  protected:
   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_;
+  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;
   const uint16 kMaxPort = 10200;
 #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.
   scoped_ptr<HostResolver> resolver(new MockHostResolver());
   HostResolver::RequestInfo info(HostPortPair("localhost", listen_port_));
   TestCompletionCallback callback;
   int rv = resolver->Resolve(
       info, DEFAULT_PRIORITY, &addr, callback.callback(), NULL, BoundNetLog());
   CHECK_EQ(ERR_IO_PENDING, rv);
   rv = callback.WaitForResult();
@@ skipping 17 matching lines @@
     if (rv == ERR_IO_PENDING)
       rv = callback->WaitForResult();
 
     EXPECT_GE(rv, 0);
     bytes_read += rv;
   }
 
   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.
 INSTANTIATE_TEST_CASE_P(StreamSocket,
                         TransportClientSocketTest,
                         ::testing::Values(TCP));
 
 TEST_P(TransportClientSocketTest, Connect) {
   TestCompletionCallback callback;
   EXPECT_FALSE(sock_->IsConnected());
@@ skipping 31 matching lines @@
   int rv = sock_->Connect(callback.callback());
   if (rv != OK) {
     ASSERT_EQ(rv, ERR_IO_PENDING);
     rv = callback.WaitForResult();
     EXPECT_EQ(rv, OK);
   }
   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);
   ASSERT_EQ(bytes_read, 1u);
 
   // Socket should be considered connected, but not idle, due to
   // pending data.
   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);
 
   // After draining the data, the socket should be back to connected
   // and idle.
   EXPECT_TRUE(sock_->IsConnected());
   EXPECT_TRUE(sock_->IsConnectedAndIdle());
 
   // 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);
 
   // As above because of data.
   EXPECT_TRUE(sock_->IsConnected());
   EXPECT_FALSE(sock_->IsConnectedAndIdle());
 
   bytes_read = DrainClientSocket(
       buf.get(), 4096, arraysize(kServerReply) - 2, &callback);
@@ skipping 13 matching lines @@
 
 TEST_P(TransportClientSocketTest, Read) {
   TestCompletionCallback callback;
   int rv = sock_->Connect(callback.callback());
   if (rv != OK) {
     ASSERT_EQ(rv, ERR_IO_PENDING);
 
     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);
 
   // 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.
 
   rv = sock_->Read(buf.get(), 4096, callback.callback());
   ASSERT_EQ(ERR_IO_PENDING, rv);
   CloseServerSocket();
   EXPECT_EQ(0, callback.WaitForResult());
 }
 
 TEST_P(TransportClientSocketTest, Read_SmallChunks) {
   TestCompletionCallback callback;
   int rv = sock_->Connect(callback.callback());
   if (rv != OK) {
     ASSERT_EQ(rv, ERR_IO_PENDING);
 
     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) {
     rv = sock_->Read(buf.get(), 1, callback.callback());
     EXPECT_TRUE(rv >= 0 || rv == ERR_IO_PENDING);
 
     if (rv == ERR_IO_PENDING)
       rv = callback.WaitForResult();
 
@@ skipping 12 matching lines @@
 
 TEST_P(TransportClientSocketTest, Read_Interrupted) {
   TestCompletionCallback callback;
   int rv = sock_->Connect(callback.callback());
   if (rv != OK) {
     ASSERT_EQ(ERR_IO_PENDING, 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();
 
   EXPECT_NE(0, rv);
 }
 
 TEST_P(TransportClientSocketTest, DISABLED_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);
   }
 
   // Read first.  There's no data, so it should return ERR_IO_PENDING.
   const int kBufLen = 4096;
   scoped_refptr<IOBuffer> buf(new IOBuffer(kBufLen));
   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();
   memset(request_data, 'A', kWriteBufLen);
   TestCompletionCallback write_callback;
 
   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();
+      // ResumeServerReads();
       rv = write_callback.WaitForResult();
       break;
     }
   }
 
   // At this point, both read and write have returned ERR_IO_PENDING, and the
   // write callback has executed.  We wait for the read callback to run now to
   // make sure that the socket can handle full duplex communications.
 
   rv = callback.WaitForResult();

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

I wonder what the best way to deal with these two disabled tests is. I tried
simply removing "PauseServerReads()", and doing listen_sock_->Read in
"ResumeServerReads()". But the read callback seems to be blocked on line 399 and
on line 453.

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

In this test, the problem is the buffer is full.  You should read everything
from the connected_sock_, and then this should pass (Could also count total
bytes written from the last n - 1 calls to write, and make sure you can read
them all)

You'll also need to write something to connected_sock_ before waiting on the
read callback, of course.

Next test presumably has similar issues.  Haven't looked at it.

[mmenke, 2015/04/06 18:44:38]

One other option:  You could make the connected_socket_ behave as the old listen
socket did, though instead of pause/resume methods, you'd just need a
StartReading method, which calls Read.  Once any data is read, it writes it all
(Setting a write callback, and writing again as needed), and once all data is
written, calling read again.

Up to you which approach you prefer.

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

Done. Right, I should have write something to connected_sock_, otherwise the
read callback will block forever. The original listen socket does not seem to do
writes, I wonder why read callback does not block (line 422 of the new patch).

   EXPECT_GE(rv, 0);
 }
 
 TEST_P(TransportClientSocketTest, DISABLED_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);
   }
 
-  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;
 
   while (true) {
     rv = sock_->Write(
         request_buffer.get(), kWriteBufLen, write_callback.callback());
     ASSERT_TRUE(rv >= 0 || rv == ERR_IO_PENDING);
@@ skipping 11 matching lines @@
     rv = sock_->Read(buf.get(), kBufLen, callback.callback());
     ASSERT_TRUE(rv >= 0 || rv == ERR_IO_PENDING);
     if (rv == ERR_IO_PENDING)
       break;
   }
 
   // At this point, both read and write have returned ERR_IO_PENDING.  Now we
   // 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);
 
   // It's possible the read is blocked because it's already read all the data.
   // Close the server socket, so there will at least be a 0-byte read.
   CloseServerSocket();
 
   rv = callback.WaitForResult();
   EXPECT_GE(rv, 0);
 }
 
 }  // namespace net