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=bagder
authorBenjamin Smedberg <benjamin@smedbergs.us>
Thu, 08 Dec 2016 09:05:39 -1000
changeset 325392 28d3c72eaecdc719fa4a01d5bb2d345b216070a9
parent 325391 8f375033dba4ea7216fda052f7b921e80db2ad8d
child 325393 a0625815bdd078d2a2519ecac5a21691f3772538
push id84690
push userbsmedberg@mozilla.com
push dateThu, 08 Dec 2016 21:50:19 +0000
treeherdermozilla-inbound@28d3c72eaecd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbagder
bugs1322377
milestone53.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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=bagder 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;