relnote: change write result handling for connectivity probing packet:

- connection will not be closed if there's any packet write error when
  sending connectivity probes as those are sent on a different path;
- connection will not be blocked if the connectivity probe is sent by a
  non-default packet writer.
Flag protected by quic_handle_write_results_for_connectivity_probe.

Manually merge internal change: 189676976.

Change-Id: Ie0389f3342365ed93d1c12c414e90a5118a004de
Reviewed-on: https://chromium-review.googlesource.com/972349Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Zhongyi Shi <zhongyi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544809}

net/quic/core/quic_connection.cc

@@ -294,7 +294,9 @@ QuicConnection::QuicConnection(
       negotiate_version_early_(
           GetQuicReloadableFlag(quic_server_early_version_negotiation)),
       always_discard_packets_after_close_(
-          GetQuicReloadableFlag(quic_always_discard_packets_after_close)) {
+          GetQuicReloadableFlag(quic_always_discard_packets_after_close)),
+      handle_write_results_for_connectivity_probe_(GetQuicReloadableFlag(
+          quic_handle_write_results_for_connectivity_probe)) {
   QUIC_DLOG(INFO) << ENDPOINT
                   << "Created connection with connection_id: " << connection_id
                   << " and version: "
@@ -2688,7 +2690,20 @@ bool QuicConnection::SendConnectivityProbingPacket(
   DCHECK(probing_writer);
 
   if (probing_writer->IsWriteBlocked()) {
-    QUIC_DLOG(INFO) << "Writer blocked when send connectivity probing packet";
+    QUIC_DLOG(INFO) << ENDPOINT
+                    << "Writer blocked when send connectivity probing packet.";
+    if (!handle_write_results_for_connectivity_probe_) {
+      visitor_->OnWriteBlocked();
+    } else {
+      QUIC_FLAG_COUNT_N(
+          quic_reloadable_flag_quic_handle_write_results_for_connectivity_probe,
+          1, 3);
+      if (probing_writer == writer_) {
+        // Visitor should not be write blocked if the probing writer is not the
+        // default packet writer.
+        visitor_->OnWriteBlocked();
+      }
+    }
     return true;
   }
 
@@ -2713,22 +2728,41 @@ bool QuicConnection::SendConnectivityProbingPacket(
       self_address().host(), peer_address, per_packet_options_);
 
   if (IsWriteError(result.status)) {
-    QUIC_DLOG(INFO) << "Write probing packet not finished with error = "
+    if (!handle_write_results_for_connectivity_probe_) {
+      OnWriteError(result.error_code);
+    } else {
+      QUIC_FLAG_COUNT_N(
+          quic_reloadable_flag_quic_handle_write_results_for_connectivity_probe,
+          2, 3);
+      // Write error for any connectivity probe should not affect the connection
+      // as it is sent on a different path.
+    }
+    QUIC_DLOG(INFO) << ENDPOINT << "Write probing packet failed with error = "
                     << result.error_code;
     return false;
   }
 
-  // Call OnPacketSent regardless of the write result. This treats a blocked
-  // write the same as a packet loss.
+  // Call OnPacketSent regardless of the write result.
   sent_packet_manager_.OnPacketSent(
       probing_packet.get(), probing_packet->original_packet_number,
       packet_send_time, probing_packet->transmission_type,
       NO_RETRANSMITTABLE_DATA);
 
   if (result.status == WRITE_STATUS_BLOCKED) {
-    visitor_->OnWriteBlocked();
+    if (!handle_write_results_for_connectivity_probe_) {
+      visitor_->OnWriteBlocked();
+    } else {
+      QUIC_FLAG_COUNT_N(
+          quic_reloadable_flag_quic_handle_write_results_for_connectivity_probe,
+          3, 3);
+      if (probing_writer == writer_) {
+        // Visitor should not be write blocked if the probing writer is not the
+        // default packet writer.
+        visitor_->OnWriteBlocked();
+      }
+    }
     if (probing_writer->IsWriteBlockedDataBuffered()) {
-      QUIC_BUG << "Write probing packet blocked";
+      QUIC_DLOG(INFO) << ENDPOINT << "Write probing packet blocked";
     }
   }
 

net/quic/core/quic_connection.h

@@ -1210,6 +1210,10 @@ class QUIC_EXPORT_PRIVATE QuicConnection
   // quic_reloadable_flag_quic_always_discard_packets_after_close.
   const bool always_discard_packets_after_close_;
 
+  // Latched valure of
+  // quic_reloadable_flag_quic_handle_write_results_for_connectivity_probe.
+  const bool handle_write_results_for_connectivity_probe_;
+
   DISALLOW_COPY_AND_ASSIGN(QuicConnection);
 };
 

net/quic/core/quic_connection_test.cc

@@ -5033,6 +5033,77 @@ TEST_P(QuicConnectionTest, SendConnectivityProbingWhenDisconnected) {
   }
 }
 
+TEST_P(QuicConnectionTest, WriteBlockedAfterClientSendsConnectivityProbe) {
+  EXPECT_EQ(Perspective::IS_CLIENT, connection_.perspective());
+  TestPacketWriter probing_writer(version(), &clock_);
+  // Block next write so that sending connectivity probe will encounter a
+  // blocked write when send a connectivity probe to the peer.
+  probing_writer.BlockOnNextWrite();
+  if (GetQuicReloadableFlag(quic_handle_write_results_for_connectivity_probe)) {
+    // Connection will not be marked as write blocked as connectivity probe only
+    // affects the probing_writer which is not the default.
+    EXPECT_CALL(visitor_, OnWriteBlocked()).Times(0);
+  } else {
+    EXPECT_CALL(visitor_, OnWriteBlocked()).Times(1);
+  }
+
+  EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, 1, _, _)).Times(1);
+  connection_.SendConnectivityProbingPacket(&probing_writer,
+                                            connection_.peer_address());
+}
+
+TEST_P(QuicConnectionTest, WriterBlockedAfterServerSendsConnectivityProbe) {
+  set_perspective(Perspective::IS_SERVER);
+  QuicPacketCreatorPeer::SetSendVersionInPacket(creator_, false);
+
+  // Block next write so that sending connectivity probe will encounter a
+  // blocked write when send a connectivity probe to the peer.
+  writer_->BlockOnNextWrite();
+  // Connection will be marked as write blocked as server uses the default
+  // writer to send connectivity probes.
+  EXPECT_CALL(visitor_, OnWriteBlocked()).Times(1);
+
+  EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, 1, _, _)).Times(1);
+  connection_.SendConnectivityProbingPacket(writer_.get(),
+                                            connection_.peer_address());
+}
+
+TEST_P(QuicConnectionTest, WriterErrorWhenClientSendsConnectivityProbe) {
+  EXPECT_EQ(Perspective::IS_CLIENT, connection_.perspective());
+  TestPacketWriter probing_writer(version(), &clock_);
+  probing_writer.SetShouldWriteFail();
+
+  if (GetQuicReloadableFlag(quic_handle_write_results_for_connectivity_probe)) {
+    // Connection should not be closed if a connectivity probe is failed to be
+    // sent.
+    EXPECT_CALL(visitor_, OnConnectionClosed(_, _, _)).Times(0);
+  } else {
+    EXPECT_CALL(visitor_, OnConnectionClosed(_, _, _)).Times(1);
+  }
+
+  EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, 1, _, _)).Times(0);
+  connection_.SendConnectivityProbingPacket(&probing_writer,
+                                            connection_.peer_address());
+}
+
+TEST_P(QuicConnectionTest, WriterErrorWhenServerSendsConnectivityProbe) {
+  set_perspective(Perspective::IS_SERVER);
+  QuicPacketCreatorPeer::SetSendVersionInPacket(creator_, false);
+
+  writer_->SetShouldWriteFail();
+  if (GetQuicReloadableFlag(quic_handle_write_results_for_connectivity_probe)) {
+    // Connection should not be closed if a connectivity probe is failed to be
+    // sent.
+    EXPECT_CALL(visitor_, OnConnectionClosed(_, _, _)).Times(0);
+  } else {
+    EXPECT_CALL(visitor_, OnConnectionClosed(_, _, _)).Times(1);
+  }
+
+  EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, 1, _, _)).Times(0);
+  connection_.SendConnectivityProbingPacket(writer_.get(),
+                                            connection_.peer_address());
+}
+
 TEST_P(QuicConnectionTest, PublicReset) {
   QuicPublicResetPacket header;
   // Public reset packet in only built by server.

net/quic/core/quic_flags_list.h

@@ -222,3 +222,11 @@ QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_remove_redundant_ping, false)
 QUIC_FLAG(bool,
           FLAGS_quic_reloadable_flag_quic_reset_stream_is_not_zombie,
           true)
+
+// If true, when a packet write for connectivity probe does not complete
+// successfully synchronously, connection will not be affected, i.e., blocked or
+// closed, if the probing packet writer is not the default writer.
+QUIC_FLAG(
+    bool,
+    FLAGS_quic_reloadable_flag_quic_handle_write_results_for_connectivity_probe,
+    true)