Separate Authenticator and Session unittests.

remoting/protocol/connection_tester.cc

+// Copyright (c) 2011 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 "remoting/protocol/connection_tester.h"
+
+#include "base/bind.h"
+#include "base/message_loop.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "net/socket/stream_socket.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace remoting {
+namespace protocol {
+
+StreamConnectionTester::StreamConnectionTester(net::StreamSocket* client_socket,
+                                               net::StreamSocket* host_socket,
+                                               int message_size,
+                                               int message_count)
+    : message_loop_(MessageLoop::current()),
+      host_socket_(host_socket),
+      client_socket_(client_socket),
+      message_size_(message_size),
+      message_count_(message_count),
+      test_data_size_(message_size * message_count),
+      done_(false),
+      ALLOW_THIS_IN_INITIALIZER_LIST(
+          write_cb_(this, &StreamConnectionTester::OnWritten)),
+      ALLOW_THIS_IN_INITIALIZER_LIST(
+          read_cb_(this, &StreamConnectionTester::OnRead)),
+      write_errors_(0),
+      read_errors_(0) {
+}
+
+StreamConnectionTester::~StreamConnectionTester() {
+}
+
+void StreamConnectionTester::Start() {
+  InitBuffers();
+  DoRead();
+  DoWrite();
+}
+
+void StreamConnectionTester::CheckResults() {
+  EXPECT_EQ(0, write_errors_);
+  EXPECT_EQ(0, read_errors_);
+
+  ASSERT_EQ(test_data_size_, input_buffer_->offset());
+
+  output_buffer_->SetOffset(0);
+  ASSERT_EQ(test_data_size_, output_buffer_->size());
+
+  EXPECT_EQ(0, memcmp(output_buffer_->data(),
+                      input_buffer_->StartOfBuffer(), test_data_size_));
+}
+
+void StreamConnectionTester::Done() {
+  done_ = true;
+  message_loop_->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+}
+
+void StreamConnectionTester::InitBuffers() {
+  output_buffer_ = new net::DrainableIOBuffer(
+      new net::IOBuffer(test_data_size_), test_data_size_);
+
+  input_buffer_ = new net::GrowableIOBuffer();
+}
+
+void StreamConnectionTester::DoWrite() {
+  int result = 1;
+  while (result > 0) {
+    if (output_buffer_->BytesRemaining() == 0)
+      break;
+
+    int bytes_to_write = std::min(output_buffer_->BytesRemaining(),
+                                  message_size_);
+    result = client_socket_->Write(output_buffer_, bytes_to_write,
+                                   &write_cb_);
+    HandleWriteResult(result);
+  }
+}
+
+void StreamConnectionTester::OnWritten(int result) {
+  HandleWriteResult(result);
+  DoWrite();
+}
+
+void StreamConnectionTester::HandleWriteResult(int result) {
+  if (result <= 0 && result != net::ERR_IO_PENDING) {
+    LOG(ERROR) << "Received error " << result << " when trying to write";
+    write_errors_++;
+    Done();
+  } else if (result > 0) {
+    output_buffer_->DidConsume(result);
+  }
+}
+
+void StreamConnectionTester::DoRead() {
+  int result = 1;
+  while (result > 0) {
+    input_buffer_->SetCapacity(input_buffer_->offset() + message_size_);
+    result = host_socket_->Read(input_buffer_, message_size_, &read_cb_);
+    HandleReadResult(result);
+  };
+}
+
+void StreamConnectionTester::OnRead(int result) {
+  HandleReadResult(result);
+  if (!done_)
+    DoRead();  // Don't try to read again when we are done reading.
+}
+
+void StreamConnectionTester::HandleReadResult(int result) {
+  if (result <= 0 && result != net::ERR_IO_PENDING) {
+    if (!done_) {

[Wez, 2011/12/09 23:42:33]

It looks like this will cause any errors reported after we're "done" to be
ignored, which seems wrong; surely we should also test for valid end-of-stream
behaviour?

[Sergey Ulanov, 2011/12/12 22:52:00]

There is no end-of-stream for P2P sockets: stream ether exists and should be
connected or it is destroyed. If one end closes connection then the other end
should be informed about it over the signalling channel and close it too. There
is Disconnect() method, but I don't see anything we can verify about it: Read()
and Write() are not supposed to be called after Disconnect() -
SSLClientSocketNSS DCHECK's if they are. In any case, the calling test can
verify end-of-stream behavior outside of this class if it needs to. The job of
this class is just to verify connectivity, but not correctness of a Socket
implementation in general.

[Wez, 2011/12/13 00:12:38]

You're right re EOF, but my point is that this code ignores errors occurring
after |done_| is set, which seems wrong; surely once we're "done" we teardown
and should not see errors?

[Sergey Ulanov, 2011/12/13 02:31:55]

I see. I removed this if because we don't need it.

+      LOG(ERROR) << "Received error " << result << " when trying to read";
+      read_errors_++;
+      Done();
+    }
+  } else if (result > 0) {
+    // Allocate memory for the next read.
+    input_buffer_->set_offset(input_buffer_->offset() + result);
+    if (input_buffer_->offset() == test_data_size_)
+      Done();
+  }
+}
+
+DatagramConnectionTester::DatagramConnectionTester(net::Socket* client_socket,
+                                                   net::Socket* host_socket,
+                                                   int message_size,
+                                                   int message_count,
+                                                   int delay_ms)
+    : message_loop_(MessageLoop::current()),
+      host_socket_(host_socket),
+      client_socket_(client_socket),
+      message_size_(message_size),
+      message_count_(message_count),
+      delay_ms_(delay_ms),
+      done_(false),
+      ALLOW_THIS_IN_INITIALIZER_LIST(
+          write_cb_(this, &DatagramConnectionTester::OnWritten)),
+      ALLOW_THIS_IN_INITIALIZER_LIST(
+          read_cb_(this, &DatagramConnectionTester::OnRead)),
+      write_errors_(0),
+      read_errors_(0),
+      packets_sent_(0),
+      packets_received_(0),
+      broken_packets_(0) {
+  sent_packets_.resize(message_count_);
+}
+
+DatagramConnectionTester::~DatagramConnectionTester() {
+}
+
+void DatagramConnectionTester::Start() {
+  DoRead();
+  DoWrite();
+}
+
+void DatagramConnectionTester::CheckResults() {
+  EXPECT_EQ(0, write_errors_);
+  EXPECT_EQ(0, read_errors_);
+
+  EXPECT_EQ(0, broken_packets_);
+
+  // Verify that we've received at least one packet.
+  EXPECT_GT(packets_received_, 0);

[Wez, 2011/12/09 23:42:33]

You're testing for >0 on the basis that ==0 means nothing got through => broken,
but we have to expect some to get dropped?  That seems like a recipe for obscure
flake to me.

[Sergey Ulanov, 2011/12/12 22:52:00]

We send 100 packets with 10ms delay between packets to the same machine. It very
unlikely that all of them are dropped. We had this test for very long time and I
never saw it cause any flake.

You can apply the same argument to TCP connection: it is possible that a TCP
connection test never succeeds because the first SYN packets gets dropped every
time, but we need some connection tests nonetheless.

[Wez, 2011/12/13 00:12:38]

Fair enough.  Sending 100 packets with 10ms delay also means at least 1s to
complete the test, which isn't ideal.  Given that the test passes so long as at
least one message gets through, perhaps it should also complete as soon as the
first message gets through?
True.  Assuming local loopback is reasonably reliable is fair, but we need to be
wary of weird behaviours under high load (i.e. try-bots).

[Sergey Ulanov, 2011/12/13 02:31:55]

I think it's useful to send more than one message to verify that messages aren't
broken.

+  LOG(INFO) << "Received " << packets_received_ << " packets out of "
+            << message_count_;
+}
+
+void DatagramConnectionTester::Done() {
+  done_ = true;
+  message_loop_->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+}
+
+void DatagramConnectionTester::DoWrite() {
+  if (packets_sent_ >= message_count_) {
+    Done();
+    return;
+  }
+
+  scoped_refptr<net::IOBuffer> packet(new net::IOBuffer(message_size_));
+  memset(packet->data(), 123, message_size_);
+  sent_packets_[packets_sent_] = packet;
+  // Put index of this packet in the beginning of the packet body.
+  memcpy(packet->data(), &packets_sent_, sizeof(packets_sent_));
+
+  int result = client_socket_->Write(packet, message_size_, &write_cb_);
+  HandleWriteResult(result);
+}
+
+void DatagramConnectionTester::OnWritten(int result) {
+  HandleWriteResult(result);
+}
+
+void DatagramConnectionTester::HandleWriteResult(int result) {
+  if (result <= 0 && result != net::ERR_IO_PENDING) {
+    LOG(ERROR) << "Received error " << result << " when trying to write";
+    write_errors_++;
+    Done();
+  } else if (result > 0) {
+    EXPECT_EQ(message_size_, result);
+    packets_sent_++;
+    message_loop_->PostDelayedTask(FROM_HERE, base::Bind(
+        &DatagramConnectionTester::DoWrite, base::Unretained(this)), delay_ms_);
+  }
+}
+
+void DatagramConnectionTester::DoRead() {
+  int result = 1;
+  while (result > 0) {
+    int kReadSize = message_size_ * 2;
+    read_buffer_ = new net::IOBuffer(kReadSize);
+
+    result = host_socket_->Read(read_buffer_, kReadSize, &read_cb_);
+    HandleReadResult(result);
+  };
+}
+
+void DatagramConnectionTester::OnRead(int result) {
+  HandleReadResult(result);
+  DoRead();
+}
+
+void DatagramConnectionTester::HandleReadResult(int result) {
+  if (result <= 0 && result != net::ERR_IO_PENDING) {
+    // Error will be received after the socket is closed.
+    if (!done_) {
+      LOG(ERROR) << "Received error " << result << " when trying to read";
+      read_errors_++;
+      Done();
+    }
+  } else if (result > 0) {
+    packets_received_++;
+    if (message_size_ != result) {
+      // Invalid packet size;
+      broken_packets_++;
+    } else {
+      // Validate packet body.
+      int packet_id;
+      memcpy(&packet_id, read_buffer_->data(), sizeof(packet_id));
+      if (packet_id < 0 || packet_id >= message_count_) {
+        broken_packets_++;
+      } else {
+        if (memcmp(read_buffer_->data(), sent_packets_[packet_id]->data(),
+                   message_size_) != 0)
+          broken_packets_++;
+      }
+    }
+  }
+}
+
+}  // namespace protocol
+}  // namespace remoting