Thread safety fixes to avoid crashes on rehash, dont reopen logs within the rehash thread. Put this in the safe part of the rehash operation, after the thread exits. Put a mutex around the part where the thread exits, just in case somehow there are two rehash threads exiting at the same time due to user

muppetry.


git-svn-id: http://svn.inspircd.org/repository/trunk/inspircd@10811 e03df62e-2008-0410-955e-edbf42e46eb7

3 files changed, 25 insertions, 17 deletions

diff --git a/src/configreader.cpp b/src/configreader.cpp
index 3be7d5945..9978f24d7 100644
--- a/src/configreader.cpp
+++ b/src/configreader.cpp
@@ -1227,12 +1227,6 @@ void ServerConfig::Read(bool bail, const std::string &useruid)
 	// write once here, to try it out and make sure its ok
 	ServerInstance->WritePID(this->PID);
 
-	/* Switch over logfiles */
-	ServerInstance->Logs->CloseLogs();
-	ServerInstance->Logs->OpenFileLogs();
-
-	ServerInstance->Logs->Log("CONFIG", DEFAULT, "Done reading configuration file.");
-
 	/* If we're rehashing, let's load any new modules, and unload old ones
 	 */
 	if (!bail)
@@ -1323,9 +1317,6 @@ void ServerConfig::Read(bool bail, const std::string &useruid)
 				}
 			}
 		}
-
-		ServerInstance->Logs->Log("CONFIG", DEFAULT, "Successfully unloaded %lu of %lu modules and loaded %lu of %lu modules.",(unsigned long)rem,(unsigned long)removed_modules.size(),(unsigned long)add,(unsigned long)added_modules.size());
-
 		ServerInstance->Threads->Unlock();
 
 	}
diff --git a/src/inspircd.cpp b/src/inspircd.cpp
index 1b173e56e..4d112b314 100644
--- a/src/inspircd.cpp
+++ b/src/inspircd.cpp
@@ -220,6 +220,8 @@ void InspIRCd::Cleanup()
 		delete this->Logs;
 		this->Logs = 0;
 	}
+
+	delete RehashFinishMutex;
 }
 
 void InspIRCd::Restart(const std::string &reason)
@@ -631,6 +633,8 @@ InspIRCd::InspIRCd(int argc, char** argv)
 	ConfigThread->Run();
 	delete ConfigThread;
 	this->ConfigThread = NULL;
+        /* Switch over logfiles */
+	Logs->OpenFileLogs();
 
 	/** Note: This is safe, the method checks for user == NULL */
 	this->Parser->SetupCommandTable();
@@ -806,6 +810,8 @@ int InspIRCd::Run()
 		Exit(0);
 	}
 
+	RehashFinishMutex = Mutexes->CreateMutex();
+
 	while (true)
 	{
 #ifndef WIN32
@@ -817,21 +823,22 @@ int InspIRCd::Run()
 #endif
 
 		/* Check if there is a config thread which has finished executing but has not yet been freed */
+		RehashFinishMutex->Lock();
 		if (this->ConfigThread && this->ConfigThread->GetExitFlag())
 		{
 			/* Rehash has completed */
-			this->Logs->Log("CONFIG",DEBUG,"Detected ConfigThread exiting, tidying up...");
 
-			/* IMPORTANT: This delete may hang if you fuck up your thread syncronization.
-			 * It will hang waiting for the ConfigThread to 'join' to avoid race conditons,
-			 * until the other thread is completed.
-			 */
-			delete ConfigThread;
-			ConfigThread = NULL;
+			/* Switch over logfiles */
+			Logs->CloseLogs();
+			Logs->OpenFileLogs();
+
+			this->Logs->Log("CONFIG",DEBUG,"Detected ConfigThread exiting, tidying up...");
 
 			/* These are currently not known to be threadsafe, so they are executed outside
 			 * of the thread. It would be pretty simple to move them to the thread Run method
-			 * once they are known threadsafe with all the correct mutexes in place.
+			 * once they are known threadsafe with all the correct mutexes in place. This might
+			 * not be worth the effort however as these functions execute relatively quickly
+			 * and would not benefit from being within the config read thread.
 			 *
 			 * XXX: The order of these is IMPORTANT, do not reorder them without testing
 			 * thoroughly!!!
@@ -844,7 +851,15 @@ int InspIRCd::Run()
 			User* user = !Config->RehashUserUID.empty() ? FindNick(Config->RehashUserUID) : NULL;
 			FOREACH_MOD_I(this, I_OnRehash, OnRehash(user, Config->RehashParameter));
 			this->BuildISupport();
+
+			/* IMPORTANT: This delete may hang if you fuck up your thread syncronization.
+			 * It will hang waiting for the ConfigThread to 'join' to avoid race conditons,
+			 * until the other thread is completed.
+			 */
+			delete ConfigThread;
+			ConfigThread = NULL;
 		}
+		RehashFinishMutex->Unlock();
 
 		/* time() seems to be a pretty expensive syscall, so avoid calling it too much.
 		 * Once per loop iteration is pleanty.
diff --git a/src/server.cpp b/src/server.cpp
index c3efc7e6b..44f2ce78f 100644
--- a/src/server.cpp
+++ b/src/server.cpp
@@ -48,6 +48,7 @@ void InspIRCd::Exit(int status)
 
 void RehashHandler::Call(const std::string &reason)
 {
+	Server->RehashFinishMutex->Lock();
 	Server->SNO->WriteToSnoMask('A', "Rehashing config file %s %s",ServerConfig::CleanFilename(Server->ConfigFileName), reason.c_str());
 	Server->RehashUsersAndChans();
 	FOREACH_MOD_I(Server, I_OnGarbageCollect, OnGarbageCollect());
@@ -59,6 +60,7 @@ void RehashHandler::Call(const std::string &reason)
 		Server->ConfigThread = new ConfigReaderThread(Server, false, "");
 		Server->Threads->Create(Server->ConfigThread);
 	}
+	Server->RehashFinishMutex->Unlock();
 }
 
 void InspIRCd::RehashServer()