Port QuicServer to Chrome network stack

net/quic/quic_server.cc

Index: net/quic/quic_server.cc
diff --git a/net/tools/quic/quic_server.cc b/net/quic/quic_server.cc
similarity index 22%
copy from net/tools/quic/quic_server.cc
copy to net/quic/quic_server.cc
index be3d9fbd128178a8161294247535f455dd3053c6..97975e490ab121bf1507370933c53778f2f8dc65 100644
--- a/net/tools/quic/quic_server.cc
+++ b/net/quic/quic_server.cc
@@ -2,182 +2,108 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "net/tools/quic/quic_server.h"
+#include "net/quic/quic_server.h"
 
-#include <errno.h>
-#include <features.h>
-#include <netinet/in.h>
 #include <string.h>
-#include <sys/epoll.h>
-#include <sys/socket.h>
 
 #include "net/base/ip_endpoint.h"
+#include "net/base/net_errors.h"
 #include "net/quic/congestion_control/tcp_receiver.h"
 #include "net/quic/crypto/crypto_handshake.h"
 #include "net/quic/crypto/quic_random.h"
-#include "net/quic/quic_clock.h"
 #include "net/quic/quic_crypto_stream.h"
 #include "net/quic/quic_data_reader.h"
+#include "net/quic/quic_dispatcher.h"
+#include "net/quic/quic_in_memory_cache.h"
 #include "net/quic/quic_protocol.h"
-#include "net/tools/quic/quic_in_memory_cache.h"
+#include "net/quic/quic_server_packet_writer.h"
 #include "net/tools/quic/quic_socket_utils.h"
 
-#define MMSG_MORE 0
 
-#ifndef SO_RXQ_OVFL
-#define SO_RXQ_OVFL 40
-#endif
+namespace net {
 
-const int kEpollFlags = EPOLLIN | EPOLLOUT | EPOLLET;
 static const char kSourceAddressTokenSecret[] = "secret";
-const uint32 kServerInitialFlowControlWindow = 100 * net::kMaxPacketSize;
 
-namespace net {
-namespace tools {
-
-QuicServer::QuicServer()
-    : port_(0),
-      fd_(-1),
-      packets_dropped_(0),
-      overflow_supported_(false),
-      use_recvmmsg_(false),
-      crypto_config_(kSourceAddressTokenSecret, QuicRandom::GetInstance()),
-      supported_versions_(QuicSupportedVersions()) {
-  // Use hardcoded crypto parameters for now.
-  config_.SetDefaults();
-  Initialize();
-}
+// Allocate some extra space so we can send an error if the client goes over
+// the limit.
+static const int kReadBufferSize = 2 * kMaxPacketSize;

[wtc, 2014/06/20 21:40:49]

Put these two constants inside an unnamed namespace.

[dmziegler, 2014/06/23 18:31:24]

Done.

 
 QuicServer::QuicServer(const QuicConfig& config,
                        const QuicVersionVector& supported_versions)
-    : port_(0),
-      fd_(-1),
-      packets_dropped_(0),
-      overflow_supported_(false),
-      use_recvmmsg_(false),
+    : helper_(new QuicConnectionHelper(

[wtc, 2014/06/20 21:40:49]

BUG: |helper_| seems to be leaked.

I think |helper_| should be declared as a non-pointer member:

  QuicConnectionHelperInterface helper_;

and initialized like this:

    : helper_(base::MessageLoop::current()->message_loop_proxy().get(),
              &clock_,
              QuicRandom::GetInstance())),

[dmziegler, 2014/06/23 18:31:24]

Done.

+        base::MessageLoop::current()->message_loop_proxy().get(),
+        &clock_,
+        QuicRandom::GetInstance())),
       config_(config),
       crypto_config_(kSourceAddressTokenSecret, QuicRandom::GetInstance()),
-      supported_versions_(supported_versions) {
+      supported_versions_(supported_versions),
+      read_pending_(false),
+      synchronous_read_count_(0),
+      read_buffer_(new IOBufferWithSize(kReadBufferSize)),
+      weak_factory_(this) {
   Initialize();
 }
 
 void QuicServer::Initialize() {
-#if MMSG_MORE
-  use_recvmmsg_ = true;
-#endif
-  epoll_server_.set_timeout_in_us(50 * 1000);

[wtc, 2014/06/20 21:40:49]

Do you know what this 50-second timeout is?

[dmziegler, 2014/06/23 18:31:23]

It controls how long epoll_server_.WaitForEventsAndExecuteCallbacks() waits;
since that was just called in an infinite loop anyway and Chrome's message loop
doesn't have this functionality, I took it out. (Also, it's actually a 50ms
timeout.)

   // Initialize the in memory cache now.
   QuicInMemoryCache::GetInstance();
 
-  QuicEpollClock clock(&epoll_server_);
-
   scoped_ptr<CryptoHandshakeMessage> scfg(
-      crypto_config_.AddDefaultConfig(
-          QuicRandom::GetInstance(), &clock,
-          QuicCryptoServerConfig::ConfigOptions()));
-
-  // Set flow control options in the config.
-  config_.SetInitialCongestionWindowToSend(kServerInitialFlowControlWindow);

[wtc, 2014/06/20 21:40:49]

Just wanted to make sure you meant to delete this.

[dmziegler, 2014/06/23 18:31:24]

Hmm, no, I can't remember deleting it.

+      crypto_config_.AddDefaultConfig(QuicRandom::GetInstance(),

[wtc, 2014/06/20 21:40:49]

It seems that we should get the QuicRandom from |helper_|.

[dmziegler, 2014/06/23 18:31:24]

Done.

+                                      helper_->GetClock(),
+                                      QuicCryptoServerConfig::ConfigOptions()));
 }
 
 QuicServer::~QuicServer() {
 }
 
-bool QuicServer::Listen(const IPEndPoint& address) {
-  port_ = address.port();
-  int address_family = address.GetSockAddrFamily();
-  fd_ = socket(address_family, SOCK_DGRAM | SOCK_NONBLOCK, IPPROTO_UDP);
-  if (fd_ < 0) {
-    LOG(ERROR) << "CreateSocket() failed: " << strerror(errno);
-    return false;
-  }
+int QuicServer::Listen(const IPEndPoint& address) {
+  scoped_ptr<UDPServerSocket> socket(
+      new UDPServerSocket(NULL, NetLog::Source()));

[wtc, 2014/06/20 21:40:49]

We may want to add a
  NetLog net_log_;
member and pass &net_log_ as the first argument to the UDPServerSocket
constructor.

[dmziegler, 2014/06/23 18:31:24]

Done.

 
-  int rc = QuicSocketUtils::SetGetAddressInfo(fd_, address_family);
+  // TODO(dmz): socket->AllowBroadcast()? socket->SetMulticastInterface?

[wtc, 2014/06/20 21:40:49]

The answer is No to both questions. The original code doesn't set the equivalent
of either socket option.

[dmziegler, 2014/06/23 18:31:24]

Done.

+  socket->AllowAddressReuse();

[wtc, 2014/06/20 21:40:49]

The original code doesn't set this socket option (SO_REUSEADDR) either. It seems
fine to me though.

[dmziegler, 2014/06/23 18:31:23]

Okay. I can't remember why I thought it should be added.

 
+  int rc = socket->Listen(address);

[wtc, 2014/06/20 21:40:49]

socket->Listen() matches the bind() call in the original code. So it is better
to do this after the socket->SetSendBufferSize call to be safe.

[dmziegler, 2014/06/23 18:31:24]

Actually, I had that originally, but the code crashed -- it appears the
UDPServerSocket doesn't have a valid fd until you call Listen.

   if (rc < 0) {
-    LOG(ERROR) << "IP detection not supported" << strerror(errno);
-    return false;
-  }
-
-  int get_overflow = 1;
-  rc = setsockopt(
-      fd_, SOL_SOCKET, SO_RXQ_OVFL, &get_overflow, sizeof(get_overflow));
-
-  if (rc < 0) {
-    DLOG(WARNING) << "Socket overflow detection not supported";
-  } else {
-    overflow_supported_ = true;
+    LOG(ERROR) << "Listen() failed: " << strerror(errno);

[wtc, 2014/06/20 21:40:49]

IMPORTANT: print ErrorToString(rc) instead of strerror(errno). Fix all the
occurrences in this file.

On Windows, |errno| is only used for Standard C Library functions, not for
Winsock functions.

[dmziegler, 2014/06/23 18:31:23]

Done.

+    return rc;
   }
 
   // These send and receive buffer sizes are sized for a single connection,
   // because the default usage of QuicServer is as a test server with one or
   // two clients.  Adjust higher for use with many clients.
-  if (!QuicSocketUtils::SetReceiveBufferSize(fd_,
-                                             TcpReceiver::kReceiveWindowTCP)) {
-    return false;
+  rc = socket->SetReceiveBufferSize(TcpReceiver::kReceiveWindowTCP);
+  if (rc < 0) {
+    LOG(ERROR) << "SetReceiveBufferSize() failed: " << strerror(errno);
+    return rc;
   }
 
-  if (!QuicSocketUtils::SetSendBufferSize(fd_,
-                                          TcpReceiver::kReceiveWindowTCP)) {
-    return false;
+  rc = socket->SetSendBufferSize(20 * kMaxPacketSize);
+  if (rc < 0) {
+    LOG(ERROR) << "SetSendBufferSize() failed: " << strerror(errno);
+    return rc;
   }
 
-  // Enable the socket option that allows the local address to be
-  // returned if the socket is bound to more than on address.
-  int get_local_ip = 1;
-  rc = setsockopt(fd_, IPPROTO_IP, IP_PKTINFO,
-                  &get_local_ip, sizeof(get_local_ip));
-  if (rc == 0 && address_family == AF_INET6) {
-    rc = setsockopt(fd_, IPPROTO_IPV6, IPV6_RECVPKTINFO,
-                    &get_local_ip, sizeof(get_local_ip));
-  }
-  if (rc != 0) {
-    LOG(ERROR) << "Failed to set required socket options";
-    return false;
-  }

[wtc, 2014/06/20 21:40:49]

This is a problem with the original code. Lines 125-137 are exactly what the
QuicSocketUtils::SetGetAddressInfo method does. So this is redundant with the
QuicSocketUtils::SetGetAddressInfo call on line 95. We should fix this in the
internal version.

 
-  sockaddr_storage raw_addr;
-  socklen_t raw_addr_len = sizeof(raw_addr);
-  CHECK(address.ToSockAddr(reinterpret_cast<sockaddr*>(&raw_addr),
-                           &raw_addr_len));
-  rc = bind(fd_,
-            reinterpret_cast<const sockaddr*>(&raw_addr),
-            sizeof(raw_addr));
-  if (rc < 0) {
-    LOG(ERROR) << "Bind failed: " << strerror(errno);
-    return false;
-  }
+  socket->GetLocalAddress(&server_address_);

[wtc, 2014/06/20 21:40:49]

Nit: check the return value.

[dmziegler, 2014/06/23 18:31:23]

Done.

 
-  DVLOG(1) << "Listening on " << address.ToString();
-  if (port_ == 0) {
-    SockaddrStorage storage;
-    IPEndPoint server_address;
-    if (getsockname(fd_, storage.addr, &storage.addr_len) != 0 ||
-        !server_address.FromSockAddr(storage.addr, storage.addr_len)) {
-      LOG(ERROR) << "Unable to get self address.  Error: " << strerror(errno);
-      return false;
-    }
-    port_ = server_address.port();
-    DVLOG(1) << "Kernel assigned port is " << port_;
-  }
+  DVLOG(1) << "Listening on " << server_address_.ToString();
 
-  epoll_server_.RegisterFD(fd_, this, kEpollFlags);
-  dispatcher_.reset(CreateQuicDispatcher());
-  dispatcher_->Initialize(fd_);
+  socket_.swap(socket);
 
-  return true;
-}
+  dispatcher_.reset(
+      new QuicDispatcher(config_,
+                         crypto_config_,
+                         supported_versions_,
+                         helper_));
+  QuicServerPacketWriter* writer = new QuicServerPacketWriter(dispatcher_.get(),
+                                                              socket_.get());
+  dispatcher_->Initialize(writer);
 
-QuicDispatcher* QuicServer::CreateQuicDispatcher() {
-  return new QuicDispatcher(
-      config_,
-      crypto_config_,
-      supported_versions_,
-      &epoll_server_);
-}
+  StartReading();
 
-void QuicServer::WaitForEvents() {
-  epoll_server_.WaitForEventsAndExecuteCallbacks();
+  return OK;
 }
 
 void QuicServer::Shutdown() {
@@ -185,60 +111,54 @@ void QuicServer::Shutdown() {
   // notify clients that they're closing.
   dispatcher_->Shutdown();
 
-  close(fd_);
-  fd_ = -1;
+  socket_->Close();

[wtc, 2014/06/20 21:40:49]

Add:
    socket_.reset();

[dmziegler, 2014/06/23 18:31:24]

Done.

 }
 
-void QuicServer::OnEvent(int fd, EpollEvent* event) {
-  DCHECK_EQ(fd, fd_);
-  event->out_ready_mask = 0;
-
-  if (event->in_events & EPOLLIN) {
-    DVLOG(1) << "EPOLLIN";
-    bool read = true;
-    while (read) {
-        read = ReadAndDispatchSinglePacket(
-            fd_, port_, dispatcher_.get(),
-            overflow_supported_ ? &packets_dropped_ : NULL);
-    }
+void QuicServer::StartReading() {
+  if (read_pending_) {
+    return;
   }
-  if (event->in_events & EPOLLOUT) {
-    dispatcher_->OnCanWrite();
-    if (dispatcher_->HasPendingWrites()) {
-      event->out_ready_mask |= EPOLLOUT;
-    }
+
+  int result = socket_->RecvFrom(
+      read_buffer_.get(),
+      read_buffer_->size(),
+      &read_addr_,
+      base::Bind(&QuicServer::OnReadComplete, base::Unretained(this)));
+
+  if (result == ERR_IO_PENDING) {
+    read_pending_ = true;
+    return;
   }

[wtc, 2014/06/20 21:40:49]

Please explain to me in person how the new code does the equivalent of lines
205-210 of the original code.

-  if (event->in_events & EPOLLERR) {
+
+  if (++synchronous_read_count_ > 32) {
+    synchronous_read_count_ = 0;

[wtc, 2014/06/20 21:40:49]

IMPORTANT: we should set |read_pending_| to true in this case. We are simulating
a pending read here even though the read has completed synchronously. A
|read_pending_| of true prevents the read_buffer_ and read_addr_ from being
overwritten by a new RecvFrom call.

[dmziegler, 2014/06/23 18:31:24]

Good point. In practice, I don't think this causes a bug, since StartReading()
should just be called once to start, and after that only from the callback.
However, it's definitely good to protect this in case that changes.

+    // Schedule the processing through the message loop to 1) prevent infinite
+    // recursion and 2) avoid blocking the thread for too long.
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(&QuicServer::OnReadComplete,
+                   weak_factory_.GetWeakPtr(),
+                   result));
+  } else {
+    OnReadComplete(result);
   }
 }
 
-/* static */
-bool QuicServer::ReadAndDispatchSinglePacket(int fd,
-                                             int port,
-                                             QuicDispatcher* dispatcher,
-                                             uint32* packets_dropped) {
-  // Allocate some extra space so we can send an error if the client goes over
-  // the limit.
-  char buf[2 * kMaxPacketSize];
-
-  IPEndPoint client_address;
-  IPAddressNumber server_ip;
-  int bytes_read =
-      QuicSocketUtils::ReadPacket(fd, buf, arraysize(buf),
-                                  packets_dropped,
-                                  &server_ip, &client_address);
-
-  if (bytes_read < 0) {
-    return false;  // We failed to read.
-  }
+void QuicServer::OnReadComplete(int result) {
+  read_pending_ = false;
+  if (result == 0)

[wtc, 2014/06/20 21:40:49]

I don't think we will ever get a read result of 0 for a connectionless socket.
If you copied this from some other file, then this is fine.

[dmziegler, 2014/06/23 18:31:24]

Yes, I copied it from QuicClientSession.

+    result = ERR_CONNECTION_CLOSED;
 
-  QuicEncryptedPacket packet(buf, bytes_read, false);
+  if (result < 0) {
+    LOG(ERROR) << "QuicServer read failed: " << ErrorToString(result);
+    Shutdown();
+    return;
+  }
 
-  IPEndPoint server_address(server_ip, port);
-  dispatcher->ProcessPacket(server_address, client_address, packet);
+  QuicEncryptedPacket packet(read_buffer_->data(), result, false);
+  dispatcher_->ProcessPacket(server_address_, read_addr_, packet);
 
-  return true;
+  StartReading();
 }
 
-}  // namespace tools
 }  // namespace net