Bug 1322377 - frequent intermittent timeouts in TestNamedPipeService - there was a race between notifying the monitor and waiting for it, so instead of a monitor use an event, r?badger draft
authorBenjamin Smedberg <benjamin@smedbergs.us>
Thu, 08 Dec 2016 09:05:39 -1000
changeset 447955 1a608016ee0725d2079e4d738b94fc108680f535
parent 447759 c2526f6786f074888d71c8e166a02aea3e19e75b
child 539170 8ec9d9dfb9d9e6543e3560417e9d0316e95bc8fb
push id38212
push userbsmedberg@mozilla.com
push dateThu, 08 Dec 2016 19:10:48 +0000
reviewersbadger
bugs1322377
milestone53.0a1
Bug 1322377 - frequent intermittent timeouts in TestNamedPipeService - there was a race between notifying the monitor and waiting for it, so instead of a monitor use an event, r?badger MozReview-Commit-ID: 6xgSH6shSm6
netwerk/test/TestNamedPipeService.cpp
--- a/netwerk/test/TestNamedPipeService.cpp
+++ b/netwerk/test/TestNamedPipeService.cpp
@@ -13,16 +13,51 @@
 #include "nsINamedPipeService.h"
 #include "nsNetCID.h"
 
 #define PIPE_NAME "\\\\.\\pipe\\TestNPS"
 #define TEST_STR "Hello World"
 
 using namespace mozilla;
 
+namespace {
+
+/**
+ * Unlike a monitor, an event allows a thread to wait on another thread
+ * completing an action without regard to ordering of the wait and the notify.
+ */
+class Event
+{
+public:
+  explicit Event(const char* aName)
+    : mMonitor(aName) { }
+
+  ~Event() = default;
+
+  void Set() {
+    MonitorAutoLock lock(mMonitor);
+    MOZ_ASSERT(!mSignaled);
+    mSignaled = true;
+    mMonitor.Notify();
+  }
+  void Wait() {
+    MonitorAutoLock lock(mMonitor);
+    while (!mSignaled) {
+      lock.Wait();
+    }
+    mSignaled = false;
+  }
+
+private:
+  Monitor mMonitor;
+  bool mSignaled = false;
+};
+
+} // anonymous namespace
+
 class nsNamedPipeDataObserver : public nsINamedPipeDataObserver
 {
 public:
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSINAMEDPIPEDATAOBSERVER
 
   explicit nsNamedPipeDataObserver(HANDLE aPipe);
 
@@ -32,58 +67,56 @@ public:
   uint32_t Transferred() const { return mBytesTransferred; }
 
 private:
   ~nsNamedPipeDataObserver() = default;
 
   HANDLE mPipe;
   OVERLAPPED mOverlapped;
   Atomic<uint32_t> mBytesTransferred;
-  Monitor mMonitor;
+  Event mEvent;
 };
 
 NS_IMPL_ISUPPORTS(nsNamedPipeDataObserver, nsINamedPipeDataObserver)
 
 nsNamedPipeDataObserver::nsNamedPipeDataObserver(HANDLE aPipe)
   : mPipe(aPipe)
   , mOverlapped()
   , mBytesTransferred(0)
-  , mMonitor("named-pipe")
+  , mEvent("named-pipe")
 {
   mOverlapped.hEvent = CreateEventA(nullptr, TRUE, TRUE, "named-pipe");
 }
 
 int
 nsNamedPipeDataObserver::Read(void* aBuffer, uint32_t aSize)
 {
   DWORD bytesRead = 0;
   if (!ReadFile(mPipe, aBuffer, aSize, &bytesRead, &mOverlapped)) {
     switch(GetLastError()) {
       case ERROR_IO_PENDING:
         {
-          MonitorAutoLock lock(mMonitor);
-          mMonitor.Wait();
+          mEvent.Wait();
         }
         if (!GetOverlappedResult(mPipe, &mOverlapped, &bytesRead, FALSE)) {
           ADD_FAILURE() << "GetOverlappedResult failed";
           return -1;
         }
         if (mBytesTransferred != bytesRead) {
           ADD_FAILURE() << "GetOverlappedResult mismatch";
           return -1;
         }
 
         break;
       default:
         ADD_FAILURE() << "ReadFile error " << GetLastError();
         return -1;
     }
   } else {
-    MonitorAutoLock lock(mMonitor);
-    mMonitor.Wait();
+    mEvent.Wait();
 
     if (mBytesTransferred != bytesRead) {
       ADD_FAILURE() << "GetOverlappedResult mismatch";
       return -1;
     }
   }
 
   mBytesTransferred = 0;
@@ -93,36 +126,34 @@ nsNamedPipeDataObserver::Read(void* aBuf
 int
 nsNamedPipeDataObserver::Write(const void* aBuffer, uint32_t aSize)
 {
   DWORD bytesWritten = 0;
   if (!WriteFile(mPipe, aBuffer, aSize, &bytesWritten, &mOverlapped)) {
     switch(GetLastError()) {
       case ERROR_IO_PENDING:
         {
-          MonitorAutoLock lock(mMonitor);
-          mMonitor.Wait();
+          mEvent.Wait();
         }
         if (!GetOverlappedResult(mPipe, &mOverlapped, &bytesWritten, FALSE)) {
           ADD_FAILURE() << "GetOverlappedResult failed";
           return -1;
         }
         if (mBytesTransferred != bytesWritten) {
           ADD_FAILURE() << "GetOverlappedResult mismatch";
           return -1;
         }
 
         break;
       default:
         ADD_FAILURE() << "WriteFile error " << GetLastError();
         return -1;
     }
   } else {
-    MonitorAutoLock lock(mMonitor);
-    mMonitor.Wait();
+    mEvent.Wait();
 
     if (mBytesTransferred != bytesWritten) {
       ADD_FAILURE() << "GetOverlappedResult mismatch";
       return -1;
     }
   }
 
   mBytesTransferred = 0;
@@ -150,18 +181,17 @@ nsNamedPipeDataObserver::OnDataAvailable
   }
 
   if (bytesTransferred != aBytesTransferred) {
     ADD_FAILURE() << "GetOverlappedResult mismatch";
     return NS_ERROR_FAILURE;
   }
 
   mBytesTransferred += aBytesTransferred;
-  MonitorAutoLock lock(mMonitor);
-  mMonitor.Notify();
+  mEvent.Set();
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsNamedPipeDataObserver::OnError(uint32_t aError, void *aOverlapped)
 {
   return NS_ERROR_NOT_IMPLEMENTED;