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
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
--- 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;