Makes migration on write error asynchronous to avoid reentrancy issues

net/quic/chromium/quic_chromium_client_session.cc

Index: net/quic/chromium/quic_chromium_client_session.cc
diff --git a/net/quic/chromium/quic_chromium_client_session.cc b/net/quic/chromium/quic_chromium_client_session.cc
index 258b0c6b4eac7cfb2498a95f5b29fc00dbc2a388..8d6f81d8fb8d0038e0672827ac23469c4210f894 100644
--- a/net/quic/chromium/quic_chromium_client_session.cc
+++ b/net/quic/chromium/quic_chromium_client_session.cc
@@ -237,8 +237,7 @@ QuicChromiumClientSession::QuicChromiumClientSession(
       token_binding_signatures_(kTokenBindingSignatureMapSize),
       streams_pushed_count_(0),
       streams_pushed_and_claimed_count_(0),
-      error_code_from_rewrite_(OK),
-      use_error_code_from_rewrite_(false),
+      packet_(nullptr),

[Ryan Hamilton, 2016/09/10 16:29:47]

nit: no need for this as the default constructor will do this.

[Jana, 2016/09/10 23:08:31]

Done.

       weak_factory_(this) {
   sockets_.push_back(std::move(socket));
   packet_readers_.push_back(base::WrapUnique(new QuicChromiumPacketReader(
@@ -952,11 +951,61 @@ int QuicChromiumClientSession::HandleWriteError(
     int error_code,
     scoped_refptr<StringIOBuffer> packet) {
   DCHECK(packet != nullptr);
-  use_error_code_from_rewrite_ = false;
-  if (stream_factory_) {
-    stream_factory_->MaybeMigrateSingleSession(this, WRITE_ERROR, packet);
+  DCHECK_NE(ERR_IO_PENDING, error_code);
+  DCHECK_GT(0, error_code);
+
+  // Post a task to migrate the session onto a new network.
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&QuicChromiumClientSession::MigrateSessionOnWriteError,
+                 weak_factory_.GetWeakPtr()));
+
+  // Store packet in the session since the actual migration and packet rewrite
+  // can happen via this posted task or via an async network notification.
+  packet_ = packet;
+
+  // Cause the packet writer to return ERR_IO_PENDING and block so
+  // that there are no write errors due to subsequent writes while the
+  // session tries migrating to a new writer/socket.

[Ryan Hamilton, 2016/09/10 16:29:46]

Since the actual write happen after yet another PostTask, this comment is a bit
inaccurate. I think it might be more clear to say something like:

// Cause the packet writer to return ERR_IO_PENDING and block so
// that the actual migration happens from the message loop instead
// of under the call stack of QuicConnection::WritePacket.

(or some such)

[Jana, 2016/09/10 23:08:30]

Much more accurate -- done.

+  return ERR_IO_PENDING;
+}
+
+void QuicChromiumClientSession::MigrateSessionOnWriteError() {
+  if (packet_ == nullptr)
+    // If packet_ no longer exists, it must have been written on a
+    // different migration attempt. Do not attempt another migration.

[Ryan Hamilton, 2016/09/10 16:29:46]

nit: As written, the body of this if is 3 lines lone, which requires {}s. But
you can move the comments to above the if, and it'll still read cleanly and then
you can avoid the {}s.

[Jana, 2016/09/10 23:08:30]

I assumed that comments didn't count. It's about readability though so makes
sense that comments would. Fixed.

+    return;
+
+  if (stream_factory_ != nullptr &&
+      stream_factory_->MaybeMigrateSingleSession(this, WRITE_ERROR) ==
+          MigrationResult::SUCCESS)
+    return;
+
+  // Close the connection if migration failed. Do not cause a
+  // connection close packet to be sent since socket may be borked.
+  connection()->CloseConnection(QUIC_PACKET_WRITE_ERROR,
+                                "Write and subsequent migration failed",
+                                ConnectionCloseBehavior::SILENT_CLOSE);
+}
+
+void QuicChromiumClientSession::WriteToNewSocket(
+    scoped_refptr<StringIOBuffer> packet) {
+  if (packet == nullptr) {
+    connection()->SendPing();
+    return;
+  }

[Ryan Hamilton, 2016/09/10 16:29:46]

Under what circumstances is this code path hit? Can it happen from write error?
If so, as far as the connection is concerned, there is a write outstanding and
the writer is write blocked. (Or at least it was when the original write error
occurred). At this point, the connection is hanging around waiting for
OnCanWrite to resume the write path. By calling SendPing here, we're actually
causing a write in a place where the connection isn't expecting it. It might all
work correctly, but it feels strange. Perhaps this should do
connection->OnCanWrite() before sending the ping?

[Jana, 2016/09/10 23:08:31]

You're partially right that you can hit this code via the write error path, but
there's an important subtlety. 

This method can be called via a write error or via NCN events. If there's only
an NCN event that leads here, packet must be null and the connection is not
write blocked.  If there's only a write error that leads here, then packet must
be non-null, since  HandleWriteError stores a packet in the packet_ member. The
packet_ member is cleared in two cases: (i) after we post a task to call this
method with the packet, or (ii) num_migrations exceeded (see MigrateToSocket
below.)

If the WriteError event and an NCN event are interleaved, leading to two
WriteToNewSockets being posted and executed, it doesn't matter which
WriteToNewSocket occurs first. The first one will rewrite packet_ and unblock
the connection (or close on error), and the second one will send a PING packet.
(The PING is not necessary, and I think I can tighten this -- I'll take a shot.
But this only happens under a very specific race -- NCN event posts a write task
after MigrateSessionOnWriteError completes but before WriteToNewSocket executes
-- and the cost is that an unnecessary PING packet is sent.)

Make sense?

Ok, coming back to this after some mode thinking --  the code didn't quite do
what I said it did, but I've fixed it now to do precisely what I describe above.
The problem was that MigrateToSocket was posting a WriteToNewSocket with a
packet parameter. Now, if WriteToNewSocket was posted by an NCN event first
(with packet=nullptr), and then a write error occurred, causing the writer to be
blocked before the NCN-event-posted WriteToNewSocket is executed, we could end
up in the situation you describe. This is hard to arrange in the test because I
need to carefully crank the message loop to arrange for these events to happen
in the order I described. (I'm trying to write tests with all possible
interleavings... but I think I'll need to use the test runner and do this by
hand.)

I've changed the code to not post any packet parameter with WriteToNewSocket,
but for the method to decide on execution which packet to write. Meaning that
the packet_ member is populated at the beginning of the WriteError process, and
set to null at the end of it, in WriteToNewSocket. If an NCN-triggered
WriteToNewSocket shows up during this process, it will do the right thing of
writing packet_ instead of sending a ping.

[Jana, 2016/09/11 05:30:46]

Ok, I've simplified and removed the possibility of multiple tasks executing
WriteToNewSocket. The code now should now allow a write to proceed if a write
posted earlier executes.

+  WriteResult result =

[Ryan Hamilton, 2016/09/10 16:29:46]

Let's add a comment here to explain that the connection is waiting for the
original write to complete asynchronously and so this code has to propagate any
synchronous write results back to the connection.

[Jana, 2016/09/10 23:08:30]

Done.

+      static_cast<QuicChromiumPacketWriter*>(connection()->writer())
+          ->WritePacketToSocket(packet);
+
+  // If write completes synchronously, notify the connection. The writer
+  // notifies the connection on async completions.
+  if (result.error_code < 0 && result.error_code != ERR_IO_PENDING) {
+    connection()->OnWriteError(result.error_code);

[Ryan Hamilton, 2016/09/10 16:29:47]

In theory, we should never get a WriteError because HandleWriteError will make
that to WRITE_BLOCKED, right?

[Jana, 2016/09/10 23:08:31]

Correct. I see this in the tests on multiple write errors. I've turned it into a
DCHECK instead.

+    return;
   }
-  return use_error_code_from_rewrite_ ? error_code_from_rewrite_ : error_code;
+  if (result.error_code != ERR_IO_PENDING)
+    connection()->OnCanWrite();

[Ryan Hamilton, 2016/09/10 16:29:47]

nit: how about

if (result.error_code == ERR_IO_PENDING)
  return;

if (result.error_code < 0)
  connection()->OnWriteError(result.error_code);
else 
  connection()->OnCanWrite();

Not any fewer lines, but it avoid ERR_IO_PENDING being checked twice.

Alternatively, it might make sense to move this logic into the writer. Perhaps
we should have a helper method something like:

  ResendPacketAfterMigration(packet)

which calls WritePacketToSocket and then knows that it has to propogate sync
write results to the connection?

*shurg* Either way.

[Jana, 2016/09/10 23:08:31]

Done. I have a mild preference for it to stay here, since this method isn't
particularly long and I think it's more readable this way. With the DCHECK, I
think it belongs here.

 }
 
 void QuicChromiumClientSession::OnWriteError(int error_code) {
@@ -971,7 +1020,7 @@ void QuicChromiumClientSession::OnWriteUnblocked() {
 
 void QuicChromiumClientSession::OnPathDegrading() {
   if (stream_factory_) {
-    stream_factory_->MaybeMigrateSingleSession(this, EARLY_MIGRATION, nullptr);
+    stream_factory_->MaybeMigrateSingleSession(this, EARLY_MIGRATION);

[Ryan Hamilton, 2016/09/10 16:29:47]

Is it possible that this is called while a write is outstanding? If so, this
means that WriteToNewSocket can get called when there is no |packet| but while
the connection is waiting for a write to complete (and my earlier comment about
needing OnCanWrite() apply)

We don't yet have the "pause when no new network" code, right? When we do, will
we need (or want) to mark the writer write blocked?

[Jana, 2016/09/10 23:08:31]

This should be handled in the same way as NCN-migration now. There are
interactions between early and other migrations, but I'm leaving early migration
testing out until after this CL.

   }
 }
 
@@ -1178,25 +1227,28 @@ void QuicChromiumClientSession::NotifyFactoryOfSessionClosed() {
 bool QuicChromiumClientSession::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) {
   DCHECK_EQ(sockets_.size(), packet_readers_.size());
   if (sockets_.size() >= kMaxReadersPerQuicSession) {
+    packet_ = nullptr;
     return false;
   }
+
   // TODO(jri): Make SetQuicPacketWriter take a scoped_ptr.
   packet_readers_.push_back(std::move(reader));
   sockets_.push_back(std::move(socket));
   StartReading();
-  QuicChromiumPacketWriter* raw_writer = writer.get();
   connection()->SetQuicPacketWriter(writer.release(), /*owns_writer=*/true);
-  if (packet == nullptr) {
-    connection()->SendPing();
-    return true;
-  }
-  // Packet rewrite after migration on socket write error.
-  error_code_from_rewrite_ = raw_writer->WritePacketToSocket(packet.get());
-  use_error_code_from_rewrite_ = true;
+
+  // Post task to write the pending packet or a PING packet to the new socket.
+  task_runner_->PostTask(
+      FROM_HERE, base::Bind(&QuicChromiumClientSession::WriteToNewSocket,
+                            weak_factory_.GetWeakPtr(), packet_));

[Ryan Hamilton, 2016/09/10 16:29:46]

Since we're not writing immediately, I think we need to make sure the new writer
is write blocked so that nothing else tries to write before we're get around to
resending |packet_| otherwise we might call WritePacketToNewSocket() while a
write is outstanding.

[Jana, 2016/09/10 23:08:31]

Good point. I'll add a ShouldWriteBlock to the delegate. Done.

+
+  // Reset packet_ since (i) the posted write can result in a write
+  // error, causing packet_ to be used again, and (ii) avoids a later
+  // migration attempt if any pending migration task is executed.
+  packet_ = nullptr;

[Ryan Hamilton, 2016/09/10 16:29:47]

Is (i) actually correct? If I understand correctly, packet_ won't need to be
reused until it's been written, right? (So it could be cleared in
WriteToNewSocket).

[Jana, 2016/09/10 23:08:31]

Right, I think the comment wasn't clear -- but I've moved all this to in
WriteToNewSocket now.

   return true;
 }