From 9636af61d621cc7a1efe7dc6b1669875db78f01c Mon Sep 17 00:00:00 2001
From: Nick Bolton <nick@synergy-project.org>
Date: Wed, 20 May 2015 15:51:07 +0100
Subject: [PATCH] Solution attempt for timing bugs in write_bufferRateLimit

It's probably better to wait until the buffer is sent, rather than
waiting until its empty. To test the output it has to be sent, but
because of timing, it may be emptied at any point.
---
 src/lib/ipc/IpcLogOutputter.cpp               | 20 ---------------
 src/lib/ipc/IpcLogOutputter.h                 |  2 --
 src/test/mock/ipc/MockIpcServer.h             | 25 ++++++++++++++++++-
 .../unittests/ipc/IpcLogOutputterTests.cpp    | 11 ++++----
 4 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/src/lib/ipc/IpcLogOutputter.cpp b/src/lib/ipc/IpcLogOutputter.cpp
index 7474f655..42749d68 100644
--- a/src/lib/ipc/IpcLogOutputter.cpp
+++ b/src/lib/ipc/IpcLogOutputter.cpp
@@ -46,8 +46,6 @@ IpcLogOutputter::IpcLogOutputter(IpcServer& ipcServer) :
 	m_notifyMutex(ARCH->newMutex()),
 	m_bufferWaiting(false),
 	m_bufferMaxSize(kBufferMaxSize),
-	m_bufferEmptyCond(ARCH->newCondVar()),
-	m_bufferEmptyMutex(ARCH->newMutex()),
 	m_bufferRateWriteLimit(kBufferRateWriteLimit),
 	m_bufferRateTimeLimit(kBufferRateTimeLimit),
 	m_bufferWriteCount(0),
@@ -66,13 +64,6 @@ IpcLogOutputter::~IpcLogOutputter()
 
 	ARCH->closeCondVar(m_notifyCond);
 	ARCH->closeMutex(m_notifyMutex);
-
-	ARCH->closeCondVar(m_bufferEmptyCond);
-	
-#ifndef WINAPI_CARBON
-	// HACK: assert fails on mac debug, can't see why.
-	ARCH->closeMutex(m_bufferEmptyMutex);
-#endif // WINAPI_CARBON
 }
 
 void
@@ -172,11 +163,6 @@ IpcLogOutputter::bufferThread(void*)
 				break;
 			}
 
-			if (m_buffer.empty()) {
-				ArchMutexLock lock(m_bufferEmptyMutex);
-				ARCH->broadcastCondVar(m_bufferEmptyCond);
-			}
-
 			m_bufferWaiting = true;
 			ARCH->waitCondVar(m_notifyCond, m_notifyMutex, -1);
 			m_bufferWaiting = false;
@@ -239,12 +225,6 @@ IpcLogOutputter::bufferMaxSize() const
 	return m_bufferMaxSize;
 }
 
-void
-IpcLogOutputter::waitForEmpty()
-{
-	ARCH->waitCondVar(m_bufferEmptyCond, m_bufferEmptyMutex, -1);
-}
-
 void
 IpcLogOutputter::bufferRateLimit(UInt16 writeLimit, double timeLimit)
 {
diff --git a/src/lib/ipc/IpcLogOutputter.h b/src/lib/ipc/IpcLogOutputter.h
index 9b277a03..fb55cfbf 100644
--- a/src/lib/ipc/IpcLogOutputter.h
+++ b/src/lib/ipc/IpcLogOutputter.h
@@ -106,8 +106,6 @@ private:
 	IArchMultithread::ThreadID
 						m_bufferThreadId;
 	UInt16				m_bufferMaxSize;
-	ArchCond			m_bufferEmptyCond;
-	ArchMutex			m_bufferEmptyMutex;
 	UInt16				m_bufferRateWriteLimit;
 	double				m_bufferRateTimeLimit;
 	UInt16				m_bufferWriteCount;
diff --git a/src/test/mock/ipc/MockIpcServer.h b/src/test/mock/ipc/MockIpcServer.h
index ae63cd34..ccbc99a1 100644
--- a/src/test/mock/ipc/MockIpcServer.h
+++ b/src/test/mock/ipc/MockIpcServer.h
@@ -19,17 +19,40 @@
 
 #include "ipc/IpcServer.h"
 #include "ipc/IpcMessage.h"
+#include "arch/Arch.h"
 
 #include "test/global/gmock.h"
 
+using ::testing::_;
+using ::testing::Invoke;
+
 class IEventQueue;
 
 class MockIpcServer : public IpcServer
 {
 public:
-	MockIpcServer() { }
+	MockIpcServer() :
+		m_sendCond(ARCH->newCondVar()),
+		m_sendMutex(ARCH->newMutex()) { }
 
 	MOCK_METHOD0(listen, void());
 	MOCK_METHOD2(send, void(const IpcMessage&, EIpcClientType));
 	MOCK_CONST_METHOD1(hasClients, bool(EIpcClientType));
+
+	void delegateToFake() {
+		ON_CALL(*this, send(_, _)).WillByDefault(Invoke(this, &MockIpcServer::mockSend));
+	}
+
+	void waitForSend() {
+		ARCH->waitCondVar(m_sendCond, m_sendMutex, -1);
+	}
+
+private:
+	void mockSend(const IpcMessage&, EIpcClientType) {
+		ArchMutexLock lock(m_sendMutex);
+		ARCH->broadcastCondVar(m_sendCond);
+	}
+
+	ArchCond			m_sendCond;
+	ArchMutex			m_sendMutex;
 };
diff --git a/src/test/unittests/ipc/IpcLogOutputterTests.cpp b/src/test/unittests/ipc/IpcLogOutputterTests.cpp
index 18f1eb21..449ebf6e 100644
--- a/src/test/unittests/ipc/IpcLogOutputterTests.cpp
+++ b/src/test/unittests/ipc/IpcLogOutputterTests.cpp
@@ -48,6 +48,7 @@ inline const Matcher<const IpcMessage&> IpcLogLineMessageEq(const String& s) {
 TEST(IpcLogOutputterTests, write_bufferSizeWrapping)
 {
 	MockIpcServer mockServer;
+	mockServer.delegateToFake();
 	
 	ON_CALL(mockServer, hasClients(_)).WillByDefault(Return(true));
 
@@ -61,14 +62,13 @@ TEST(IpcLogOutputterTests, write_bufferSizeWrapping)
 	outputter.write(kNOTE, "mock 1");
 	outputter.write(kNOTE, "mock 2");
 	outputter.write(kNOTE, "mock 3");
-
-	// wait for the buffer to be empty (all lines sent to IPC)
-	outputter.waitForEmpty();
+	mockServer.waitForSend();
 }
 
 TEST(IpcLogOutputterTests, write_bufferRateLimit)
 {
 	MockIpcServer mockServer;
+	mockServer.delegateToFake();
 	
 	ON_CALL(mockServer, hasClients(_)).WillByDefault(Return(true));
 
@@ -82,14 +82,13 @@ TEST(IpcLogOutputterTests, write_bufferRateLimit)
 	// log 1 more line than the buffer can accept in time limit.
 	outputter.write(kNOTE, "mock 1");
 	outputter.write(kNOTE, "mock 2");
-	outputter.waitForEmpty();
+	mockServer.waitForSend();
 	
 	// after waiting the time limit send another to make sure
 	// we can log after the time limit passes.
-	ARCH->sleep(0.01); // 10ms
 	outputter.write(kNOTE, "mock 3");
 	outputter.write(kNOTE, "mock 4");
-	outputter.waitForEmpty();
+	mockServer.waitForSend();
 }
 
 #endif // WINAPI_MSWINDOWS