Simple multi-threaded time scheduler

Question

Here's a rather hastily made time scheduler to suit my requirements and even for my future projects. It is working as intended but I want to improve more . I am a self taught enthusiast, and rather weak as a language lawyer.

This scheduler should be able to schedule tasks which will be executed the task at the right instance. It should arrange the time-points in chronological(//TODO boundary condition) order.

typedef std::function<void(void)> func;
typedef std::chrono::system_clock::time_point time_point;
std::map<time_point, func> schedule;
int main()
{
 schedule.insert(make_pair(std::chrono::system_clock::now() + std::chrono::seconds(4s), []() { std::cout << " Slow first \n"; }));
 schedule.insert(make_pair(std::chrono::system_clock::now() + std::chrono::seconds(3s), []() { std::cout << " Medium second \n"; }));
 thread t([]()
 {
 while (!schedule.empty())
 {
 if (chrono::system_clock::now() >= (schedule.begin())->first)
 {
 (schedule.begin())->second();
 schedule.erase(schedule.begin());
 }
 this_thread::sleep_for(chrono::milliseconds(1ms));
 }
 });
 schedule.insert(make_pair(std::chrono::system_clock::now() + std::chrono::milliseconds(500ms), []() { std::cout << " Impatience third \n"; }));
 std::cout << " main is waiting for the thread to end already \n";
 t.join();
 return 0;
}

Answer 1

I notice a very bad (anti-)pattern that you're using. You're polling. "Wake up frequently to check if there's any work, if not go back to sleep". The thread demands to be woken up every 1ms or so. This will cause a big burden on the OS thread scheduler and hinder performance. It also prevents your CPU from going to sleep (low power mode) so the power usage and heat generated by your CPU will greatly increase.

What you should do instead is to have your thread stay asleep until the next time that an event is scheduled to occur. Make sure your sleep has a method of being interrupted in case a new event comes in. Look up "condition variables". (#include <condition_variable>)

Secondly, you're treating std::map as if it's thread-safe. It's not. If the main thread is modifying the map at the same instant that the scheduler thread is checking it, bad things can happen. Spooky bugs that occur intermittently and are impossible to recreate or trace. You need to use a thread-safe data structure (or control access using some kind of locking mechanism, which you'll need anyway for the condition variable) if you're going to share it between multiple threads.

Let me make it clear: don't expose the actual std::map to the event providers. All access must go through methods that enforce proper object locking (and wake up the thread if necessary).

If you want to be able to expand this, you really need to make it its own class with a specific interface -- then you can define the implementation however you wish. The operations could be: add an event, remove an event, scheduling on, scheduling off, list all events, enumerate events one by one, change time of an event, change action of an event, remove all events.

• ark1974: Nice feedback. +1 for the std::map thread-safe behavior. I was also planning to replace the polling with some lock and cv thing, waking up from sleep to update new time_point is fairly challenging .
• Snowbody: Thanks. There is plenty of sample code out there using C++11's condition_variable; I'm sure someone has used it for a scheduled task system. The scheduler thread waits using condition_variable::wait_until and when a new event comes in the controller calls condition_variable::notify_one to make the scheduler thread wake up early if necessary so it can adjust its wait time.