Makes migration on write error asynchronous to avoid reentrancy issues

net/quic/chromium/quic_chromium_client_session.h

 // 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.
 //
 // A client specific QuicSession subclass.  This class owns the underlying
 // QuicConnection and QuicConnectionHelper objects.  The connection stores
 // a non-owning pointer to the helper so this session needs to ensure that
 // the helper outlives the connection.
 
 #ifndef NET_QUIC_QUIC_CHROMIUM_CLIENT_SESSION_H_
@@ skipping 145 matching lines @@
                       QuicChromiumClientStream** stream);
 
   // Cancels the pending stream creation request.
   void CancelRequest(StreamRequest* request);
 
   // QuicChromiumPacketWriter::Delegate override.
   int HandleWriteError(int error_code,
                        scoped_refptr<StringIOBuffer> last_packet) override;
   void OnWriteError(int error_code) override;
   void OnWriteUnblocked() override;
+  bool ShouldWriteBlock() override;
 
   // QuicSpdySession methods:
   void OnHeadersHeadOfLineBlocking(QuicTime::Delta delta) override;
 
   // QuicSession methods:
   void OnStreamFrame(const QuicStreamFrame& frame) override;
   QuicChromiumClientStream* CreateOutgoingDynamicStream(
       SpdyPriority priority) override;
   QuicCryptoClientStream* GetCryptoStream() override;
   void CloseStream(QuicStreamId stream_id) override;
@@ skipping 70 matching lines @@
 
   // Returns true if |hostname| may be pooled onto this session.  If this
   // is a secure QUIC session, then |hostname| must match the certificate
   // presented during the handshake.
   bool CanPool(const std::string& hostname, PrivacyMode privacy_mode) const;
 
   const QuicServerId& server_id() const { return server_id_; }
 
   QuicDisabledReason disabled_reason() const { return disabled_reason_; }
 
+  // Attempts to migrate session when a write error is encountered.
+  void MigrateSessionOnWriteError();
+
+  // Helper method that writes a packet on the new socket after
+  // migration completes. If not null, the packet_ member is written,
+  // else a PING packet is written.
+  void WriteToNewSocket();
+
   // Migrates session onto new socket, i.e., starts reading from
   // |socket| in addition to any previous sockets, and sets |writer|
   // to be the new default writer. Returns true if socket was
   // successfully added to the session and the session was
-  // successfully migrated to using the new socket. If not null,
-  // |packet| is sent on the new network, else a PING frame is
-  // sent. Returns true on successful migration, or false if number of
-  // migrations exceeds kMaxReadersPerQuicSession.  Takes ownership of
-  // |socket|, |reader|, and |writer|.
+  // successfully migrated to using the new socket. Returns true on
+  // successful migration, or false if number of migrations exceeds
+  // kMaxReadersPerQuicSession. Takes ownership of |socket|, |reader|,
+  // and |writer|.
   bool MigrateToSocket(std::unique_ptr<DatagramClientSocket> socket,
                        std::unique_ptr<QuicChromiumPacketReader> reader,
-                       std::unique_ptr<QuicChromiumPacketWriter> writer,
-                       scoped_refptr<StringIOBuffer> packet);
+                       std::unique_ptr<QuicChromiumPacketWriter> writer);
 
   // Populates network error details for this session.
   void PopulateNetErrorDetails(NetErrorDetails* details);
 
   // Returns current default socket. This is the socket over which all
   // QUIC packets are sent. This default socket can change, so do not store the
   // returned socket.
   const DatagramClientSocket* GetDefaultSocket() const;
 
   bool IsAuthorized(const std::string& hostname) override;
@@ skipping 79 matching lines @@
   // True when the session is going away, and streams may no longer be created
   // on this session. Existing stream will continue to be processed.
   bool going_away_;
   // True when the session receives a go away from server due to port migration.
   bool port_migration_detected_;
   QuicDisabledReason disabled_reason_;
   TokenBindingSignatureMap token_binding_signatures_;
   // UMA histogram counters for streams pushed to this session.
   int streams_pushed_count_;
   int streams_pushed_and_claimed_count_;
-  // Return value from packet rewrite packet on new socket. Used
-  // during connection migration on socket write error.
-  int error_code_from_rewrite_;
-  bool use_error_code_from_rewrite_;
+  // Stores packet that witnesses socket write error. This packet is
+  // written to a new socket after migration completes.
+  scoped_refptr<StringIOBuffer> packet_;
+  bool migration_pending_;   // True while migration is underway.
+  bool should_write_block_;  // True if the writer should write block.

[Ryan Hamilton, 2016/09/11 15:38:42]

Instead of doings this, it might just be easier to call set_write_blocked on the
writer?

[Jana, 2016/09/11 18:32:30]

Given that the delegate is part of the process anyways for the writer, the
current interaction doesn't seem unnatural... I'm inclined to leave it as is
because the writer is using the delegate to block and all delegate logic is in
the session. Though if you feel strongly, I'm happy to change it.

[Ryan Hamilton, 2016/09/11 20:44:24]

That's a fair point. I think set_write_blocked() might be better (after noodling
about it for a while) for a few reasons. 1) if we already have set_write_blocked
we dont' really need a delegate method. 2) it requires even more state in the
session (which is full of state already :>). 3) ShouldWriteBlock() might (via a
bug) be intended to apply to writer A but we swap it out and it ends up applying
with writer B.

This is minor in the grand scheme of things, but with all the exciting
complications of migration, I think it'd be slightly cleaner. 

[Jana, 2016/09/11 21:37:40]

Fair enough. Done.

   base::WeakPtrFactory<QuicChromiumClientSession> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(QuicChromiumClientSession);
 };
 
 }  // namespace net
 
 #endif  // NET_QUIC_QUIC_CHROMIUM_CLIENT_SESSION_H_