added better handling of X server disconnecting unexpectedly.

the apps still exit but they do it in a mostly controlled
manner.  in particular, the server threads except the one
processing primary screen events will terminate gracefully.
this will be important should the server ever allow HTTP
clients to rewrite the configuration file.

note that X makes it effectively impossible to continue once
the X server disconnects.  even if it didn't it would be
difficult for synergy to recover.  users will have to add
synergy to the X display manager's startup script if they
expect the server to be restarted.  alternatively, we could
add code to fork synergy at startup;  the child would do
the normal work while the parent would simply wait for the
child to exit and restart it.

client/CClient.cpp

@@ -10,6 +10,7 @@
 #include "CThread.h"
 #include "CTimerThread.h"
 #include "TMethodJob.h"
+#include "XScreen.h"
 #include "XSynergy.h"
 #include "XThread.h"
 #include <assert.h>
@@ -51,7 +52,16 @@ void					CClient::run(const CNetworkAddress& serverAddress)
 		log((CLOG_NOTE "starting client"));
 
 		// connect to secondary screen
+		while (m_screen == NULL) {
+			try {
 				openSecondaryScreen();
+			}
+			catch (XScreenOpenFailure&) {
+				// can't open screen yet.  wait a few seconds to retry.
+				log((CLOG_INFO "failed to open screen.  waiting to retry."));
+				CThread::sleep(3.0);
+			}
+		}
 
 		// start server interactions
 		m_serverAddress = &serverAddress;
@@ -88,7 +98,9 @@ void					CClient::run(const CNetworkAddress& serverAddress)
 			thread->wait();
 			delete thread;
 		}
+		if (m_screen != NULL) {
 			closeSecondaryScreen();
+		}
 		throw;
 	}
 	catch (...) {
@@ -101,7 +113,9 @@ void					CClient::run(const CNetworkAddress& serverAddress)
 			thread->wait();
 			delete thread;
 		}
+		if (m_screen != NULL) {
 			closeSecondaryScreen();
+		}
 		throw;
 	}
 }

client/CXWindowsSecondaryScreen.cpp

@@ -290,12 +290,11 @@ void					CXWindowsSecondaryScreen::getClipboard(
 	getDisplayClipboard(id, clipboard);
 }
 
-void					CXWindowsSecondaryScreen::onOpenDisplay()
+void					CXWindowsSecondaryScreen::onOpenDisplay(
+								Display* display)
 {
 	assert(m_window == None);
 
-	CDisplayLock display(this);
-
 	// create the cursor hiding window.  this window is used to hide the
 	// cursor when it's not on the screen.  the window is hidden as soon
 	// as the cursor enters the screen or the display's real cursor is
@@ -326,16 +325,18 @@ CXWindowsClipboard*		CXWindowsSecondaryScreen::createClipboard(
 	return new CXWindowsClipboard(display, m_window, id);
 }
 
-void					CXWindowsSecondaryScreen::onCloseDisplay()
+void					CXWindowsSecondaryScreen::onCloseDisplay(
+								Display* display)
 {
 	assert(m_window != None);
 
+	if (display != NULL) {
 		// no longer impervious to server grabs
-	CDisplayLock display(this);
 		XTestGrabControl(display, False);
 
 		// destroy window
 		XDestroyWindow(display, m_window);
+	}
 	m_window = None;
 }
 

client/CXWindowsSecondaryScreen.h

@@ -35,10 +35,10 @@ public:
 
 protected:
 	// CXWindowsScreen overrides
-	virtual void		onOpenDisplay();
+	virtual void		onOpenDisplay(Display*);
 	virtual CXWindowsClipboard*
 						createClipboard(ClipboardID);
-	virtual void		onCloseDisplay();
+	virtual void		onCloseDisplay(Display*);
 	virtual void		onLostClipboard(ClipboardID);
 
 private:

server/CServer.cpp

@@ -17,6 +17,7 @@
 #include "CStopwatch.h"
 #include "CFunctionJob.h"
 #include "TMethodJob.h"
+#include "XScreen.h"
 #include "XSocket.h"
 #include "XSynergy.h"
 #include "XThread.h"
@@ -46,7 +47,8 @@ else { wait(0); exit(1); }
 
 const SInt32			CServer::s_httpMaxSimultaneousRequests = 3;
 
-CServer::CServer() : m_primary(NULL),
+CServer::CServer() : m_cleanupSize(&m_mutex, 0),
+								m_primary(NULL),
 								m_active(NULL),
 								m_primaryInfo(NULL),
 								m_seqNum(0),
@@ -69,7 +71,16 @@ void					CServer::run()
 		log((CLOG_NOTE "starting server"));
 
 		// connect to primary screen
+		while (m_primary == NULL) {
+			try {
 				openPrimaryScreen();
+			}
+			catch (XScreenOpenFailure&) {
+				// can't open screen yet.  wait a few seconds to retry.
+				log((CLOG_INFO "failed to open screen.  waiting to retry."));
+				CThread::sleep(3.0);
+			}
+		}
 
 		// start listening for HTTP requests
 		m_httpServer = new CHTTPServer(this);
@@ -84,9 +95,9 @@ void					CServer::run()
 
 		// clean up
 		log((CLOG_NOTE "stopping server"));
+		cleanupThreads();
 		delete m_httpServer;
 		m_httpServer = NULL;
-		cleanupThreads();
 		closePrimaryScreen();
 	}
 	catch (XBase& e) {
@@ -94,18 +105,20 @@ void					CServer::run()
 
 		// clean up
 		log((CLOG_NOTE "stopping server"));
+		cleanupThreads();
 		delete m_httpServer;
 		m_httpServer = NULL;
-		cleanupThreads();
 		closePrimaryScreen();
 	}
 	catch (XThread&) {
 		// clean up
 		log((CLOG_NOTE "stopping server"));
+		cleanupThreads();
 		delete m_httpServer;
 		m_httpServer = NULL;
-		cleanupThreads();
+		if (m_primary != NULL) {
 			closePrimaryScreen();
+		}
 		throw;
 	}
 	catch (...) {
@@ -113,10 +126,12 @@ void					CServer::run()
 
 		// clean up
 		log((CLOG_NOTE "stopping server"));
+		cleanupThreads();
 		delete m_httpServer;
 		m_httpServer = NULL;
-		cleanupThreads();
+		if (m_primary != NULL) {
 			closePrimaryScreen();
+		}
 		throw;
 	}
 }
@@ -126,6 +141,19 @@ void					CServer::quit()
 	m_primary->stop();
 }
 
+void					CServer::shutdown()
+{
+	// stop all running threads but don't wait too long since some
+	// threads may be unable to proceed until this thread returns.
+	cleanupThreads(3.0);
+
+	// done with the HTTP server
+	delete m_httpServer;
+	m_httpServer = NULL;
+
+	// note -- we do not attempt to close down the primary screen
+}
+
 bool					CServer::setConfig(const CConfig& config)
 {
 	typedef std::vector<CThread> CThreads;
@@ -1289,6 +1317,7 @@ void					CServer::openPrimaryScreen()
 	// reset sequence number
 	m_seqNum = 0;
 
+	try {
 		// add connection
 		m_active      = addConnection(CString("primary"/* FIXME */), NULL);
 		m_primaryInfo = m_active;
@@ -1302,6 +1331,15 @@ void					CServer::openPrimaryScreen()
 #endif
 		log((CLOG_DEBUG1 "opening primary screen"));
 		m_primary->open(this);
+	}
+	catch (...) {
+		delete m_primary;
+		removeConnection(CString("primary"/* FIXME */));
+		m_primary     = NULL;
+		m_primaryInfo = NULL;
+		m_active      = NULL;
+		throw;
+	}
 
 	// set the clipboard owner to the primary screen and then get the
 	// current clipboard data.
@@ -1340,6 +1378,7 @@ void					CServer::addCleanupThread(const CThread& thread)
 {
 	CLock lock(&m_mutex);
 	m_cleanupList.insert(m_cleanupList.begin(), new CThread(thread));
+	m_cleanupSize = m_cleanupSize + 1;
 }
 
 void					CServer::removeCleanupThread(const CThread& thread)
@@ -1350,30 +1389,52 @@ void					CServer::removeCleanupThread(const CThread& thread)
 		if (**index == thread) {
 			CThread* thread = *index;
 			m_cleanupList.erase(index);
+			m_cleanupSize = m_cleanupSize - 1;
+			if (m_cleanupSize == 0) {
+				m_cleanupSize.broadcast();
+			}
 			delete thread;
 			return;
 		}
 	}
 }
 
-void					CServer::cleanupThreads()
+void					CServer::cleanupThreads(double timeout)
 {
 	log((CLOG_DEBUG1 "cleaning up threads"));
-	m_mutex.lock();
-	while (m_cleanupList.begin() != m_cleanupList.end()) {
-		// get the next thread and cancel it
-		CThread* thread = m_cleanupList.front();
-		thread->cancel();
 
-		// wait for thread to finish with cleanup list unlocked.  the
+	// first cancel every thread except the current one (with mutex
-		// thread will remove itself from the cleanup list.
+	// locked so the cleanup list won't change).
-		m_mutex.unlock();
+	CLock lock(&m_mutex);
-		thread->wait();
+	CThread current(CThread::getCurrentThread());
-		m_mutex.lock();
+	SInt32 minCount = 0;
+	for (CThreadList::iterator index = m_cleanupList.begin();
+								index != m_cleanupList.end(); ++index) {
+		CThread* thread = *index;
+		if (thread != &current) {
+			thread->cancel();
+		}
+		else {
+			minCount = 1;
+		}
 	}
 
-	// FIXME -- delete remaining threads from list
+	// now wait for the threads (with mutex unlocked as each thread
-	m_mutex.unlock();
+	// will remove itself from the list)
+	CStopwatch timer(true);
+	while (m_cleanupSize > minCount) {
+		m_cleanupSize.wait(timer, timeout);
+	}
+
+	// delete remaining threads
+	for (CThreadList::iterator index = m_cleanupList.begin();
+								index != m_cleanupList.end(); ++index) {
+		CThread* thread = *index;
+		delete thread;
+	}
+	m_cleanupList.clear();
+	m_cleanupSize = 0;
+
 	log((CLOG_DEBUG1 "cleaned up threads"));
 }
 

server/CServer.h

@@ -34,6 +34,11 @@ public:
 	// tell server to exit gracefully
 	void				quit();
 
+	// tell the server to shutdown.  this is called in an emergency
+	// when we need to tell the server that we cannot continue.  the
+	// server will attempt to clean up.
+	void				shutdown();
+
 	// update screen map.  returns true iff the new configuration was
 	// accepted.
 	bool				setConfig(const CConfig&);
@@ -173,7 +178,7 @@ private:
 	void				updatePrimaryClipboard(ClipboardID);
 
 	// cancel running threads
-	void				cleanupThreads();
+	void				cleanupThreads(double timeout = -1.0);
 
 	// thread method to accept incoming client connections
 	void				acceptClients(void*);
@@ -220,6 +225,7 @@ private:
 	ISecurityFactory*	m_securityFactory;
 
 	CThreadList			m_cleanupList;
+	CCondVar<SInt32>	m_cleanupSize;
 
 	IPrimaryScreen*		m_primary;
 	CScreenList			m_screens;

server/CXWindowsPrimaryScreen.cpp

@@ -179,6 +179,7 @@ void					CXWindowsPrimaryScreen::run()
 
 void					CXWindowsPrimaryScreen::stop()
 {
+	CDisplayLock display(this);
 	doStop();
 }
 
@@ -187,12 +188,12 @@ void					CXWindowsPrimaryScreen::open(CServer* server)
 	assert(m_server == NULL);
 	assert(server   != NULL);
 
-	// set the server
-	m_server = server;
-
 	// open the display
 	openDisplay();
 
+	// set the server
+	m_server = server;
+
 	// check for peculiarities
 	// FIXME -- may have to get these from some database
 	m_numLockHalfDuplex  = false;
@@ -419,12 +420,10 @@ bool					CXWindowsPrimaryScreen::isLockedToScreen() const
 	return false;
 }
 
-void					CXWindowsPrimaryScreen::onOpenDisplay()
+void					CXWindowsPrimaryScreen::onOpenDisplay(Display* display)
 {
 	assert(m_window == None);
 
-	CDisplayLock display(this);
-
 	// get size of screen
 	SInt32 w, h;
 	getScreenSize(&w, &h);
@@ -458,16 +457,25 @@ CXWindowsClipboard*		CXWindowsPrimaryScreen::createClipboard(
 	return new CXWindowsClipboard(display, m_window, id);
 }
 
-void					CXWindowsPrimaryScreen::onCloseDisplay()
+void					CXWindowsPrimaryScreen::onCloseDisplay(Display* display)
 {
 	assert(m_window != None);
 
 	// destroy window
-	CDisplayLock display(this);
+	if (display != NULL) {
 		XDestroyWindow(display, m_window);
+	}
 	m_window = None;
 }
 
+void					CXWindowsPrimaryScreen::onUnexpectedClose()
+{
+	// tell server to shutdown
+	if (m_server != NULL) {
+		m_server->shutdown();
+	}
+}
+
 void					CXWindowsPrimaryScreen::onLostClipboard(
 								ClipboardID id)
 {

server/CXWindowsPrimaryScreen.h

@@ -30,10 +30,11 @@ public:
 
 protected:
 	// CXWindowsScreen overrides
-	virtual void		onOpenDisplay();
+	virtual void		onOpenDisplay(Display*);
 	virtual CXWindowsClipboard*
 						createClipboard(ClipboardID);
-	virtual void		onCloseDisplay();
+	virtual void		onCloseDisplay(Display*);
+	virtual void		onUnexpectedClose();
 	virtual void		onLostClipboard(ClipboardID);
 
 private:

synergy/CXWindowsScreen.cpp

@@ -6,6 +6,8 @@
 #include "CLog.h"
 #include "CString.h"
 #include "CThread.h"
+#include "XScreen.h"
+#include <stdlib.h>
 #include <string.h>
 #include <assert.h>
 
@@ -13,29 +15,38 @@
 // CXWindowsScreen
 //
 
+CXWindowsScreen*		CXWindowsScreen::s_screen = NULL;
+
 CXWindowsScreen::CXWindowsScreen() :
 								m_display(NULL),
 								m_root(None),
 								m_w(0), m_h(0),
 								m_stop(false)
 {
-	// do nothing
+	assert(s_screen == NULL);
+	s_screen = this;
 }
 
 CXWindowsScreen::~CXWindowsScreen()
 {
+	assert(s_screen  != NULL);
 	assert(m_display == NULL);
+
+	s_screen = NULL;
 }
 
 void					CXWindowsScreen::openDisplay()
 {
 	assert(m_display == NULL);
 
+	// set the X I/O error handler so we catch the display disconnecting
+	XSetIOErrorHandler(&CXWindowsScreen::ioErrorHandler);
+
 	// open the display
 	log((CLOG_DEBUG "XOpenDisplay(%s)", "NULL"));
 	m_display = XOpenDisplay(NULL);	// FIXME -- allow non-default
 	if (m_display == NULL)
-		throw int(5);	// FIXME -- make exception for this
+		throw XScreenOpenFailure();
 
 	// get default screen
 	m_screen = DefaultScreen(m_display);
@@ -50,7 +61,7 @@ void					CXWindowsScreen::openDisplay()
 	m_root = RootWindow(m_display, m_screen);
 
 	// let subclass prep display
-	onOpenDisplay();
+	onOpenDisplay(m_display);
 
 	// initialize clipboards
 	for (ClipboardID id = 0; id < kClipboardEnd; ++id) {
@@ -60,10 +71,10 @@ void					CXWindowsScreen::openDisplay()
 
 void					CXWindowsScreen::closeDisplay()
 {
-	assert(m_display != NULL);
+	CLock lock(&m_mutex);
 
 	// let subclass close down display
-	onCloseDisplay();
+	onCloseDisplay(m_display);
 
 	// destroy clipboards
 	for (ClipboardID id = 0; id < kClipboardEnd; ++id) {
@@ -71,9 +82,12 @@ void					CXWindowsScreen::closeDisplay()
 	}
 
 	// close the display
+	if (m_display != NULL) {
 		XCloseDisplay(m_display);
 		m_display = NULL;
 		log((CLOG_DEBUG "closed display"));
+	}
+	XSetIOErrorHandler(NULL);
 }
 
 int						CXWindowsScreen::getScreen() const
@@ -164,7 +178,7 @@ bool					CXWindowsScreen::getEvent(XEvent* xevent) const
 
 void					CXWindowsScreen::doStop()
 {
-	CLock lock(&m_mutex);
+	// caller must have locked display
 	m_stop = true;
 }
 
@@ -179,6 +193,11 @@ ClipboardID				CXWindowsScreen::getClipboardID(Atom selection) const
 	return kClipboardEnd;
 }
 
+void					CXWindowsScreen::onUnexpectedClose()
+{
+	// do nothing
+}
+
 bool					CXWindowsScreen::processEvent(XEvent* xevent)
 {
 	switch (xevent->type) {
@@ -326,6 +345,21 @@ void					CXWindowsScreen::destroyClipboardRequest(
 	}
 }
 
+int						CXWindowsScreen::ioErrorHandler(Display*)
+{
+	// the display has disconnected, probably because X is shutting
+	// down.  X forces us to exit at this point.  that's arguably
+	// a flaw in X but, realistically, it's difficult to gracefully
+	// handle not having a Display* anymore.  we'll simply log the
+	// error, notify the subclass (which must not use the display
+	// so we set it to NULL), and exit.
+	log((CLOG_WARN "X display has unexpectedly disconnected"));
+	s_screen->m_display = NULL;
+	s_screen->onUnexpectedClose();
+	log((CLOG_CRIT "quiting due to X display disconnection"));
+	exit(1);
+}
+
 
 //
 // CXWindowsScreen::CDisplayLock

synergy/CXWindowsScreen.h

@@ -52,7 +52,8 @@ protected:
 	// wait for and get the next X event.  cancellable.
 	bool				getEvent(XEvent*) const;
 
-	// cause getEvent() to return false immediately and forever after
+	// cause getEvent() to return false immediately and forever after.
+	// the caller must have locked the display.
 	void				doStop();
 
 	// set the contents of the clipboard (i.e. primary selection)
@@ -63,15 +64,21 @@ protected:
 	bool				getDisplayClipboard(ClipboardID,
 								IClipboard* clipboard) const;
 
-	// called by openDisplay() to allow subclasses to prepare the display
+	// called by openDisplay() to allow subclasses to prepare the display.
-	virtual void		onOpenDisplay() = 0;
+	// the display is locked and passed to the subclass.
+	virtual void		onOpenDisplay(Display*) = 0;
 
 	// called by openDisplay() after onOpenDisplay() to create each clipboard
 	virtual CXWindowsClipboard*
 						createClipboard(ClipboardID) = 0;
 
-	// called by closeDisplay() to 
+	// called by closeDisplay() to allow subclasses to clean up the display.
-	virtual void		onCloseDisplay() = 0;
+	// the display is locked and passed to the subclass.  note that the
+	// display may be NULL if the display has unexpectedly disconnected.
+	virtual void		onCloseDisplay(Display*) = 0;
+
+	// called if the display is unexpectedly closing.  default does nothing.
+	virtual void		onUnexpectedClose();
 
 	// called when a clipboard is lost
 	virtual void		onLostClipboard(ClipboardID) = 0;
@@ -91,6 +98,9 @@ private:
 	// terminate a selection request
 	void				destroyClipboardRequest(Window window);
 
+	// X I/O error handler
+	static int			ioErrorHandler(Display*);
+
 private:
 	Display*			m_display;
 	int					m_screen;
@@ -103,6 +113,10 @@ private:
 
 	// X is not thread safe
 	CMutex				m_mutex;
+
+	// pointer to (singleton) screen.  this is only needed by
+	// ioErrorHandler().
+	static CXWindowsScreen*	s_screen;
 };
 
 #endif

synergy/Makefile

@@ -24,6 +24,7 @@ CXXFILES = 				\
 	CXWindowsClipboard.cpp		\
 	CXWindowsScreen.cpp		\
 	CXWindowsUtil.cpp		\
+	XScreen.cpp			\
 	XSynergy.cpp			\
 	$(NULL)
 

synergy/XScreen.cpp

@@ -0,0 +1,10 @@
+#include "XScreen.h"
+
+//
+// XScreenOpenFailure
+//
+
+CString					XScreenOpenFailure::getWhat() const throw()
+{
+	return "XScreenOpenFailure";
+}

synergy/XScreen.h

@@ -0,0 +1,14 @@
+#ifndef XSCREEN_H
+#define XSCREEN_H
+
+#include "XBase.h"
+
+class XScreen : public XBase { };
+
+// screen cannot be opened
+class XScreenOpenFailure : public XScreen {
+protected:
+	virtual CString		getWhat() const throw();
+};
+
+#endif

synergy/synergy.dsp

@@ -115,6 +115,10 @@ SOURCE=.\CTCPSocketFactory.cpp
 # End Source File
 # Begin Source File
 
+SOURCE=.\XScreen.cpp
+# End Source File
+# Begin Source File
+
 SOURCE=.\XSynergy.cpp
 # End Source File
 # End Group
@@ -187,6 +191,10 @@ SOURCE=.\ProtocolTypes.h
 # End Source File
 # Begin Source File
 
+SOURCE=.\XScreen.h
+# End Source File
+# Begin Source File
+
 SOURCE=.\XSynergy.h
 # End Source File
 # End Group