r3231 - in trunk/libmnetutil: include/libmnetutil source

[erik at minisip.org]

Author: erik
Date: 2007-03-07 20:53:09 +0100 (Wed, 07 Mar 2007)
New Revision: 3231

Modified:
   trunk/libmnetutil/include/libmnetutil/SocketServer.h
   trunk/libmnetutil/source/SocketServer.cxx
   trunk/libmnetutil/source/init.cxx
Log:

 * libmsip: Fix race condition in SocketServer

   When SocketServer::start is called it creates
   a thread that executes the "run" method. This
   method initializes internal data that is
   used in the signal method. If the stop method
   is called before the "run" has initialized the
   internal data, the thread will not stop running
   and "stop" will block indefinetly.

   Example code that often hangs:
     MRef<SipStack*> s = new SipStack( config );
     s->startTcpServer();
     s->stopRunning(); //This can block indefinetly - race

   This fix initializes the internal data in the start method.
   If the start method is not called and you instead call
   the run method directly, then the run method will initialize
   the data.


 * Minor change: I think we should use the LIBMNETUTIL_API not
   only in the header file. I don't remember if it is required
   in the .cxx file as well by the Microsoft compiler, but we
   have done it in the rest of the code.





Modified: trunk/libmnetutil/include/libmnetutil/SocketServer.h
===================================================================
--- trunk/libmnetutil/include/libmnetutil/SocketServer.h	2007-03-07 09:55:29 UTC (rev 3230)
+++ trunk/libmnetutil/include/libmnetutil/SocketServer.h	2007-03-07 19:53:09 UTC (rev 3231)
@@ -63,11 +63,16 @@
 		int buildFdSet( fd_set *set, int pipeFd );
 
 	private:
+		void createSignalPipe();
 		typedef std::map< MRef<Socket*>, MRef<InputReadyHandler*> > Sockets;
 		Mutex csMutex;
 		MRef<Thread *> thread;
 		Sockets sockets;
-		int fdSignal;
+
+		int fdSignal;		///Socket pair used to wake thread running run()
+		int fdSignalInternal;	///from its select. This is typically used to
+					///to tell the thread to stop.
+
 		bool doStop;
 };
 #endif

Modified: trunk/libmnetutil/source/SocketServer.cxx
===================================================================
--- trunk/libmnetutil/source/SocketServer.cxx	2007-03-07 09:55:29 UTC (rev 3230)
+++ trunk/libmnetutil/source/SocketServer.cxx	2007-03-07 19:53:09 UTC (rev 3231)
@@ -84,11 +84,34 @@
 	stop();
 }
 
+void SocketServer::createSignalPipe(){
+	int pipeFds[2] = {-1,-1};
+
+	if( fdSignal >= 0 ){
+		close( fdSignal );
+		fdSignal = -1;
+	}
+
+#ifdef WIN32
+	// Use TCP sockets since Windows pipes don't support select
+	if( createTcpPipe( pipeFds )) {
+#else
+	if( ::pipe( pipeFds ) ){
+#endif
+		throw Exception( "Can't create pipe" );
+	}
+
+	fdSignal = pipeFds[1];
+	fdSignalInternal = pipeFds[0];
+}
+
 void SocketServer::start()
 {
 	CriticalSection cs( csMutex );
 	if( !thread.isNull() )
 		return;
+	if (fdSignal < 0)
+		createSignalPipe();
 
 	thread = new Thread( this );
 }
@@ -154,6 +177,7 @@
 
 void SocketServer::signal()
 {
+
 	char c = 0;
 
 	if( fdSignal < 0 )
@@ -255,32 +279,14 @@
 	struct timeval timeout;
 	fd_set tmpl;
 	fd_set set;
-	int pipeFds[2] = {-1,-1};
 	int maxFd = -1;
 	Sockets::const_iterator i;
 
-	csMutex.lock();
+	if (fdSignal < 0)
+		createSignalPipe();
 
-	if( fdSignal >= 0 ){
-		close( fdSignal );
-		fdSignal = -1;
-	}
-
-#ifdef WIN32
-	// Use TCP sockets since Windows pipes don't support select
-	if( createTcpPipe( pipeFds )) {
-#else
-	if( ::pipe( pipeFds ) ){
-#endif
-		throw Exception( "Can't create pipe" );
-	}
-
-	fdSignal = pipeFds[1];
-
-	csMutex.unlock();
-
 	while (!doStop){
-		maxFd = buildFdSet( &tmpl, pipeFds[0] );
+		maxFd = buildFdSet( &tmpl, fdSignalInternal );
 
 		int avail;
 		do{
@@ -297,13 +303,13 @@
 // 			cerr<< "SipSocketServer::run(): Timeout"<< endl;
 		}
 
-		if( FD_ISSET( pipeFds[0], &set ) ){
+		if( FD_ISSET( fdSignalInternal, &set ) ){
 			char buf[255];
 
 #ifdef WIN32
-			if( recv( pipeFds[0], buf, sizeof(buf), 0 ) < 0){
+			if( recv( fdSignalInternal, buf, sizeof(buf), 0 ) < 0){
 #else
-			if( read( pipeFds[0], buf, sizeof(buf) ) < 0){
+			if( read( fdSignalInternal, buf, sizeof(buf) ) < 0){
 #endif
 				cerr << "Read failed" << endl;
 				throw NetworkException( errno );
@@ -325,9 +331,9 @@
 
 // 	csMutex.lock();
 
+	closesocket( fdSignalInternal );
+	closesocket( fdSignal );
 	fdSignal = -1;
-	closesocket( pipeFds[0] );
-	closesocket( pipeFds[1] );
 	doStop = false;
 
 // 	csMutex.unlock();

Modified: trunk/libmnetutil/source/init.cxx
===================================================================
--- trunk/libmnetutil/source/init.cxx	2007-03-07 09:55:29 UTC (rev 3230)
+++ trunk/libmnetutil/source/init.cxx	2007-03-07 19:53:09 UTC (rev 3231)
@@ -29,7 +29,7 @@
 
 static unsigned int g_initialized;
 
-void libmnetutilInit()
+LIBMNETUTIL_API void libmnetutilInit()
 {
 	if( g_initialized++ > 0 )
 		return;
@@ -40,8 +40,10 @@
 	}
 }
 
-void libmnetutilUninit()
+LIBMNETUTIL_API void libmnetutilUninit()
 {
 	if( --g_initialized )
 		return;
 }
+
+