Timer changes and TimerManager enhancements

Timer::Tick() now has a bool return value: if false is returned the timer is deleted using operator delete, otherwise, if it's a repeating timer then it's rescheduled (readded)
Timers are removed from the TimerManager automatically at destruction

Timers are now stored in a multimap instead of a sorted vector

9 files changed, 78 insertions, 73 deletions

diff --git a/include/inspsocket.h b/include/inspsocket.h
index 75c6dc511..ccc2301ed 100644
--- a/include/inspsocket.h
+++ b/include/inspsocket.h
@@ -92,7 +92,7 @@ class CoreExport SocketTimeout : public Timer
 
 	/** Handle tick event
 	 */
-	virtual void Tick(time_t now);
+	virtual bool Tick(time_t now);
 };
 
 /**
diff --git a/include/timer.h b/include/timer.h
index 0dda6876e..6f1834178 100644
--- a/include/timer.h
+++ b/include/timer.h
@@ -21,6 +21,8 @@
 
 #pragma once
 
+class Module;
+
 /** Timer class for one-second resolution timers
  * Timer provides a facility which allows module
  * developers to create one-shot timers. The timer
@@ -28,61 +30,70 @@
  * resolution. To use Timer, inherit a class from
  * Timer, then insert your inherited class into the
  * queue using Server::AddTimer(). The Tick() method of
- * your object (which you should override) will be called
+ * your object (which you have to override) will be called
  * at the given time.
  */
 class CoreExport Timer
 {
- private:
 	/** The triggering time
 	 */
 	time_t trigger;
+
 	/** Number of seconds between triggers
 	 */
-	long secs;
+	unsigned int secs;
+
 	/** True if this is a repeating timer
 	 */
 	bool repeat;
+
  public:
 	/** Default constructor, initializes the triggering time
+	 * @param mod The module that created this timer
 	 * @param secs_from_now The number of seconds from now to trigger the timer
 	 * @param now The time now
 	 * @param repeating Repeat this timer every secs_from_now seconds if set to true
 	 */
-	Timer(long secs_from_now, time_t now, bool repeating = false)
+	Timer(unsigned int secs_from_now, time_t now, bool repeating = false)
 	{
 		trigger = now + secs_from_now;
 		secs = secs_from_now;
 		repeat = repeating;
 	}
 
-	/** Default destructor, does nothing.
+	/** Default destructor, removes the timer from the timer manager
 	 */
-	virtual ~Timer() { }
+	virtual ~Timer();
 
 	/** Retrieve the current triggering time
 	 */
-	virtual time_t GetTimer()
+	time_t GetTrigger() const
 	{
 		return trigger;
 	}
 
 	/** Sets the trigger timeout to a new value
+	 * This does not update the bookkeeping in TimerManager, use SetInterval()
+	 * to change the interval between ticks while keeping TimerManager updated
 	 */
-	virtual void SetTimer(time_t t)
+	void SetTrigger(time_t nexttrigger)
 	{
-		trigger = t;
+		trigger = nexttrigger;
 	}
 
+	/** Sets the interval between two ticks.
+	 */
+	void SetInterval(time_t interval);
+
 	/** Called when the timer ticks.
 	 * You should override this method with some useful code to
 	 * handle the tick event.
 	 */
-	virtual void Tick(time_t TIME) = 0;
+	virtual bool Tick(time_t TIME) = 0;
 
 	/** Returns true if this timer is set to repeat
 	 */
-	bool GetRepeat()
+	bool GetRepeat() const
 	{
 		return repeat;
 	}
@@ -90,7 +101,7 @@ class CoreExport Timer
 	/** Returns the interval (number of seconds between ticks)
 	 * of this timer object.
 	 */
-	long GetSecs()
+	unsigned int GetInterval() const
 	{
 		return secs;
 	}
@@ -98,12 +109,6 @@ class CoreExport Timer
 	/** Cancels the repeat state of a repeating timer.
 	 * If you call this method, then the next time your
 	 * timer ticks, it will be removed immediately after.
-	 * You should use this method call to remove a recurring
-	 * timer if you wish to do so within the timer's Tick
-	 * event, as calling TimerManager::DelTimer() from within
-	 * the Timer::Tick() method is dangerous and may
-	 * cause a segmentation fault. Calling CancelRepeat()
-	 * is safe in this case.
 	 */
 	void CancelRepeat()
 	{
@@ -111,6 +116,7 @@ class CoreExport Timer
 	}
 };
 
+typedef std::multimap<time_t, Timer*> TimerMap;
 
 /** This class manages sets of Timers, and triggers them at their defined times.
  * This will ensure timers are not missed, as well as removing timers that have
@@ -118,17 +124,11 @@ class CoreExport Timer
  */
 class CoreExport TimerManager
 {
- protected:
 	/** A list of all pending timers
 	 */
-	std::vector<Timer *> Timers;
+	TimerMap Timers;
 
  public:
-	/** Constructor
-	 */
-	TimerManager();
-	~TimerManager();
-
 	/** Tick all pending Timers
 	 * @param TIME the current system time
 	 */
@@ -139,12 +139,8 @@ class CoreExport TimerManager
 	 */
 	void AddTimer(Timer *T);
 
-	/** Delete an Timer
-	 * @param T an Timer derived class to delete
+	/** Remove a Timer
+	 * @param T an Timer derived class to remove
 	 */
 	void DelTimer(Timer* T);
-
-	/** Compares two timers
-	 */
-	static bool TimerComparison( Timer *one,  Timer*two);
 };
diff --git a/src/dns.cpp b/src/dns.cpp
index 461722b29..b9c605e78 100644
--- a/src/dns.cpp
+++ b/src/dns.cpp
@@ -123,9 +123,10 @@ class CacheTimer : public Timer
 	CacheTimer(DNS* thisdns)
 		: Timer(3600, ServerInstance->Time(), true), dns(thisdns) { }
 
-	virtual void Tick(time_t)
+	virtual bool Tick(time_t)
 	{
 		dns->PruneCache();
+		return true;
 	}
 };
 
@@ -143,7 +144,7 @@ class RequestTimeout : public Timer
 			Tick(0);
 	}
 
-	void Tick(time_t)
+	bool Tick(time_t)
 	{
 		if (ServerInstance->Res->requests[watchid] == watch)
 		{
@@ -157,6 +158,7 @@ class RequestTimeout : public Timer
 			ServerInstance->Res->requests[watchid] = NULL;
 			delete watch;
 		}
+		return false;
 	}
 };
 
diff --git a/src/inspsocket.cpp b/src/inspsocket.cpp
index fdd002ad6..5f2da2341 100644
--- a/src/inspsocket.cpp
+++ b/src/inspsocket.cpp
@@ -431,12 +431,12 @@ void StreamSocket::WriteData(const std::string &data)
 	ServerInstance->SE->ChangeEventMask(this, FD_ADD_TRIAL_WRITE);
 }
 
-void SocketTimeout::Tick(time_t)
+bool SocketTimeout::Tick(time_t)
 {
 	ServerInstance->Logs->Log("SOCKET", LOG_DEBUG,"SocketTimeout::Tick");
 
 	if (ServerInstance->SE->GetRef(this->sfd) != this->sock)
-		return;
+		return false;
 
 	if (this->sock->state == I_CONNECTING)
 	{
@@ -452,6 +452,7 @@ void SocketTimeout::Tick(time_t)
 	}
 
 	this->sock->Timeout = NULL;
+	return false;
 }
 
 void BufferedSocket::OnConnected() { }
@@ -476,8 +477,8 @@ BufferedSocket::~BufferedSocket()
 	this->Close();
 	if (Timeout)
 	{
-		ServerInstance->Timers->DelTimer(Timeout);
-		Timeout = NULL;
+		// The timer is removed from the TimerManager in Timer::~Timer()
+		delete Timeout;
 	}
 }
 
diff --git a/src/modules/extra/m_pgsql.cpp b/src/modules/extra/m_pgsql.cpp
index 6d2e0c88a..4690f9851 100644
--- a/src/modules/extra/m_pgsql.cpp
+++ b/src/modules/extra/m_pgsql.cpp
@@ -62,7 +62,7 @@ class ReconnectTimer : public Timer
 	ReconnectTimer(ModulePgSQL* m) : Timer(5, ServerInstance->Time(), false), mod(m)
 	{
 	}
-	virtual void Tick(time_t TIME);
+	virtual bool Tick(time_t TIME);
 };
 
 struct QueueItem
@@ -504,6 +504,11 @@ class ModulePgSQL : public Module
 	ConnMap connections;
 	ReconnectTimer* retimer;
 
+	ModulePgSQL()
+		: retimer(NULL)
+	{
+	}
+
 	void init()
 	{
 		ReadConf();
@@ -514,8 +519,7 @@ class ModulePgSQL : public Module
 
 	virtual ~ModulePgSQL()
 	{
-		if (retimer)
-			ServerInstance->Timers->DelTimer(retimer);
+		delete retimer;
 		ClearAllConnections();
 	}
 
@@ -594,10 +598,11 @@ class ModulePgSQL : public Module
 	}
 };
 
-void ReconnectTimer::Tick(time_t time)
+bool ReconnectTimer::Tick(time_t time)
 {
 	mod->retimer = NULL;
 	mod->ReadConf();
+	return false;
 }
 
 void SQLConn::DelayReconnect()
diff --git a/src/modules/m_spanningtree/cachetimer.cpp b/src/modules/m_spanningtree/cachetimer.cpp
index 6703e84b1..4fdc7056f 100644
--- a/src/modules/m_spanningtree/cachetimer.cpp
+++ b/src/modules/m_spanningtree/cachetimer.cpp
@@ -24,12 +24,13 @@
 
 /* $ModDep: m_spanningtree/cachetimer.h m_spanningtree/utils.h */
 
-CacheRefreshTimer::CacheRefreshTimer(SpanningTreeUtilities *Util) : Timer(3600, ServerInstance->Time(), true), Utils(Util)
+CacheRefreshTimer::CacheRefreshTimer(SpanningTreeUtilities* Util) : Timer(3600, ServerInstance->Time(), true), Utils(Util)
 {
 }
 
-void CacheRefreshTimer::Tick(time_t TIME)
+bool CacheRefreshTimer::Tick(time_t TIME)
 {
 	Utils->RefreshIPCache();
+	return true;
 }
 
diff --git a/src/modules/m_spanningtree/cachetimer.h b/src/modules/m_spanningtree/cachetimer.h
index acfb7434c..5d2278cd6 100644
--- a/src/modules/m_spanningtree/cachetimer.h
+++ b/src/modules/m_spanningtree/cachetimer.h
@@ -31,5 +31,5 @@ class CacheRefreshTimer : public Timer
 	SpanningTreeUtilities *Utils;
  public:
 	CacheRefreshTimer(SpanningTreeUtilities* Util);
-	virtual void Tick(time_t TIME);
+	virtual bool Tick(time_t TIME);
 };
diff --git a/src/modules/m_spanningtree/main.cpp b/src/modules/m_spanningtree/main.cpp
index bd49a3075..029f5e888 100644
--- a/src/modules/m_spanningtree/main.cpp
+++ b/src/modules/m_spanningtree/main.cpp
@@ -70,7 +70,7 @@ void ModuleSpanningTree::init()
 	ServerInstance->Modules->AddService(commands->fhost);
 	ServerInstance->Modules->AddService(commands->fident);
 	ServerInstance->Modules->AddService(commands->fname);
-	RefreshTimer = new CacheRefreshTimer(Utils);
+	RefreshTimer = new CacheRefreshTimer(this, Utils);
 	ServerInstance->Timers->AddTimer(RefreshTimer);
 
 	Implementation eventlist[] =
@@ -907,6 +907,7 @@ CullResult ModuleSpanningTree::cull()
 {
 	Utils->cull();
 	ServerInstance->Timers->DelTimer(RefreshTimer);
+	delete RefreshTimer;
 	return this->Module::cull();
 }
 
diff --git a/src/timer.cpp b/src/timer.cpp
index a1ee0b488..f098a5e6b 100644
--- a/src/timer.cpp
+++ b/src/timer.cpp
@@ -25,55 +25,54 @@
 #include "inspircd.h"
 #include "timer.h"
 
-TimerManager::TimerManager()
+void Timer::SetInterval(time_t newinterval)
 {
+	ServerInstance->Timers->DelTimer(this);
+	secs = newinterval;
+	SetTrigger(ServerInstance->Time() + newinterval);
+	ServerInstance->Timers->AddTimer(this);
 }
 
-TimerManager::~TimerManager()
+Timer::~Timer()
 {
-	for(std::vector<Timer *>::iterator i = Timers.begin(); i != Timers.end(); i++)
-		delete *i;
+	ServerInstance->Timers->DelTimer(this);
 }
 
 void TimerManager::TickTimers(time_t TIME)
 {
-	while ((Timers.size()) && (TIME > (*Timers.begin())->GetTimer()))
+	for (TimerMap::iterator i = Timers.begin(); i != Timers.end(); )
 	{
-		std::vector<Timer *>::iterator i = Timers.begin();
-		Timer *t = (*i);
+		Timer* t = i->second;
+		if (t->GetTrigger() > TIME)
+			break;
 
-		// Probable fix: move vector manipulation to *before* we modify the vector.
-		Timers.erase(i);
+		Timers.erase(i++);
 
-		t->Tick(TIME);
-		if (t->GetRepeat())
+		if (!t->Tick(TIME))
+			delete t;
+		else if (t->GetRepeat())
 		{
-			t->SetTimer(TIME + t->GetSecs());
+			t->SetTrigger(TIME + t->GetInterval());
 			AddTimer(t);
 		}
-		else
-			delete t;
 	}
 }
 
-void TimerManager::DelTimer(Timer* T)
+void TimerManager::DelTimer(Timer* t)
 {
-	std::vector<Timer *>::iterator i = std::find(Timers.begin(), Timers.end(), T);
+	std::pair<TimerMap::iterator, TimerMap::iterator> itpair = Timers.equal_range(t->GetTrigger());
 
-	if (i != Timers.end())
+	for (TimerMap::iterator i = itpair.first; i != itpair.second; ++i)
 	{
-		delete (*i);
-		Timers.erase(i);
+		if (i->second == t)
+		{
+			Timers.erase(i);
+			break;
+		}
 	}
 }
 
-void TimerManager::AddTimer(Timer* T)
-{
-	Timers.push_back(T);
-	sort(Timers.begin(), Timers.end(), TimerManager::TimerComparison);
-}
-
-bool TimerManager::TimerComparison( Timer *one, Timer *two)
+void TimerManager::AddTimer(Timer* t)
 {
-	return (one->GetTimer()) < (two->GetTimer());
+	Timers.insert(std::make_pair(t->GetTrigger(), t));
 }