Oops, calling {Producer,Consumer}CloseImplNoLock() isn't the same as calling {Producer,Consumer}Clo…

mojo/edk/system/data_pipe.cc

 // Copyright 2013 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.
 
 #include "mojo/edk/system/data_pipe.h"
 
 #include <string.h>
 
 #include <algorithm>
 #include <limits>
@@ skipping 73 matching lines @@
 }
 
 void DataPipe::ProducerCancelAllAwakables() {
   base::AutoLock locker(lock_);
   DCHECK(has_local_producer_no_lock());
   producer_awakable_list_->CancelAll();
 }
 
 void DataPipe::ProducerClose() {
   base::AutoLock locker(lock_);
-  DCHECK(producer_open_);
-  producer_open_ = false;
-  DCHECK(has_local_producer_no_lock());
-  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;
-  ProducerCloseImplNoLock();
-  AwakeConsumerAwakablesForStateChangeNoLock(
-      ConsumerGetHandleSignalsStateImplNoLock());
+  ProducerCloseNoLock();
 }
 
 MojoResult DataPipe::ProducerWriteData(UserPointer<const void> elements,
                                        UserPointer<uint32_t> num_bytes,
                                        bool all_or_none) {
   base::AutoLock locker(lock_);
   DCHECK(has_local_producer_no_lock());
 
   if (producer_in_two_phase_write_no_lock())
     return MOJO_RESULT_BUSY;
@@ skipping 149 matching lines @@
 }
 
 void DataPipe::ConsumerCancelAllAwakables() {
   base::AutoLock locker(lock_);
   DCHECK(has_local_consumer_no_lock());
   consumer_awakable_list_->CancelAll();
 }
 
 void DataPipe::ConsumerClose() {
   base::AutoLock locker(lock_);
-  DCHECK(consumer_open_);
-  consumer_open_ = false;
-  DCHECK(has_local_consumer_no_lock());
-  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;
-  ConsumerCloseImplNoLock();
-  AwakeProducerAwakablesForStateChangeNoLock(
-      ProducerGetHandleSignalsStateImplNoLock());
+  ConsumerCloseNoLock();
 }
 
 MojoResult DataPipe::ConsumerReadData(UserPointer<void> elements,
                                       UserPointer<uint32_t> num_bytes,
                                       bool all_or_none,
                                       bool peek) {
   base::AutoLock locker(lock_);
   DCHECK(has_local_consumer_no_lock());
 
   if (consumer_in_two_phase_read_no_lock())
@@ skipping 199 matching lines @@
             MOJO_RESULT_OK);
 }
 
 DataPipe::~DataPipe() {
   DCHECK(!producer_open_);
   DCHECK(!consumer_open_);
   DCHECK(!producer_awakable_list_);
   DCHECK(!consumer_awakable_list_);
 }
 
+void DataPipe::ProducerCloseNoLock() {
+  lock_.AssertAcquired();
+  DCHECK(producer_open_);
+  producer_open_ = false;
+  DCHECK(has_local_producer_no_lock());
+  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;
+  ProducerCloseImplNoLock();

[yzshen1, 2015/02/19 18:07:50]

I feel that usually XXXImpl should do all (or most) of XXX's work. This doesn't
seem the case. Maybe we should change it to use a more specific name? It helps
to avoid such confusion in the future.

+  AwakeConsumerAwakablesForStateChangeNoLock(
+      ConsumerGetHandleSignalsStateImplNoLock());
+}
+
+void DataPipe::ConsumerCloseNoLock() {
+  lock_.AssertAcquired();
+  DCHECK(consumer_open_);
+  consumer_open_ = false;
+  DCHECK(has_local_consumer_no_lock());
+  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;
+  ConsumerCloseImplNoLock();

[yzshen1, 2015/02/19 18:07:50]

ditto

+  AwakeProducerAwakablesForStateChangeNoLock(
+      ProducerGetHandleSignalsStateImplNoLock());
+}
+
 void DataPipe::AwakeProducerAwakablesForStateChangeNoLock(
     const HandleSignalsState& new_producer_state) {
   lock_.AssertAcquired();
   if (!has_local_producer_no_lock())
     return;
   producer_awakable_list_->AwakeForStateChange(new_producer_state);
 }
 
 void DataPipe::AwakeConsumerAwakablesForStateChangeNoLock(
     const HandleSignalsState& new_consumer_state) {
   lock_.AssertAcquired();
   if (!has_local_consumer_no_lock())
     return;
   consumer_awakable_list_->AwakeForStateChange(new_consumer_state);
 }
 
 }  // namespace system
 }  // namespace mojo