Refactored `timer` by removing static functions and `timer::node` and adding a `detach` instance function; `timer::manager` is now `impl::timer_manager`

src/common/CMakeLists.txt

@@ -3,6 +3,6 @@ cmake_minimum_required(VERSION 3.8)
 # Setup source files
 include_directories(../include)
 set(SOURCE_FILES
-        timer.cpp)
+        timer.cpp timer_manager.cpp)
 
 add_library(jessilib ${SOURCE_FILES})

src/common/timer.cpp

@@ -18,188 +18,130 @@
 
 #include "timer.hpp"
 #include <iostream>
+#include <cassert>
+#include "impl/timer_manager.hpp"
 
 namespace jessilib {
 
-/** timer::manager */
+/** helper */
-
+timer::function_t callback_with_iterations(timer::iterations_t iterations, timer::function_t callback) {
-timer::manager timer::s_manager;
+	assert(iterations > 0);
-
-timer::manager::~manager() {
-	m_thread_active = false;
-	m_cvar.notify_one();
-	m_thread.join();
-}
 
-void timer::manager::loop() {
+	return [iterations, callback](timer& timer) mutable {
-	// loop
+		callback(timer);
-	while (m_thread_active) {
-		std::unique_lock<std::mutex> lock(m_mutex);
-		auto itr{ m_timers.begin() };
-		if (itr != m_timers.end()) {
-			// Wait until the next timer is ready to fire
-			if (m_cvar.wait_until(lock, (*itr)->next()) == std::cv_status::timeout && itr != m_timers.end()) {
-				// Set active
-				std::shared_ptr<timer::node> target = *itr;
-				m_active = target.get();
-
-				// Execute timer
-				target->m_callback(*target);
-				itr = m_timers.begin();
-				if (itr != m_timers.end() && *itr == target) {
-					// Timer still exists; pop
-					m_timers.erase(itr);
-
-					// reset timings
-					target->m_next = target->calc_next();
-
-					// push
-					m_timers.insert(std::move(target));
-				}
 
-				// Reset active
+		if (--iterations == 0) {
-				m_active = nullptr;
+			timer.cancel();
-			}
-		}
-		else {
-			// Wait until a timer is added
-			m_cvar.wait(lock);
-		}
 		}
+	};
 }
 
-/** timer::node */
+/** timer */
-timer::node::node(duration_t period, callback_t callback)
-	: m_period{ period },
-	m_next{ calc_next() },
-	m_callback{ callback } {
-	// Empty ctor body
-}
-
-timer::time_point_t timer::node::next() const {
-	return m_next;
-}
 
-timer::duration_t timer::node::period() const {
+timer::timer()
-	return m_period;
+	: m_callback{ nullptr },
+	m_self{ impl::timer_manager::instance().m_detached_timers.end() } {
+	// Empty ctor body
 }
 
-timer::callback_t timer::node::function() const {
+timer::timer(timer&& in_timer)
-	return m_callback;
+	: m_period{ in_timer.m_period },
-}
+	m_next{ in_timer.m_next },
+	m_callback{ std::move(in_timer.m_callback) },
+	m_self{ impl::timer_manager::instance().m_detached_timers.end() } {
 
-void timer::node::cancel() {
+	// Cancel in_timer
-	// Race condition: cancel() being called externally while active
+	in_timer.m_callback = nullptr;
-	if (s_manager.m_active == this && s_manager.m_thread.get_id() == std::this_thread::get_id()) {
-		s_manager.m_timers.erase(shared_from_this());
-		return;
-	}
 
+	// Replace active timer
+	impl::timer_manager& manager = impl::timer_manager::instance();
 	{
-		std::lock_guard<std::mutex> lock(s_manager.m_mutex);
+		std::lock_guard<std::mutex> lock(manager.m_mutex);
-		s_manager.m_timers.erase(shared_from_this());
+		manager.m_active_timers.erase(&in_timer); // remove in_timer
-	}
+		manager.m_active_timers.insert(this); // insert this
-	s_manager.m_cvar.notify_one();
-}
-
-// Internals
-
-timer::time_point_t timer::node::calc_next() {
-	return std::chrono::steady_clock::now() + m_period;
-}
-
-/** timer */
-
-// Static functions
-void timer::set(duration_t period, callback_t callback) {
-	// Emplace timer
-	{
-		std::lock_guard<std::mutex> lock(s_manager.m_mutex);
-		s_manager.m_timers.emplace(new node(period, callback));
 	}
 
 	// Notify
-	s_manager.m_cvar.notify_one();
+	manager.m_cvar.notify_one();
-}
-
-void timer::set(duration_t period, size_t iterations, callback_t callback) {
-	// Expand callback and forward to set
-	set(period, callback_with_iterations(iterations, callback));
 }
 
-// Constructors
+timer::timer(duration_t in_period, function_t in_callback)
-timer::timer(duration_t period, callback_t callback) {
+	: m_period{ in_period },
-	// Sanity check
+	m_next{ calc_next() },
-	assert(callback != nullptr);
+	m_callback{ in_callback },
-
+	m_self{ impl::timer_manager::instance().m_detached_timers.end() } {
-	// Initialize timer
+	// Assertion checks
+	assert(m_callback != nullptr);
+	assert(m_period.count() != 0);
+
+	// Add timer
+	impl::timer_manager& manager = impl::timer_manager::instance();
 	{
-		std::lock_guard<std::mutex> lock(s_manager.m_mutex);
+		std::lock_guard<std::mutex> lock(manager.m_mutex);
-		m_timer = *s_manager.m_timers.emplace(new node(period, callback));
+		manager.m_active_timers.insert(this);
 	}
 
 	// Notify
-	s_manager.m_cvar.notify_one();
+	manager.m_cvar.notify_one();
 }
 
-timer::timer(duration_t period, size_t iterations, callback_t callback)
+timer::timer(duration_t in_period, iterations_t in_iterations, function_t in_callback)
-	: timer{ period, callback_with_iterations(iterations, callback) } {
+	: timer{ in_period, callback_with_iterations(in_iterations, in_callback) } {
 	// Empty ctor body
 }
 
-// Destructor
 timer::~timer() {
-	if (auto my_timer = m_timer.lock()) {
+	// If it's null, then it was either never added (default constructed) or already removed (as part of move)
-		my_timer->cancel();
+	if (!null()) {
+		cancel();
 	}
 }
 
-// Functions
 timer::time_point_t timer::next() const {
-	if (auto my_timer = m_timer.lock()) {
+	return m_next;
-		my_timer->next();
-	}
-
-	return {};
 }
 
-
 timer::duration_t timer::period() const {
-	if (auto my_timer = m_timer.lock()) {
+	return m_period;
-		return my_timer->period();
+}
-	}
 
-	return duration_t{ 0 };
+timer::function_t timer::function() const {
+	return m_callback;
 }
 
-timer::callback_t timer::function() const {
+bool timer::null() const {
-	if (auto my_timer = m_timer.lock()) {
+	return m_callback == nullptr;
-		return my_timer->function();
+}
-	}
+
+void timer::detach() {
+	impl::timer_manager& manager = impl::timer_manager::instance();
 
-	return nullptr;
+	assert(!manager.is_current(*this)); // you cannot detach a timer from within itself, because that would destroy the callback you're currently executing
+	assert(!manager.is_detached(*this)); // you cannot detach a timer that is already detached
+	std::lock_guard<std::mutex> lock(manager.m_detached_timers_mutex);
+	manager.m_detached_timers.emplace_back(std::move(*this));
+	--manager.m_detached_timers.back().m_self; // Is this a race condition?
 }
 
 void timer::cancel() {
-	if (auto my_timer = m_timer.lock()) {
+	impl::timer_manager& manager = impl::timer_manager::instance();
-		my_timer->cancel();
+
+	if (manager.is_current(*this)) {
+		manager.m_active_timers.erase(this);
+		return;
 	}
-}
 
-bool timer::active() const {
+	{
-	return m_timer.lock() != nullptr;
+		std::lock_guard<std::mutex> lock(manager.m_mutex);
+		manager.m_active_timers.erase(this);
+	}
+	manager.m_cvar.notify_one();
 }
 
-timer::callback_t timer::callback_with_iterations(size_t iterations, callback_t callback) {
+// Internals
-	assert(iterations > 0);
-
-	return [iterations, callback](node& timer) mutable {
-		callback(timer);
 
-		if (--iterations == 0) {
+timer::time_point_t timer::calc_next() {
-			timer.cancel();
+	return std::chrono::steady_clock::now() + m_period;
-		}
-	};
 }
 
 } // namespace jessilib

src/common/timer_manager.cpp

@@ -0,0 +1,93 @@
+/**
+ * Copyright (C) 2017 Jessica James.
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
+ * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
+ * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
+ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ *
+ * Written by Jessica James <jessica.aj@outlook.com>
+ */
+
+#include "impl/timer_manager.hpp"
+
+namespace jessilib {
+namespace impl {
+
+/** timer_manager */
+
+timer_manager& timer_manager::instance() {
+	static timer_manager s_manager;
+	return s_manager;
+}
+
+timer_manager::~timer_manager() {
+	m_thread_active = false;
+	m_cvar.notify_one();
+	m_thread.join();
+}
+
+void timer_manager::loop() {
+	// loop
+	while (m_thread_active) {
+		std::unique_lock<std::mutex> lock(m_mutex);
+		auto itr{m_active_timers.begin()};
+		if (itr != m_active_timers.end()) {
+			// Wait until the next timer is ready to fire
+			if (m_cvar.wait_until(lock, (*itr)->next()) == std::cv_status::timeout && itr != m_active_timers.end()) {
+				// Set active
+				timer* target = *itr;
+				m_current_timer = target;
+				bool detached = is_detached(*target);
+
+				// Execute timer
+				target->m_callback(*target);
+
+				// Check if timer still exists
+				itr = m_active_timers.begin();
+				if (itr != m_active_timers.end() && *itr == target) {
+					// Timer still exists; extract
+					m_active_timers.erase(itr);
+
+					// reset timings
+					target->m_next = target->calc_next();
+
+					// push
+					m_active_timers.insert(target);
+				}
+				else if (detached) {
+					// Timer has been canceled; remove it from the detached timers list
+					std::lock_guard<std::mutex> lock(m_detached_timers_mutex);
+					m_detached_timers.erase(target->m_self);
+				}
+
+				// Reset active
+				m_current_timer = nullptr;
+			}
+			// else // m_active_timers changed; itr may be invalid; itr may not be the next timer
+		}
+		else {
+			// Wait until a timer is added
+			m_cvar.wait(lock);
+		}
+	}
+}
+
+bool timer_manager::is_current(timer& in_timer) {
+	return m_current_timer == &in_timer && m_thread.get_id() == std::this_thread::get_id();
+}
+
+bool timer_manager::is_detached(timer& in_timer) {
+	std::lock_guard<std::mutex> lock(m_detached_timers_mutex);
+	return in_timer.m_self != m_detached_timers.end();
+}
+
+} // namespace impl
+} // namespace jessilib

src/include/impl/timer_manager.hpp

@@ -0,0 +1,70 @@
+/**
+ * Copyright (C) 2017 Jessica James.
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
+ * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
+ * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
+ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ *
+ * Written by Jessica James <jessica.aj@outlook.com>
+ */
+
+#pragma once
+
+#include <list>
+#include <set>
+#include <mutex>
+#include <condition_variable>
+#include <atomic>
+#include <thread>
+#include "../timer.hpp"
+
+namespace jessilib {
+namespace impl {
+
+/** timer::manager */
+
+class timer_manager {
+    // Singleton accessor
+    static timer_manager& instance();
+
+    // Destructor
+    ~timer_manager();
+
+    struct timer_sort {
+        bool operator()(const timer* lhs, const timer* rhs) const {
+            return lhs->next() < rhs->next();
+        }
+    };
+
+    // Loop
+    void loop();
+
+	// helpers
+	bool is_current(timer& in_timer);
+	bool is_detached(timer& in_timer);
+
+    // Members
+    std::list<timer> m_detached_timers;
+    std::multiset<timer*, timer_sort> m_active_timers;
+    std::mutex m_mutex, m_detached_timers_mutex;
+    std::condition_variable m_cvar;
+    std::atomic<timer*> m_current_timer{nullptr};
+    std::atomic<bool> m_thread_active{true};
+    std::thread m_thread{[this]() {
+        loop();
+    }};
+
+    // Friends
+    friend timer;
+}; // class timer_manager
+
+} // namespace impl
+} // namespace jessilib

src/include/timer.hpp

@@ -18,122 +18,74 @@
 
 #pragma once
 
-#include <cassert>
-#include <mutex>
 #include <chrono>
-#include <set>
+#include <list>
-#include <condition_variable>
-#include <atomic>
 #include <memory>
-
+#include <functional>
-// TODO: add 'timer* m_parent' to timer::node; rename timer::node to timer::internals?
 
 namespace jessilib {
 
+/** forward delcarations */
+namespace impl {
+	class timer_manager;
+} // namespace impl
+
 /** timer */
 
 class timer {
 public:
-	class node;
-
 	// Types
-	using callback_t = std::function<void(node&)>; /** Function type called by the timer */
+	using function_t = std::function<void(timer&)>; /** Function type called by the timer */
 	using time_point_t = std::chrono::steady_clock::time_point; /** Type representing the point in time at which a timer will be called */
 	using duration_t = std::chrono::steady_clock::duration;
+	using iterations_t = size_t;
 
-	// Static functions
+	enum class state {
-	static void set(duration_t period, callback_t callback);
+		active, // this timer is still active
-	static void set(duration_t period, size_t iterations, callback_t callback);
+		processing, // this timer is processing right now
+		null // this timer is inactive
+	};
 
 	// Constructors
-	timer(duration_t period, callback_t callback);
+	timer();
-	timer(duration_t period, size_t iterations, callback_t callback);
+	timer(timer&& in_timer);
+	timer(duration_t in_period, function_t in_callback);
+	timer(duration_t in_period, iterations_t in_iterations, function_t in_callback);
 
-	// Explicitly deleted constructors
+	// Move operator
-	timer() = delete;
+	timer& operator=(timer&& in_timer);
+
+	// Explicitly deleted copy constructor and assignment operator
 	timer(const timer&) = delete;
-	timer(timer&& in_timer) = delete;
+	timer& operator=(const timer&) = delete;
 
 	// Destructor
 	~timer();
 
-	// Functions
+	// Accessors
 	time_point_t next() const;
 	duration_t period() const;
-	callback_t function() const;
+	function_t function() const;
-	void cancel();
+	bool null() const;
-	bool active() const;
 
-private:
+	// Mutators
-	// Weak pointer to referenced timer
+	void detach();
-	std::weak_ptr<node> m_timer;
-
-	// Timer manager
-	class manager;
-	static manager s_manager;
-
-	// Helper
-	static callback_t callback_with_iterations(size_t iterations, callback_t callback);
-}; // timer
-
-/** timer::node */
-
-class timer::node : public std::enable_shared_from_this<node> {
-public:
-	time_point_t next() const;
-	duration_t period() const;
-	callback_t function() const;
 	void cancel();
 
 private:
-	// Internal constructor
+	// possible states: null, active, processing, cancelled (still processing)
-	node(duration_t period, callback_t callback);
-
-	// Explicitly deleted constructors
-	node() = delete;
-	node(const node&) = delete;
-	node(node&& in_timer) = delete;
 
 	// Internal helpers
 	time_point_t calc_next();
 
 	// Members
 	duration_t m_period;
-	callback_t m_callback;
+	function_t m_callback;
 	time_point_t m_next;
+	std::list<timer>::iterator m_self;
 
 	// Friends
-	friend class timer;
+	friend impl::timer_manager;
-}; // class timer::node
+}; // class timer
-
-
-/** timer::manager */
-
-class timer::manager {
-	// Destructor
-	~manager();
-
-	struct timer_sort {
-		bool operator()(const std::shared_ptr<node>& lhs, const std::shared_ptr<node>& rhs) const {
-			return lhs->next() < rhs->next();
-		}
-	};
-
-	// Loop
-	void loop();
-
-	// Members
-	std::multiset<std::shared_ptr<node>, timer_sort> m_timers;
-	std::mutex m_mutex;
-	std::condition_variable m_cvar;
-	std::atomic<node*> m_active{ nullptr };
-	std::atomic<bool> m_thread_active{ true };
-	std::thread m_thread{ [this]() {
-		loop();
-	}};
-
-	// Friends
-	friend class timer;
-}; // class timer::manager
 
-}; // namespace jessilib
+} // namespace jessilib

src/test/timer.cpp

@@ -27,84 +27,95 @@ constexpr size_t total_iterations{ 4 };
 constexpr std::chrono::steady_clock::duration period = 1ms;
 constexpr std::chrono::steady_clock::duration timeout = 1s;
 
-TEST(TimerTest, set) {
+TEST(TimerTest, scoped) {
 	size_t iterations{ 0 };
 	std::promise<void> promise;
 
-	timer::set(period, [&iterations, &promise](timer::node& in_timer) {
+	timer timer_obj{ period, [&iterations, &promise](timer& in_timer) {
 		if (++iterations == total_iterations) {
 			in_timer.cancel();
 			promise.set_value();
 		}
-	});
+	} };
 
 	EXPECT_EQ(promise.get_future().wait_for(timeout), std::future_status::ready);
 }
 
-TEST(TimerTest, scoped) {
+TEST(TimerTest, detached) {
 	size_t iterations{ 0 };
 	std::promise<void> promise;
 
-	timer timer{ period, [&iterations, &promise](timer::node& in_timer) {
+	{
+		timer timer_obj{ period, [&iterations, &promise](timer& in_timer) {
 			if (++iterations == total_iterations) {
+				EXPECT_FALSE(in_timer.null());
 				in_timer.cancel();
 				promise.set_value();
 			}
 		} };
 
+		EXPECT_FALSE(timer_obj.null());
+		timer_obj.detach();
+		EXPECT_TRUE(timer_obj.null());
+	}
+
 	EXPECT_EQ(promise.get_future().wait_for(timeout), std::future_status::ready);
 }
 
-TEST(TimerTest, setWithIterations) {
+TEST(TimerTest, scopedWithIterations) {
 	size_t iterations{ 0 };
 	std::promise<void> promise;
 
-	timer::set(period, total_iterations, [&iterations, &promise](timer::node& in_timer) {
+	timer timer_obj{ period, total_iterations, [&iterations, &promise](timer& in_timer) {
 		if (++iterations == total_iterations) {
 			promise.set_value();
 		}
-	});
+	} };
 
 	EXPECT_EQ(promise.get_future().wait_for(timeout), std::future_status::ready);
 }
 
-TEST(TimerTest, scopedWithIterations) {
+TEST(TimerTest, detachedWithIterations) {
 	size_t iterations{ 0 };
 	std::promise<void> promise;
 
-	timer timer{ period, total_iterations, [&iterations, &promise](timer::node& in_timer) {
+	{
+		timer{period, total_iterations, [&iterations, &promise](timer& in_timer) {
 			if (++iterations == total_iterations) {
 				promise.set_value();
 			}
-	} };
+		}}.detach();
+	}
 
 	EXPECT_EQ(promise.get_future().wait_for(timeout), std::future_status::ready);
 }
 
-TEST(TimerTest, setWithIterationsCancel) {
+TEST(TimerTest, scopedWithIterationsCancel) {
 	size_t iterations{ 0 };
 	std::promise<void> promise;
 
-	timer::set(period, total_iterations, [&iterations, &promise](timer::node& in_timer) {
+	timer timer_obj{ period, total_iterations, [&iterations, &promise](timer& in_timer) {
 		if (++iterations == total_iterations) {
 			in_timer.cancel();
 			promise.set_value();
 		}
-	});
+	} };
 
 	EXPECT_EQ(promise.get_future().wait_for(timeout), std::future_status::ready);
 }
 
-TEST(TimerTest, scopedWithIterationsCancel) {
+TEST(TimerTest, detachedWithIterationsCancel) {
 	size_t iterations{ 0 };
 	std::promise<void> promise;
 
-	timer timer{ period, total_iterations, [&iterations, &promise](timer::node& in_timer) {
+	{
+		timer{period, total_iterations, [&iterations, &promise](timer& in_timer) {
 			if (++iterations == total_iterations) {
 				in_timer.cancel();
 				promise.set_value();
 			}
-	} };
+		}}.detach();
+	}
 
 	EXPECT_EQ(promise.get_future().wait_for(timeout), std::future_status::ready);
 }