Refactoring: Create per-connection packet writers in QuicDispatcher.

net/quic/quic_dispatcher.h

 // 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.
 //
 // A server side dispatcher which dispatches a given client's data to their
 // stream.
 
 #ifndef NET_QUIC_QUIC_DISPATCHER_H_
 #define NET_QUIC_QUIC_DISPATCHER_H_
 
 #include <list>
 
 #include "base/basictypes.h"
 #include "base/containers/hash_tables.h"
 #include "base/memory/scoped_ptr.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/linked_hash_map.h"
 #include "net/quic/quic_blocked_writer_interface.h"
 #include "net/quic/quic_connection_helper.h"
 #include "net/quic/quic_protocol.h"
 #include "net/quic/quic_server_packet_writer.h"
 #include "net/quic/quic_server_session.h"
 #include "net/quic/quic_time_wait_list_manager.h"
 
-#if defined(COMPILER_GCC)
+#if defined(COMPILER_GCC) && !defined(QUIC_BLOCKED_WRITER_INTERFACE_HASH)
+#define QUIC_BLOCKED_WRITER_INTERFACE_HASH

[wtc, 2014/08/13 21:01:34]

IMPORTANT: this change is not ideal. We should try to find a better solution.

After looking into this, I suggest we move this hash<> template specialization
into the net/quic/quic_blocked_writer_interface.h header. This seems to
recommended by the comment at the top of base/containers/hash_tables.h:

// NOTE: It is an explicit non-goal of this class to provide a generic hash
// function for pointers.  If you want to hash a pointers to a particular class,
// please define the template specialization elsewhere (for example, in its
// header file) and keep it specific to just pointers to that class.  This is
// because identity hashes are not desirable for all types that might show up
// in containers as pointers.

[dmz, 2014/08/14 17:22:42]

Done.

 namespace BASE_HASH_NAMESPACE {
 template <>
 struct hash<net::QuicBlockedWriterInterface*> {
   std::size_t operator()(const net::QuicBlockedWriterInterface* ptr) const {
     return hash<size_t>()(reinterpret_cast<size_t>(ptr));

[wtc, 2014/08/13 21:01:34]

Nit: we may want to switch to this implementation from the internal tree:

// Hash pointers as if they were int's, but bring more entropy to
// the lower bits.
template <>
struct hash<net::QuicBlockedWriterInterface*> {
  std::size_t operator()(const net::QuicBlockedWriterInterface* ptr) const {
    size_t k = reinterpret_cast<size_t>(ptr);
    return k + (k >> 6);
  }
};

The rationale is:

  ... pointers can have low entropy in the least significant bits, and
  those bits are important for efficiency in some uses ...

which seems reasonable to me.

[dmz, 2014/08/14 17:22:41]

Done.

   }
 };
 }
 #endif
 
 namespace net {
 
 class QuicConfig;
 class QuicCryptoServerConfig;
 class QuicSession;
 
 namespace test {
 class QuicDispatcherPeer;
 }  // namespace test
 
 class DeleteSessionsAlarm;
 
 class ProcessPacketInterface {
  public:
   virtual ~ProcessPacketInterface() {}
   virtual void ProcessPacket(const IPEndPoint& server_address,
                              const IPEndPoint& client_address,
                              const QuicEncryptedPacket& packet) = 0;
 };
 
 class QuicDispatcher : public QuicBlockedWriterInterface,
                        public QuicServerSessionVisitor,
                        public ProcessPacketInterface {
  public:
+  // Creates per-connection packet writers out of the QuicDispatcher's shared
+  // QuicPacketWriter. The per-connection writers' IsWriteBlocked() state must
+  // always be the same as the shared writer's IsWriteBlocked(), or else the
+  // QuicDispatcher::OnCanWrite logic will not work. (This will hopefully be
+  // cleaned up for bug 16950226.)
+  class PacketWriterFactory {
+   public:
+    virtual ~PacketWriterFactory() {}
+
+    virtual QuicPacketWriter* Create(QuicServerPacketWriter* writer,

[wtc, 2014/08/13 21:01:34]

IMPORTANT: please confirm that it is correct for these two Create() methods to
take a QuicServerPacketWriter* input and returns a QuicPacketWriter*. The
original code from the internal CL takes a QuicPacketWriter*.

[dmz, 2014/08/14 17:22:42]

Yes, that's right.

+                                     QuicConnection* connection) = 0;
+  };
+
+  // Creates ordinary QuicPerConnectionPacketWriter instances.
+  class DefaultPacketWriterFactory : public PacketWriterFactory {
+   public:
+    virtual ~DefaultPacketWriterFactory() {}
+
+    virtual QuicPacketWriter* Create(
+        QuicServerPacketWriter* writer,
+        QuicConnection* connection) OVERRIDE;
+  };
+
   // Ideally we'd have a linked_hash_set: the  boolean is unused.
   typedef linked_hash_map<QuicBlockedWriterInterface*, bool> WriteBlockedList;
 
-  // Due to the way delete_sessions_closure_ is registered, the Dispatcher
-  // must live until epoll_server Shutdown. |supported_versions| specifies the
-  // list of supported QUIC versions.
+  // Due to the way delete_sessions_closure_ is registered, the Dispatcher must
+  // live until epoll_server Shutdown. |supported_versions| specifies the list
+  // of supported QUIC versions. Takes ownership of |packet_writer_factory|,
+  // which is used to create per-connection writers.
   QuicDispatcher(const QuicConfig& config,
                  const QuicCryptoServerConfig& crypto_config,
                  const QuicVersionVector& supported_versions,
+                 PacketWriterFactory* packet_writer_factory,
                  QuicConnectionHelperInterface* helper);
 
   virtual ~QuicDispatcher();
 
   // Takes ownership of the packet writer
   virtual void Initialize(QuicServerPacketWriter* writer);
 
   // Process the incoming packet by creating a new session, passing it to
   // an existing session, or passing it to the TimeWaitListManager.
   virtual void ProcessPacket(const IPEndPoint& server_address,
@@ skipping 27 matching lines @@
   const SessionMap& session_map() const { return session_map_; }
 
  protected:
   virtual QuicSession* CreateQuicSession(QuicConnectionId connection_id,
                                          const IPEndPoint& server_address,
                                          const IPEndPoint& client_address);
 
   virtual QuicConnection* CreateQuicConnection(
       QuicConnectionId connection_id,
       const IPEndPoint& server_address,
-      const IPEndPoint& client_address,
-      QuicPerConnectionPacketWriter* writer);
+      const IPEndPoint& client_address);
 
   // Called by |framer_visitor_| when the public header has been parsed.
   virtual bool OnUnauthenticatedPublicHeader(
       const QuicPacketPublicHeader& header);
 
   // Create and return the time wait list manager for this dispatcher, which
   // will be owned by the dispatcher as time_wait_list_manager_
   virtual QuicTimeWaitListManager* CreateQuicTimeWaitListManager();
 
   // Replaces the packet writer with |writer|. Takes ownership of |writer|.
@@ skipping 22 matching lines @@
   const QuicConfig& config() const { return config_; }
 
   const QuicCryptoServerConfig& crypto_config() const { return crypto_config_; }
 
   QuicFramer* framer() { return &framer_; }
 
   QuicConnectionHelperInterface* helper() { return helper_; }
 
   QuicServerPacketWriter* writer() { return writer_.get(); }
 
+  const QuicConnection::PacketWriterFactory& connection_writer_factory() {
+    return connection_writer_factory_;
+  }
+
  private:
   class QuicFramerVisitor;
   friend class net::test::QuicDispatcherPeer;
 
+  // An adapter that creates packet writers using the dispatcher's
+  // PacketWriterFactory and shared writer. Essentially, it just curries the
+  // writer argument away from QuicDispatcher::PacketWriterFactory.
+  class PacketWriterFactoryAdapter :
+    public QuicConnection::PacketWriterFactory {
+   public:
+    PacketWriterFactoryAdapter(QuicDispatcher* dispatcher);
+    virtual ~PacketWriterFactoryAdapter ();
+
+    virtual QuicPacketWriter* Create(QuicConnection* connection) const OVERRIDE;
+
+   private:
+    QuicDispatcher* dispatcher_;
+  };
+
   // Called by |framer_visitor_| when the private header has been parsed
   // of a data packet that is destined for the time wait manager.
   void OnUnauthenticatedHeader(const QuicPacketHeader& header);
 
   // Removes the session from the session map and write blocked list, and
   // adds the ConnectionId to the time-wait list.
   void CleanUpSession(SessionMap::iterator it);
 
   bool HandlePacketForTimeWait(const QuicPacketPublicHeader& header);
 
@@ skipping 14 matching lines @@
 
   // An alarm which deletes closed sessions.
   scoped_ptr<QuicAlarm> delete_sessions_alarm_;
 
   // The list of closed but not-yet-deleted sessions.
   std::list<QuicSession*> closed_session_list_;
 
   // The writer to write to the socket with.
   scoped_ptr<QuicServerPacketWriter> writer_;
 
+  // Used to create per-connection packet writers, not |writer_| itself.
+  scoped_ptr<PacketWriterFactory> packet_writer_factory_;
+
+  // Passed in to QuicConnection for it to create the per-connection writers
+  PacketWriterFactoryAdapter connection_writer_factory_;
+
   // This vector contains QUIC versions which we currently support.
   // This should be ordered such that the highest supported version is the first
   // element, with subsequent elements in descending order (versions can be
   // skipped as necessary).
   const QuicVersionVector supported_versions_;
 
   // Information about the packet currently being handled.
   IPEndPoint current_client_address_;
   IPEndPoint current_server_address_;
   const QuicEncryptedPacket* current_packet_;
 
   QuicFramer framer_;
   scoped_ptr<QuicFramerVisitor> framer_visitor_;
 
   DISALLOW_COPY_AND_ASSIGN(QuicDispatcher);
 };
 
 }  // namespace net
 
 #endif  // NET_QUIC_QUIC_DISPATCHER_H_