From f0493351a1432f9b491ce64c1d1c6556bbdabd2a Mon Sep 17 00:00:00 2001
From: Nick Bolton <nick@synergy-project.org>
Date: Sun, 8 Jul 2012 16:01:27 +0000
Subject: [PATCH] attempting to solve ipc recursion/deadlock problem by always
 buffering in the log outputter.

---
 src/lib/ipc/CIpcLogOutputter.cpp | 101 +++++++++++++++++++------------
 src/lib/ipc/CIpcLogOutputter.h   |  11 +++-
 src/lib/synergy/CDaemonApp.cpp   |  12 ----
 3 files changed, 69 insertions(+), 55 deletions(-)

diff --git a/src/lib/ipc/CIpcLogOutputter.cpp b/src/lib/ipc/CIpcLogOutputter.cpp
index 49703329..b0556b4f 100644
--- a/src/lib/ipc/CIpcLogOutputter.cpp
+++ b/src/lib/ipc/CIpcLogOutputter.cpp
@@ -24,26 +24,23 @@
 #include "TMethodEventJob.h"
 #include "CIpcClientProxy.h"
 #include "CArch.h"
+#include "CThread.h"
+#include "TMethodJob.h"
 
 CIpcLogOutputter::CIpcLogOutputter(CIpcServer& ipcServer) :
 m_ipcServer(ipcServer),
-m_sending(false)
+m_bufferMutex(ARCH->newMutex()),
+m_sending(false),
+m_running(true)
 {
-	m_mutex = ARCH->newMutex();
+	m_bufferThread = new CThread(new TMethodJob<CIpcLogOutputter>(
+		this, &CIpcLogOutputter::bufferThread));
 }
 
 CIpcLogOutputter::~CIpcLogOutputter()
 {
-}
-
-void
-CIpcLogOutputter::sendBuffer(CIpcClientProxy& proxy)
-{
-	CIpcMessage message;
-	message.m_type = kIpcLogLine;
-	message.m_data = new CString(m_buffer);
-	proxy.send(message);
-	m_buffer.clear();
+	ARCH->closeMutex(m_bufferMutex);
+	delete m_bufferThread;
 }
 
 void
@@ -54,6 +51,8 @@ CIpcLogOutputter::open(const char* title)
 void
 CIpcLogOutputter::close()
 {
+	m_running = false;
+	m_bufferThread->wait(5);
 }
 
 void
@@ -62,37 +61,59 @@ CIpcLogOutputter::show(bool showIfEmpty)
 }
 
 bool
-CIpcLogOutputter::write(ELevel level, const char* text)
+CIpcLogOutputter::write(ELevel, const char* text)
 {
-	// drop messages logged while sending over ipc, since ipc can cause
-	// log messages (sending these could cause recursion or deadlocks).
-	// this has the side effect of dropping messages from other threads
-	// which weren't caused by ipc, but that is just the downside of
-	// logging this way.
+	// sending the buffer generates log messages, which we must throw
+	// away (otherwise this would cause recursion). this is just a drawback
+	// of logging this way. there is also the risk that this could throw
+	// away log messages not generated by the ipc, but it seems like it
+	// would be difficult to distinguish (other than looking at the stack
+	// trace somehow). perhaps a file stream might be a better option :-/
 	if (m_sending) {
-		return false;
-	}
-
-	// protect the value of m_sending.
-	CArchMutexLock lock(m_mutex);
-	m_sending = true;
-	
-	try {
-		if (m_ipcServer.hasClients(kIpcClientGui)) {
-			CIpcMessage message;
-			message.m_type = kIpcLogLine;
-			message.m_data = new CString(text);
-			m_ipcServer.send(message, kIpcClientGui);
-		}
-		else {
-			m_buffer.append(text);
-			m_buffer.append("\n");
-		}
-		m_sending = false;
 		return true;
 	}
-	catch (...) {
-		m_sending = false;
-		throw;
+
+	CArchMutexLock lock(m_bufferMutex);
+	m_buffer.append(text);
+	m_buffer.append("\n");
+	return true;
+}
+
+void
+CIpcLogOutputter::bufferThread(void*)
+{
+	while (m_running) {
+		while (m_running && m_buffer.size() == 0) {
+			ARCH->sleep(.1);
+		}
+
+		if (!m_running) {
+			break;
+		}
+
+		if (m_ipcServer.hasClients(kIpcClientGui)) {
+			sendBuffer();
+		}
 	}
 }
+
+CString*
+CIpcLogOutputter::emptyBuffer()
+{
+	CArchMutexLock lock(m_bufferMutex);
+	CString* copy = new CString(m_buffer);
+	m_buffer.clear();
+	return copy;
+}
+
+void
+CIpcLogOutputter::sendBuffer()
+{
+	CIpcMessage message;
+	message.m_type = kIpcLogLine;
+	message.m_data = emptyBuffer();
+
+	m_sending = true;
+	m_ipcServer.send(message, kIpcClientGui);
+	m_sending = false;
+}
diff --git a/src/lib/ipc/CIpcLogOutputter.h b/src/lib/ipc/CIpcLogOutputter.h
index f2bc62e4..5e17c566 100644
--- a/src/lib/ipc/CIpcLogOutputter.h
+++ b/src/lib/ipc/CIpcLogOutputter.h
@@ -18,6 +18,7 @@
 #pragma once
 
 #include "ILogOutputter.h"
+#include "CArch.h"
 
 class CIpcServer;
 class CEvent;
@@ -38,12 +39,16 @@ public:
 	virtual void		show(bool showIfEmpty);
 	virtual bool		write(ELevel level, const char* message);
 
-	//! Sends messages queued while no clients were connected.
-	void				sendBuffer(CIpcClientProxy& proxy);
+private:
+	void				bufferThread(void*);
+	CString*			emptyBuffer();
+	void				sendBuffer();
 
 private:
 	CIpcServer&			m_ipcServer;
 	CString				m_buffer;
-	CArchMutex			m_mutex;
+	CArchMutex			m_bufferMutex;
 	bool				m_sending;
+	CThread*			m_bufferThread;
+	bool				m_running;
 };
diff --git a/src/lib/synergy/CDaemonApp.cpp b/src/lib/synergy/CDaemonApp.cpp
index 47b96c38..8a07207f 100644
--- a/src/lib/synergy/CDaemonApp.cpp
+++ b/src/lib/synergy/CDaemonApp.cpp
@@ -330,17 +330,5 @@ CDaemonApp::handleIpcMessage(const CEvent& e, void*)
 			m_relauncher->command(command);
 			break;
 		}
-
-		case kIpcHello: {
-			if (m.m_source != nullptr) {
-				CIpcClientProxy& proxy = *static_cast<CIpcClientProxy*>(m.m_source);
-				if (proxy.m_clientType == kIpcClientGui) {
-					// when a new gui client connects, send them all the
-					// log messages queued up while they were gone.
-					m_ipcLogOutputter->sendBuffer(proxy);
-				}
-			}
-			break;
-		}
 	}
 }