commit f54da366a84db70d2aa6a747b91f3e30b8e8df00
author Viet-Trung Luu <viettrungluu@chromium.org> Tue Jun 07 17:15:25 2016 -0700
committer Viet-Trung Luu <viettrungluu@chromium.org> Tue Jun 07 17:15:25 2016 -0700
tree 9f0774901c06adf94fcf58c1519b45221ee360c8
parent 86aafcbd2e56b2b7277d1b4a84713631ffa54cbd

Make the data pipe "cancel all state" functions cancel two-phase operations.

Refactor some code paths to share more code. This leads to (very slight)
redundancy in a couple of code paths, because on closing a handle both
"cancel all state" and "close" get called.

See the commit message for c59a3473231b17af071fc401fdd66ec0adbcdd4c for
details about why this is being done. This CL doesn't actually change
any visible behavior.

R=azani@chromium.org
BUG=#782

Review URL: https://codereview.chromium.org/2045003002 .

mojo/edk/system/data_pipe.cc

diff --git a/mojo/edk/system/data_pipe.cc b/mojo/edk/system/data_pipe.cc
index 7657f64..d671a76 100644
--- a/mojo/edk/system/data_pipe.cc
+++ b/mojo/edk/system/data_pipe.cc
@@ -272,8 +272,7 @@
 
 void DataPipe::ProducerCancelAllState() {
   MutexLocker locker(&mutex_);
-  DCHECK(has_local_producer_no_lock());
-  producer_awakable_list_->CancelAll();
+  ProducerCancelAllStateNoLock();
 }
 
 void DataPipe::ProducerClose() {
@@ -450,15 +449,8 @@
   bool rv = impl_->ProducerEndSerialize(channel, destination, actual_size,
                                         platform_handles);
 
-  // TODO(vtl): The code below is similar to, but not quite the same as,
-  // |ProducerCloseNoLock()|.
-  DCHECK(has_local_producer_no_lock());
-  producer_awakable_list_->CancelAll();
+  ProducerCancelAllStateNoLock();
   producer_awakable_list_.reset();
-  // Not a bug, except possibly in "user" code.
-  DVLOG_IF(2, producer_in_two_phase_write_no_lock())
-      << "Producer transferred with active two-phase write";
-  producer_two_phase_max_num_bytes_written_ = 0;
   if (!has_local_consumer_no_lock())
     producer_open_ = false;
 
@@ -472,8 +464,7 @@
 
 void DataPipe::ConsumerCancelAllState() {
   MutexLocker locker(&mutex_);
-  DCHECK(has_local_consumer_no_lock());
-  consumer_awakable_list_->CancelAll();
+  ConsumerCancelAllStateNoLock();
 }
 
 void DataPipe::ConsumerClose() {
@@ -682,14 +673,8 @@
   bool rv = impl_->ConsumerEndSerialize(channel, destination, actual_size,
                                         platform_handles);
 
-  // TODO(vtl): The code below is similar to, but not quite the same as,
-  // |ConsumerCloseNoLock()|.
-  consumer_awakable_list_->CancelAll();
+  ConsumerCancelAllStateNoLock();
   consumer_awakable_list_.reset();
-  // Not a bug, except possibly in "user" code.
-  DVLOG_IF(2, consumer_in_two_phase_read_no_lock())
-      << "Consumer transferred with active two-phase read";
-  consumer_two_phase_max_num_bytes_read_ = 0;
   if (!has_local_producer_no_lock())
     consumer_open_ = false;
 
@@ -747,6 +732,26 @@
   return rv;
 }
 
+void DataPipe::ProducerCancelAllStateNoLock() {
+  DCHECK(has_local_producer_no_lock());
+  if (producer_awakable_list_)
+    producer_awakable_list_->CancelAll();
+  // Not a bug, except possibly in "user" code.
+  DVLOG_IF(2, producer_in_two_phase_write_no_lock())
+      << "Active two-phase write cancelled";
+  producer_two_phase_max_num_bytes_written_ = 0;
+}
+
+void DataPipe::ConsumerCancelAllStateNoLock() {
+  DCHECK(has_local_consumer_no_lock());
+  if (consumer_awakable_list_)
+    consumer_awakable_list_->CancelAll();
+  // Not a bug, except possibly in "user" code.
+  DVLOG_IF(2, consumer_in_two_phase_read_no_lock())
+      << "Active two-phase read cancelled";
+  consumer_two_phase_max_num_bytes_read_ = 0;
+}
+
 void DataPipe::SetProducerClosedNoLock() {
   mutex_.AssertHeld();
   DCHECK(!has_local_producer_no_lock());
@@ -766,11 +771,8 @@
   DCHECK(producer_open_);
   producer_open_ = false;
   if (has_local_producer_no_lock()) {
+    ProducerCancelAllStateNoLock();
     producer_awakable_list_.reset();
-    // Not a bug, except possibly in "user" code.
-    DVLOG_IF(2, producer_in_two_phase_write_no_lock())
-        << "Producer closed with active two-phase write";
-    producer_two_phase_max_num_bytes_written_ = 0;
     impl_->ProducerClose();
     AwakeConsumerAwakablesForStateChangeNoLock(
         impl_->ConsumerGetHandleSignalsState());
@@ -782,11 +784,8 @@
   DCHECK(consumer_open_);
   consumer_open_ = false;
   if (has_local_consumer_no_lock()) {
+    ConsumerCancelAllStateNoLock();
     consumer_awakable_list_.reset();
-    // Not a bug, except possibly in "user" code.
-    DVLOG_IF(2, consumer_in_two_phase_read_no_lock())
-        << "Consumer closed with active two-phase read";
-    consumer_two_phase_max_num_bytes_read_ = 0;
     impl_->ConsumerClose();
     AwakeProducerAwakablesForStateChangeNoLock(
         impl_->ProducerGetHandleSignalsState());

mojo/edk/system/data_pipe.h

diff --git a/mojo/edk/system/data_pipe.h b/mojo/edk/system/data_pipe.h
index bdd298d..4437b76 100644
--- a/mojo/edk/system/data_pipe.h
+++ b/mojo/edk/system/data_pipe.h
@@ -174,6 +174,10 @@
   std::unique_ptr<DataPipeImpl> ReplaceImplNoLock(
       std::unique_ptr<DataPipeImpl> new_impl)
       MOJO_EXCLUSIVE_LOCKS_REQUIRED(mutex_);
+
+  void ProducerCancelAllStateNoLock() MOJO_EXCLUSIVE_LOCKS_REQUIRED(mutex_);
+  void ConsumerCancelAllStateNoLock() MOJO_EXCLUSIVE_LOCKS_REQUIRED(mutex_);
+
   void SetProducerClosedNoLock() MOJO_EXCLUSIVE_LOCKS_REQUIRED(mutex_);
   void SetConsumerClosedNoLock() MOJO_EXCLUSIVE_LOCKS_REQUIRED(mutex_);