Added failing test for IpcLogOutputter::write(...) #4651

- Changed behavior of close() to stop the buffer thread
- Fixed code style in IpcLogOutputter.cpp
- Changed MAX_SEND macro to enum
- Added Doxygen @name sections

src/lib/ipc/IpcLogOutputter.cpp

@@ -30,17 +30,22 @@
 #include "base/TMethodEventJob.h"
 #include "base/TMethodJob.h"
 
-// limit number of log lines sent in one message.
+enum EIpcLogOutputter {
-#define MAX_SEND 100
+	kBufferMaxSize = 1000,
+	kMaxSendLines = 100
+};
 
 IpcLogOutputter::IpcLogOutputter(IpcServer& ipcServer) :
-m_ipcServer(ipcServer),
+	m_ipcServer(ipcServer),
-m_bufferMutex(ARCH->newMutex()),
+	m_bufferMutex(ARCH->newMutex()),
-m_sending(false),
+	m_sending(false),
-m_running(true),
+	m_running(true),
-m_notifyCond(ARCH->newCondVar()),
+	m_notifyCond(ARCH->newCondVar()),
-m_notifyMutex(ARCH->newMutex()),
+	m_notifyMutex(ARCH->newMutex()),
-m_bufferWaiting(false)
+	m_bufferWaiting(false),
+	m_bufferMaxSize(kBufferMaxSize),
+	m_bufferEmptyCond(ARCH->newCondVar()),
+	m_bufferEmptyMutex(ARCH->newMutex())
 {
 	m_bufferThread = new Thread(new TMethodJob<IpcLogOutputter>(
 		this, &IpcLogOutputter::bufferThread));
@@ -48,15 +53,16 @@ m_bufferWaiting(false)
 
 IpcLogOutputter::~IpcLogOutputter()
 {
-	m_running = false;
+	close();
-	notifyBuffer();
-	m_bufferThread->wait(5);
 
 	ARCH->closeMutex(m_bufferMutex);
 	delete m_bufferThread;
 
 	ARCH->closeCondVar(m_notifyCond);
 	ARCH->closeMutex(m_notifyMutex);
+
+	ARCH->closeCondVar(m_bufferEmptyCond);
+	ARCH->closeMutex(m_bufferEmptyMutex);
 }
 
 void
@@ -67,6 +73,9 @@ IpcLogOutputter::open(const char* title)
 void
 IpcLogOutputter::close()
 {
+	m_running = false;
+	notifyBuffer();
+	m_bufferThread->wait(5);
 }
 
 void
@@ -133,6 +142,11 @@ 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;
@@ -176,9 +190,31 @@ IpcLogOutputter::getChunk(size_t count)
 void
 IpcLogOutputter::sendBuffer()
 {
-	IpcLogLineMessage message(getChunk(MAX_SEND));
+	IpcLogLineMessage message(getChunk(kMaxSendLines));
 
 	m_sending = true;
 	m_ipcServer.send(message, kIpcClientGui);
 	m_sending = false;
 }
+
+void
+IpcLogOutputter::bufferMaxSize(UInt16 bufferMaxSize)
+{
+	m_bufferMaxSize = bufferMaxSize;
+}
+
+UInt16
+IpcLogOutputter::bufferMaxSize() const
+{
+	return m_bufferMaxSize;
+}
+
+void
+IpcLogOutputter::close(bool waitForEmpty)
+{
+	if (waitForEmpty) {
+		ARCH->waitCondVar(m_bufferEmptyCond, m_bufferEmptyMutex, -1);
+	}
+
+	close();
+}

src/test/unittests/ipc/IpcLogOutputterTests.cpp

@@ -17,21 +17,50 @@
 
 #define TEST_ENV
 
+#include "test/mock/ipc/MockIpcServer.h"
+
+#include "mt/Thread.h"
 #include "ipc/IpcLogOutputter.h"
 #include "base/String.h"
 
-#include "test/mock/ipc/MockIpcServer.h"
+#include "test/global/gmock.h"
-
 #include "test/global/gtest.h"
 
+using ::testing::_;
+using ::testing::Return;
+using ::testing::Matcher;
+using ::testing::MatcherCast;
+using ::testing::Property;
+using ::testing::StrEq;
+
 using namespace synergy;
 
+inline const Matcher<const IpcMessage&> IpcLogLineMessageEq(const String& s) {
+	const Matcher<const IpcLogLineMessage&> m(
+		Property(&IpcLogLineMessage::logLine, StrEq(s)));
+	return MatcherCast<const IpcMessage&>(m);
+}
+
 TEST(IpcLogOutputterTests, write_bufferSizeWrapping)
 {
 	MockIpcServer mockServer;
-	IpcLogOutputter outputter(mockServer);
+	
+	ON_CALL(mockServer, hasClients(_)).WillByDefault(Return(true));
 
-	outputter.write(kNOTE, "hello world", false);
+	EXPECT_CALL(mockServer, hasClients(_)).Times(1);
+	EXPECT_CALL(mockServer, send(IpcLogLineMessageEq("mock 2\nmock 3\n"), _)).Times(1);
+
+	IpcLogOutputter outputter(mockServer);
+	outputter.bufferMaxSize(2);
+
+	// log more lines than the buffer can contain
+	for (UInt8 i = 1; i <= 3; i++) {
+		String s = string::sprintf("mock %d", i);
+		outputter.write(kNOTE, s.c_str());
+	}
+
+	// close, but wait until the buffer is empty.
+	outputter.close(true);
 
 	EXPECT_EQ(true, true);
 }