From 17a92f4f4ce7055a6f704dcd1a8187dd7344c809 Mon Sep 17 00:00:00 2001 From: Nick Bolton Date: Fri, 6 Jul 2012 14:46:46 +0000 Subject: [PATCH] attempted to fix deadlock caused by ipc logger causing recursion. --- src/gui/src/IpcClient.cpp | 6 ++- src/lib/ipc/CIpcClientProxy.cpp | 86 +++++++++++++++++++++++---------- src/lib/ipc/CIpcClientProxy.h | 4 ++ src/lib/synergy/CDaemonApp.cpp | 20 +++++++- 4 files changed, 87 insertions(+), 29 deletions(-) diff --git a/src/gui/src/IpcClient.cpp b/src/gui/src/IpcClient.cpp index f868a469..dc1e947b 100644 --- a/src/gui/src/IpcClient.cpp +++ b/src/gui/src/IpcClient.cpp @@ -62,12 +62,14 @@ void IpcClient::read() char lenBuf[2]; stream.readRawData(lenBuf, 2); int len = bytesToInt(lenBuf, 2); - std::cout << "len: " << len << std::endl; + std::cout << "told len: " << len << std::endl; char* data = new char[len]; stream.readRawData(data, len); - readLogLine(QString::fromUtf8(data, len)); + QString line = QString::fromUtf8(data, len); + std::cout << "actual len: " << line.size() << std::endl; + readLogLine(line); break; } diff --git a/src/lib/ipc/CIpcClientProxy.cpp b/src/lib/ipc/CIpcClientProxy.cpp index 73915035..1656d0de 100644 --- a/src/lib/ipc/CIpcClientProxy.cpp +++ b/src/lib/ipc/CIpcClientProxy.cpp @@ -22,6 +22,7 @@ #include "CLog.h" #include "CIpcMessage.h" #include "CProtocolUtil.h" +#include "CArch.h" CEvent::Type CIpcClientProxy::s_messageReceivedEvent = CEvent::kUnknown; CEvent::Type CIpcClientProxy::s_disconnectedEvent = CEvent::kUnknown; @@ -30,13 +31,21 @@ CIpcClientProxy::CIpcClientProxy(synergy::IStream& stream) : m_stream(stream), m_enableLog(false), m_clientType(kIpcClientUnknown), -m_disconnecting(false) +m_disconnecting(false), +m_socketBusy(false) { + m_mutex = ARCH->newMutex(); + EVENTQUEUE->adoptHandler( m_stream.getInputReadyEvent(), stream.getEventTarget(), new TMethodEventJob( this, &CIpcClientProxy::handleData)); + EVENTQUEUE->adoptHandler( + m_stream.getOutputErrorEvent(), stream.getEventTarget(), + new TMethodEventJob( + this, &CIpcClientProxy::handleWriteError)); + EVENTQUEUE->adoptHandler( m_stream.getInputShutdownEvent(), stream.getEventTarget(), new TMethodEventJob( @@ -52,6 +61,8 @@ CIpcClientProxy::~CIpcClientProxy() { EVENTQUEUE->removeHandler( m_stream.getInputReadyEvent(), m_stream.getEventTarget()); + EVENTQUEUE->removeHandler( + m_stream.getOutputErrorEvent(), m_stream.getEventTarget()); EVENTQUEUE->removeHandler( m_stream.getInputShutdownEvent(), m_stream.getEventTarget()); EVENTQUEUE->removeHandler( @@ -77,6 +88,12 @@ CIpcClientProxy::handleWriteError(const CEvent&, void*) void CIpcClientProxy::handleData(const CEvent&, void*) { + // ugh, not sure if this is needed here. the write function has it to protect + // m_socketBusy (for dropping log messages primarily). but i think i saw a + // deadlock even after adding this mechanism. my rationale here is that i don't + // want the read to deadlock the stream (if that's even possible). + CArchMutexLock lock(m_mutex); + UInt8 code[1]; UInt32 n = m_stream.read(code, 1); while (n != 0) { @@ -115,35 +132,52 @@ CIpcClientProxy::handleData(const CEvent&, void*) void CIpcClientProxy::send(const CIpcMessage& message) { - if (m_enableLog) { - LOG((CLOG_DEBUG "ipc client proxy write: %d", message.m_type)); + // discard message if we're busy writing already. this is to prevent + // deadlock, since this function can sometimes generate log messages + // through stream usage. + if (m_socketBusy) { + return; } - UInt8 code[1]; - code[0] = message.m_type; - m_stream.write(code, 1); - - switch (message.m_type) { - case kIpcLogLine: { - CString* s = (CString*)message.m_data; - const char* data = s->c_str(); - - int len = strlen(data); - CProtocolUtil::writef(&m_stream, "%2i", len); - - m_stream.write(data, len); - break; - } - - case kIpcShutdown: - // no data. - break; - - default: + CArchMutexLock lock(m_mutex); + m_socketBusy = true; + try { if (m_enableLog) { - LOG((CLOG_ERR "message not supported: %d", message.m_type)); + LOG((CLOG_DEBUG "ipc client proxy write: %d", message.m_type)); } - break; + + UInt8 code[1]; + code[0] = message.m_type; + m_stream.write(code, 1); + + switch (message.m_type) { + case kIpcLogLine: { + CString* s = (CString*)message.m_data; + const char* data = s->c_str(); + + int len = strlen(data); + CProtocolUtil::writef(&m_stream, "%2i", len); + + m_stream.write(data, len); + break; + } + + case kIpcShutdown: + // no data. + break; + + default: + if (m_enableLog) { + LOG((CLOG_ERR "message not supported: %d", message.m_type)); + } + break; + } + + m_socketBusy = false; + } + catch (...) { + m_socketBusy = false; + throw; } } diff --git a/src/lib/ipc/CIpcClientProxy.h b/src/lib/ipc/CIpcClientProxy.h index d1866b9a..df2c0d60 100644 --- a/src/lib/ipc/CIpcClientProxy.h +++ b/src/lib/ipc/CIpcClientProxy.h @@ -19,6 +19,7 @@ #include "CEvent.h" #include "Ipc.h" +#include "CMutex.h" namespace synergy { class IStream; } class CIpcMessage; @@ -50,8 +51,11 @@ public: bool m_enableLog; EIpcClientType m_clientType; bool m_disconnecting; + bool m_socketBusy; private: + CArchMutex m_mutex; + static CEvent::Type s_messageReceivedEvent; static CEvent::Type s_disconnectedEvent; }; diff --git a/src/lib/synergy/CDaemonApp.cpp b/src/lib/synergy/CDaemonApp.cpp index 05ed72b3..47b96c38 100644 --- a/src/lib/synergy/CDaemonApp.cpp +++ b/src/lib/synergy/CDaemonApp.cpp @@ -297,13 +297,31 @@ CDaemonApp::handleIpcMessage(const CEvent& e, void*) CString& command = *static_cast(m.m_data); LOG((CLOG_DEBUG "got new command: %s", command.c_str())); + CString debugArg("--debug"); + int debugArgPos = command.find(debugArg); + if (debugArgPos != CString::npos) { + int from = debugArgPos + debugArg.size() + 1; + int nextSpace = command.find(" ", from); + CString logLevel(command.substr(from, nextSpace - from)); + + try { + // change log level based on that in the command string + // and change to that log level now. + ARCH->setting("LogLevel", logLevel); + CLOG->setFilter(logLevel.c_str()); + } + catch (XArch& e) { + LOG((CLOG_ERR "failed to save LogLevel setting, %s", e.what().c_str())); + } + } + try { // store command in system settings. this is used when the daemon // next starts. ARCH->setting("Command", command); } catch (XArch& e) { - LOG((CLOG_ERR "failed to save setting, %s", e.what().c_str())); + LOG((CLOG_ERR "failed to save Command setting, %s", e.what().c_str())); } // tell the relauncher about the new command. this causes the