* new file lib/UnlockGuard.h — unlock_guard is for unlocking a mutex for the scope time (RAII)

* all lock/unlock and unlock/lock pairs are done by RAII guards now
* fixed two possible crashes at the end of battle when last stack was killed by spell. That should fix #749 and #752.
* fixed a very nasty race condition, eliminating possible deadlock at the start of battle when human hero has tactics
* fixed #422

AI/VCAI/VCAI.cpp

@@ -1,5 +1,6 @@
 #include "StdInc.h"
 #include "VCAI.h"
+#include "../../lib/UnlockGuard.h"
 
 #define I_AM_ELEMENTAR return CGoal(*this).setisElementar(true)
 
@@ -1093,9 +1094,8 @@ void VCAI::battleEnd(const BattleResult *br)
 
 void VCAI::waitTillFree()
 {
-	cb->getGsMutex().unlock_shared();
+	auto unlock = vstd::makeUnlockSharedGuard(cb->getGsMutex());
 	status.waitTillFree();
-	cb->getGsMutex().lock_shared();
 }
 
 void VCAI::validateVisitableObjs()

CCallback.cpp

@@ -25,6 +25,7 @@
 #ifdef max
 #undef max
 #endif
+#include "lib/UnlockGuard.h"
 
 /*
  * CCallback.cpp, part of VCMI engine
@@ -230,11 +231,11 @@ void CBattleCallback::sendRequest(const CPack* request)
 
 	if(waitTillRealize)
 	{
+		std::unique_ptr<vstd::unlock_shared_guard> unlocker; //optional, if flag set
 		if(unlockGsWhenWaiting)
-			getGsMutex().unlock_shared();
+			unlocker = vstd::make_unique<vstd::unlock_shared_guard>(vstd::makeUnlockSharedGuard(getGsMutex()));
+
 		cl->waitingRequest.waitWhileTrue();
-		if(unlockGsWhenWaiting)
-			getGsMutex().lock_shared();
 	}
 }
 

Global.h

@@ -289,6 +289,17 @@ namespace vstd
 		delete ptr;
 		ptr = NULL;
 	}
+
+	template<typename T>
+	std::unique_ptr<T> make_unique()
+	{
+		return std::unique_ptr<T>(new T());
+	}
+	template<typename T, typename Arg1>
+	std::unique_ptr<T> make_unique(Arg1&& arg1)
+	{
+		return std::unique_ptr<T>(new T(std::forward<Arg1>(arg1)));
+	}
 }
 using vstd::operator-=;
 

client/BattleInterface/CBattleInterface.cpp

@@ -39,7 +39,7 @@ const double M_PI = 3.14159265358979323846;
 #define _USE_MATH_DEFINES
 #include <cmath>
 #endif
-#include <boost/format.hpp>
+#include "../../lib/UnlockGuard.h"
 
 const time_t CBattleInterface::HOVER_ANIM_DELTA = 1;
 
@@ -367,6 +367,10 @@ CBattleInterface::CBattleInterface(const CCreatureSet * army1, const CCreatureSe
 
 CBattleInterface::~CBattleInterface()
 {
+	curInt->battleInt = NULL;
+	givenCommand->cond.notify_all(); //that two lines should make any activeStack waiting thread to finish
+
+
 	if (active) //dirty fix for #485
 	{
 		deactivate();
@@ -408,7 +412,6 @@ CBattleInterface::~CBattleInterface()
 		delete g->second;
 
 	delete siegeH;
-	curInt->battleInt = NULL;
 
 	//TODO: play AI tracks if battle was during AI turn
 	//if (!curInt->makingTurn)
@@ -1557,6 +1560,7 @@ void CBattleInterface::stackActivated(const CStack * stack) //TODO: check it all
 	stackToActivate = stack;
 	waitForAnims();
 	//if(pendingAnims.size() == 0)
+	if(stackToActivate) //during waiting stack may have gotten activated through show
 		activateStack();
 }
 
@@ -2053,10 +2057,12 @@ void CBattleInterface::stackIsCatapulting(const CatapultAttack & ca)
 void CBattleInterface::battleFinished(const BattleResult& br)
 {
 	bresult = &br;
-	LOCPLINT->pim->unlock();
+	{
+		auto unlockPim = vstd::makeUnlockGuard(*LOCPLINT->pim);
 		animsAreDisplayed.waitUntil(false);
-	LOCPLINT->pim->lock();
+	}
 	displayBattleFinished();
+	activeStack = NULL;
 }
 
 void CBattleInterface::displayBattleFinished()
@@ -3045,9 +3051,8 @@ void CBattleInterface::startAction(const BattleAction* action)
 
 void CBattleInterface::waitForAnims()
 {
-	LOCPLINT->pim->unlock();
+	auto unlockPim = vstd::makeUnlockGuard(*LOCPLINT->pim);
 	animsAreDisplayed.waitWhileTrue();
-	LOCPLINT->pim->lock();
 }
 
 void CBattleInterface::bEndTacticPhase()

client/CAdvmapInterface.cpp

@@ -30,6 +30,7 @@
 #include "CMusicHandler.h"
 #include "UIFramework/CGuiHandler.h"
 #include "UIFramework/CIntObjectClasses.h"
+#include "../lib/UnlockGuard.h"
 
 #ifdef _MSC_VER
 #pragma warning (disable : 4355)
@@ -1463,9 +1464,8 @@ void CAdvMapInt::keyPressed(const SDL_KeyboardEvent & key)
 				return;
 			if(h && key.state == SDL_PRESSED)
 			{
-				LOCPLINT->pim->unlock();
+				auto unlockPim = vstd::makeUnlockGuard(*LOCPLINT->pim);
 				LOCPLINT->cb->moveHero(h,h->pos);
-				LOCPLINT->pim->lock();
 			}
 		}
 		return;

client/CMT.cpp

@@ -43,6 +43,7 @@
 #include "SDL_syswm.h"
 #endif
 #include "../lib/CDefObjInfoHandler.h"
+#include "../lib/UnlockGuard.h"
 
 #if __MINGW32__
 #undef main
@@ -543,6 +544,13 @@ void processCommand(const std::string &message)
 		readed >> fname;
 		startGameFromFile(fname);
 	}
+	else if(cn == "unlock")
+	{
+		std::string mxname;
+		readed >> mxname;
+		if(mxname == "pim" && LOCPLINT)
+			LOCPLINT->pim->unlock();
+	}
 	else if(client && client->serv && client->serv->connected && LOCPLINT) //send to server
 	{
 		boost::unique_lock<boost::recursive_mutex> un(*LOCPLINT->pim);
@@ -709,9 +717,10 @@ static void listenForEvents()
 		} 
 
 		//tlog0 << " pushing ";
-		eventsM.lock();
+		{
+			boost::unique_lock<boost::mutex> lock(eventsM); 
 			events.push(ev);
-		eventsM.unlock();
+		}
 		//tlog0 << " done\n";
 	}
 }
@@ -735,9 +744,8 @@ void startGame(StartInfo * options, CConnection *serv/* = NULL*/)
 		requestChangingResolution();
 
 		//allow event handling thread change resolution
-		eventsM.unlock();
+		auto unlock = vstd::makeUnlockGuard(eventsM);
 		while(!setResolution) boost::this_thread::sleep(boost::posix_time::milliseconds(50));
-		eventsM.lock();
 	}
 	else
 		setResolution = true;

client/CPlayerInterface.cpp

@@ -37,6 +37,7 @@
 #include "../lib/CGameState.h"
 #include "../lib/GameConstants.h"
 #include "UIFramework/CGuiHandler.h"
+#include "../lib/UnlockGuard.h"
 
 #ifdef min
 #undef min
@@ -740,7 +741,11 @@ BattleAction CPlayerInterface::activeStack(const CStack * stack) //called when i
 	//wait till BattleInterface sets its command
 	boost::unique_lock<boost::mutex> lock(b->givenCommand->mx);
 	while(!b->givenCommand->data)
+	{
 		b->givenCommand->cond.wait(lock);
+		if(!battleInt) //batle ended while we were waiting for movement (eg. because of spell)
+			throw boost::thread_interrupted(); //will shut the thread peacefully
+	}
 
 	//tidy up
 	BattleAction ret = *(b->givenCommand->data);
@@ -1158,11 +1163,12 @@ bool CPlayerInterface::moveHero( const CGHeroInstance *h, CGPath path )
 	}
 
 	int i = 1;
+	bool result = false; //TODO why not set to true anywhere?
+	{
 		//evil...
-	eventsM.unlock();
-	pim->unlock();
-	cb->getGsMutex().unlock_shared();
-	bool result = false;
+		auto unlockEvents = vstd::makeUnlockGuard(eventsM);
+		auto unlockPim = vstd::makeUnlockGuard(*pim);
+		auto unlockGs = vstd::makeUnlockSharedGuard(cb->getGsMutex());
 
 		{
 			path.convert(0);
@@ -1222,9 +1228,8 @@ bool CPlayerInterface::moveHero( const CGHeroInstance *h, CGPath path )
 			CCS->soundh->stopSound(sh);
 		}
 
-	cb->getGsMutex().lock_shared();
-	pim->lock();
-	eventsM.lock();
+		//RAII unlocks
+	}
 
 	if (adventureInt)
 	{
@@ -1262,9 +1267,8 @@ void CPlayerInterface::showGarrisonDialog( const CArmedInstance *up, const CGHer
 	boost::unique_lock<boost::recursive_mutex> un(*pim);
 	while(dialogs.size())
 	{
-		pim->unlock();
+		auto unlock = vstd::makeUnlockGuard(*pim);
 		SDL_Delay(20);
-		pim->lock();
 	}
 	CGarrisonWindow *cgw = new CGarrisonWindow(up,down,removableUnits);
 	cgw->quit->callback += onEnd;
@@ -1471,6 +1475,7 @@ void CPlayerInterface::update()
 	if(terminate_cond.get())
 		return;
 
+	boost::unique_lock<boost::recursive_mutex> un(*pim, boost::adopt_lock); //create lock from owned mutex (defer_lock)
 	//make sure that gamestate won't change when GUI objects may obtain its parts on event processing or drawing request
 	boost::shared_lock<boost::shared_mutex> gsLock(cb->getGsMutex());
 
@@ -1485,7 +1490,6 @@ void CPlayerInterface::update()
 	//in some conditions we may receive calls before selection is initialized - we must ignore them
 	if(adventureInt && !adventureInt->selection && GH.topInt() == adventureInt)
 	{
-		pim->unlock();
 		return;
 	}
 
@@ -1505,8 +1509,6 @@ void CPlayerInterface::update()
 	CCS->curh->draw1();
 	CSDL_Ext::update(screen);
 	CCS->curh->draw2();
-
-	pim->unlock();
 }
 
 int CPlayerInterface::getLastIndex( std::string namePrefix)

client/Client.cpp

@@ -123,7 +123,13 @@ void CClient::waitForMoveAndSend(int color)
 		MakeAction temp_action(ba);
 		serv->sendPackToServer(temp_action, color);
 		return;
-	}HANDLE_EXCEPTION
+	}
+	catch(boost::thread_interrupted&)
+	{
+		tlog5 << "Wait for move thread was interrupted and no action will be send. Was a battle ended by spell?\n";
+		return;
+	}
+	HANDLE_EXCEPTION
 	tlog1 << "We should not be here!" << std::endl;
 }
 
@@ -182,8 +188,8 @@ void CClient::endGame( bool closeConnection /*= true*/ )
 
 	GH.curInt = NULL;
 	LOCPLINT->terminate_cond.setn(true);
-	LOCPLINT->pim->lock();
-
+	{
+		boost::unique_lock<boost::recursive_mutex> un(*LOCPLINT->pim);
 		tlog0 << "\n\nEnding current game!" << std::endl;
 		if(GH.topInt())
 			GH.topInt()->deactivate();
@@ -198,10 +204,8 @@ void CClient::endGame( bool closeConnection /*= true*/ )
 
 		const_cast<CGameInfo*>(CGI)->state.dellNull();
 		tlog0 << "Deleted mapHandler and gameState." << std::endl;
-
-	CPlayerInterface * oldInt = LOCPLINT;
 		LOCPLINT = NULL;
-	oldInt->pim->unlock();
+	}
 	while (!playerint.empty())
 	{
 		CGameInterface *pint = playerint.begin()->second;
@@ -560,9 +564,12 @@ void CClient::battleStarted(const BattleInfo * info)
 		def = NULL;
 
 	if(att || def || gs->scenarioOps->mode == StartInfo::DUEL)
+	{
+		boost::unique_lock<boost::recursive_mutex> un(*LOCPLINT->pim);
 		new CBattleInterface(info->belligerents[0], info->belligerents[1], info->heroes[0], info->heroes[1],
 			Rect((settings["video"]["gameRes"]["width"].Float()  - 800)/2, 
 			     (settings["video"]["gameRes"]["height"].Float() - 600)/2, 800, 600), att, def);
+	}
 
 	if(vstd::contains(battleints,info->sides[0]))
 		battleints[info->sides[0]]->battleStart(info->belligerents[0], info->belligerents[1], info->tile, info->heroes[0], info->heroes[1], 0);

client/GUIClasses.cpp

@@ -3988,8 +3988,7 @@ void CInGameConsole::show(SDL_Surface * to)
 
 	std::vector<std::list< std::pair< std::string, int > >::iterator> toDel;
 
-	texts_mx.lock();
-
+	boost::unique_lock<boost::mutex> lock(texts_mx); 
 	for(std::list< std::pair< std::string, int > >::iterator it = texts.begin(); it != texts.end(); ++it, ++number)
 	{
 		SDL_Color green = {0,0xff,0,0};
@@ -4009,13 +4008,11 @@ void CInGameConsole::show(SDL_Surface * to)
 	{
 		texts.erase(toDel[it]);
 	}
-
-	texts_mx.unlock();
 }
 
 void CInGameConsole::print(const std::string &txt)
 {
-	texts_mx.lock();
+	boost::unique_lock<boost::mutex> lock(texts_mx); 
 	int lineLen = conf.go()->ac.outputLineLength;
 
 	if(txt.size() < lineLen)
@@ -4042,8 +4039,6 @@ void CInGameConsole::print(const std::string &txt)
 			}
 		}
 	}
-
-	texts_mx.unlock();
 }
 
 void CInGameConsole::keyPressed (const SDL_KeyboardEvent & key)

lib/CGameState.cpp

@@ -73,11 +73,9 @@ public:
 	void applyOnGS(CGameState *gs, void *pack) const
 	{
 		T *ptr = static_cast<T*>(pack);
-		gs->mx->lock();
-// 		while(!gs->mx->try_lock())
-// 			boost::this_thread::sleep(boost::posix_time::milliseconds(1)); //give other threads time to finish
+
+		boost::unique_lock<boost::shared_mutex> lock(*gs->mx); 
 		ptr->applyGs(gs);
-		gs->mx->unlock();
 	}
 };
 

lib/CLodHandler.cpp

@@ -59,7 +59,9 @@ ui8 * CLodHandler::giveFile(std::string fname, LodFileType type, int * length)
 	Entry ourEntry = *en_it;
 
 	if(length) *length = ourEntry.realSize;
-	mutex->lock();
+
+
+	boost::unique_lock<boost::mutex> lock(*mutex);
 
 	ui8 * outp;
 	if (ourEntry.offset<0) //file is in the sprites/ folder; no compression
@@ -74,7 +76,6 @@ ui8 * CLodHandler::giveFile(std::string fname, LodFileType type, int * length)
 		}
 		else
 			result = -1;
-		mutex->unlock();
 		if(result<0)
 		{
 			tlog1<<"Error in file reading: " << myDir << "/" << ourEntry.name << std::endl;
@@ -90,7 +91,6 @@ ui8 * CLodHandler::giveFile(std::string fname, LodFileType type, int * length)
 
 		LOD.seekg(ourEntry.offset, std::ios::beg);
 		LOD.read((char*)outp, ourEntry.realSize);
-		mutex->unlock();
 		return outp;
 	}
 	else //we will decompress file
@@ -101,7 +101,6 @@ ui8 * CLodHandler::giveFile(std::string fname, LodFileType type, int * length)
 		LOD.read((char*)outp, ourEntry.size);
 		ui8 * decomp = NULL;
 		infs2(outp, ourEntry.size, ourEntry.realSize, decomp);
-		mutex->unlock();
 		delete[] outp;
 		return decomp;
 	}

lib/CThreadHelper.cpp

@@ -32,18 +32,14 @@ void CThreadHelper::processTasks()
 	int pom;
 	while(true)
 	{
-		rtinm.lock();
-		if((pom=currentTask) >= amount)
 		{
-			rtinm.unlock();
+			boost::unique_lock<boost::mutex> lock(rtinm); 
+			if((pom = currentTask) >= amount)
 				break;
-		}
 			else
-		{
 				++currentTask;
-			rtinm.unlock();
-			(*tasks)[pom]();
 		}
+		(*tasks)[pom]();
 	}
 }
 

lib/CondSh.h

@@ -27,16 +27,16 @@ template <typename T> struct CondSh
 
 	void set(T t)
 	{
-		mx.lock();
+		boost::unique_lock<boost::mutex> lock(mx); 
 		data=t;
-		mx.unlock();
 	} 
 
 	void setn(T t) //set data and notify
 	{
-		mx.lock();
+		{
+			boost::unique_lock<boost::mutex> lock(mx); 
 			data=t;
-		mx.unlock();
+		}
 		cond.notify_all();
 	};
 

lib/Interprocess.h

@@ -28,9 +28,10 @@ struct ServerReady
 
 	void setToTrueAndNotify()
 	{
-		mutex.lock();
+		{
+			boost::unique_lock<boost::interprocess::interprocess_mutex> lock(mutex); 
 			ready = true;
-		mutex.unlock();
+		}
 		cond.notify_all();
 	}
 };

lib/UnlockGuard.h

@@ -0,0 +1,83 @@
+#pragma once
+
+namespace vstd
+{
+	namespace detail
+	{
+		template<typename Mutex>
+		class unlock_policy
+		{
+		protected:
+			void unlock(Mutex &m)
+			{
+				m.unlock();
+			}
+			void lock(Mutex &m)
+			{
+				m.lock();
+			}
+		};
+
+		template<typename Mutex>
+		class unlock_shared_policy
+		{
+		protected:
+			void unlock(Mutex &m)
+			{
+				m.unlock_shared();
+			}
+			void lock(Mutex &m)
+			{
+				m.lock_shared();
+			}
+		};
+	}
+
+
+	//similar to boost::lock_guard but UNlocks for the scope + assertions
+	template<typename Mutex, typename LockingPolicy = detail::unlock_policy<Mutex> >
+	class unlock_guard : LockingPolicy
+	{
+	private:
+		Mutex* m;
+
+		explicit unlock_guard(unlock_guard&);
+		unlock_guard& operator=(unlock_guard&);
+	public:
+		explicit unlock_guard(Mutex& m_):
+		m(&m_)
+		{
+			unlock(*m);
+		}
+
+		unlock_guard(unlock_guard &&other)
+			: m(other.m)
+		{
+			other.m = NULL;
+		}
+
+		void release()
+		{
+			m = NULL;
+		}
+
+		~unlock_guard()
+		{
+			if(m)
+				lock(*m);
+		}
+	};
+
+	template<typename Mutex>
+	unlock_guard<Mutex, detail::unlock_policy<Mutex> > makeUnlockGuard(Mutex &m_)
+	{
+		return unlock_guard<Mutex, detail::unlock_policy<Mutex> >(m_);
+	}
+	template<typename Mutex>
+	unlock_guard<Mutex, detail::unlock_shared_policy<Mutex> > makeUnlockSharedGuard(Mutex &m_)
+	{
+		return unlock_guard<Mutex, detail::unlock_shared_policy<Mutex> >(m_);
+	}
+
+	typedef unlock_guard<boost::shared_mutex, detail::unlock_shared_policy<boost::shared_mutex> > unlock_shared_guard;
+}

lib/VCMI_lib.vcxproj

@@ -305,6 +305,7 @@
     <ClInclude Include="ResourceSet.h" />
     <ClInclude Include="StartInfo.h" />
     <ClInclude Include="StdInc.h" />
+    <ClInclude Include="UnlockGuard.h" />
     <ClInclude Include="VCMI_Lib.h" />
     <ClInclude Include="VCMIDirs.h" />
   </ItemGroup>

server/CGameHandler.cpp

@@ -4713,6 +4713,7 @@ bool CGameHandler::dig( const CGHeroInstance *h )
 		giveHeroNewArtifact(h, VLC->arth->artifacts[2], -1); //give grail
 		sendAndApply(&iw);
 
+		iw.soundID = soundBase::invalid;
 		iw.text.clear();
 		iw.text.addTxt(MetaString::ART_DESCR, 2);
 		sendAndApply(&iw);

server/CVCMIServer.cpp

@@ -33,6 +33,7 @@
 #include "../lib/GameConstants.h"
 
 #include <boost/asio.hpp>
+#include "../lib/UnlockGuard.h"
 
 std::string NAME_AFFIX = "server";
 std::string NAME = GameConstants::VCMI_VERSION + std::string(" (") + NAME_AFFIX + ')'; //application name
@@ -97,9 +98,8 @@ void CPregameServer::handleConnection(CConnection *cpc)
 			if(startingGame)
 			{
 				//wait for sending thread to announce start
-				mx.unlock();
+				auto unlock = vstd::makeUnlockGuard(mx);
 				while(state == RUNNING) boost::this_thread::sleep(boost::posix_time::milliseconds(50));
-				mx.lock();
 			}
 		}
 	}