Limits only 16 new QUIC connections can be opened per socket event for QuicSimpleServer. Fix test f…

net/tools/quic/quic_simple_server.cc

 // Copyright 2014 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/tools/quic/quic_simple_server.h"
 
 #include <string.h>
 
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/net_errors.h"
 #include "net/quic/core/crypto/crypto_handshake.h"
 #include "net/quic/core/crypto/quic_random.h"
 #include "net/quic/core/quic_crypto_stream.h"
 #include "net/quic/core/quic_data_reader.h"
 #include "net/quic/core/quic_protocol.h"
 #include "net/tools/quic/quic_simple_dispatcher.h"
 #include "net/tools/quic/quic_simple_per_connection_packet_writer.h"
 #include "net/tools/quic/quic_simple_server_packet_writer.h"
 #include "net/tools/quic/quic_simple_server_session_helper.h"
 #include "net/udp/udp_server_socket.h"
 
 namespace net {
 
 namespace {
 
 const char kSourceAddressTokenSecret[] = "secret";
+const size_t kNumSessionsToCreatePerSocketEvent = 16;
 
 // Allocate some extra space so we can send an error if the client goes over
 // the limit.
 const int kReadBufferSize = 2 * kMaxPacketSize;
 
 }  // namespace
 
 QuicSimpleServer::QuicSimpleServer(
     std::unique_ptr<ProofSource> proof_source,
     const QuicConfig& config,
     const QuicCryptoServerConfig::ConfigOptions& crypto_config_options,
     const QuicVersionVector& supported_versions)
     : version_manager_(supported_versions),
       helper_(
           new QuicChromiumConnectionHelper(&clock_, QuicRandom::GetInstance())),
       alarm_factory_(new QuicChromiumAlarmFactory(
           base::ThreadTaskRunnerHandle::Get().get(),
           &clock_)),
       config_(config),
       crypto_config_options_(crypto_config_options),
       crypto_config_(kSourceAddressTokenSecret,
                      QuicRandom::GetInstance(),
                      std::move(proof_source)),
       read_pending_(false),
       synchronous_read_count_(0),
       read_buffer_(new IOBufferWithSize(kReadBufferSize)),
+      check_packet_buffer_(true),
       weak_factory_(this) {
   Initialize();
 }
 
 void QuicSimpleServer::Initialize() {
 #if MMSG_MORE
   use_recvmmsg_ = true;
 #endif
 
   // If an initial flow control window has not explicitly been set, then use a
@@ skipping 79 matching lines @@
   socket_->Close();
   socket_.reset();
 }
 
 void QuicSimpleServer::StartReading() {
   if (read_pending_) {
     return;
   }
   read_pending_ = true;
 
+  if (check_packet_buffer_) {
+    dispatcher_->ProcessBufferedChlos(kNumSessionsToCreatePerSocketEvent);
+  }
+
+  check_packet_buffer_ = false;
   int result = socket_->RecvFrom(
       read_buffer_.get(), read_buffer_->size(), &client_address_,
       base::Bind(&QuicSimpleServer::OnReadComplete, base::Unretained(this)));
 
   if (result == ERR_IO_PENDING) {
     synchronous_read_count_ = 0;
+    if (dispatcher_->HasChlosBuffered()) {
+      // No more packets to read, but still keep processing buffered packets if
+      // there is any.
+      read_pending_ = false;

[Ryan Hamilton, 2016/09/06 21:18:33]

This looks like a bug. read_pending_ should be true until OnReadComplete() is
called. Otherwise we can end up with multiple reads happening at once.

+      check_packet_buffer_ = true;
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE, base::Bind(&QuicSimpleServer::StartReading,
+                                weak_factory_.GetWeakPtr()));

[Ryan Hamilton, 2016/09/06 21:18:33]

I don't think this is the right method to call. This will end up issuing another
read to the socket, which will crash. 

I think you're going to need to split reading into two phases:

1) ProcessBufferedPackets
2) ReadPacketsFrom the network

When we're done processing buffered packets, if there are still more buffered
packets, we can post a task to process more buffered packets. When we've hit
ERR_IO_PENDING, we can post a task to process more buffered packets.

But we can not read more packets from the network until the last read completed
because of how chrome sockets work.

+    }
     return;
   }
 
   if (++synchronous_read_count_ > 32) {
     synchronous_read_count_ = 0;
     // Schedule the processing through the message loop to 1) prevent infinite
     // recursion and 2) avoid blocking the thread for too long.
+    check_packet_buffer_ = true;
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(&QuicSimpleServer::OnReadComplete,
                               weak_factory_.GetWeakPtr(), result));
   } else {
+    // No need to check buffer befor following read.

[Ryan Hamilton, 2016/09/06 21:18:33]

nit: "before" not "befor"

That being said, I'm sure sure that this comment add value. We've just read a
packet and need to process it, so calling OnReadComplete seems like the right
choice. It's not obvious to me why we might consider checking the buffer here.
(If we needed to do that, I would think we would have done that before calling
read)

     OnReadComplete(result);
   }
 }
 
 void QuicSimpleServer::OnReadComplete(int result) {
   read_pending_ = false;
   if (result == 0)
     result = ERR_CONNECTION_CLOSED;
 
   if (result < 0) {
     LOG(ERROR) << "QuicSimpleServer read failed: " << ErrorToString(result);
     Shutdown();
     return;
   }
 
   QuicReceivedPacket packet(read_buffer_->data(), result,
                             helper_->GetClock()->Now(), false);
   dispatcher_->ProcessPacket(server_address_, client_address_, packet);
 
   StartReading();
 }
 
 }  // namespace net