HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Portable periodic/one-shot timer implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
periodiconeshottimerportableimplementation

Problem

Note: I have posted a follow-up question for a significantly updated version of this code.

I have implemented a class that provides portable one-shot or periodic timers.

The API provides a way to schedule one or more timer callbacks to fire some number of milliseconds in the future, and optionally fire again every so many milliseconds.

The API returns an ID which can be used later to see if the timer still exists, or destroy it.

The implementation assigns increasing IDs to the timers, starting at one. The timer IDs use a 64-bit unsigned integer, to avoid dealing with wraparound.

The implementation stores the context of each timer Instance in an unordered_map called active, keyed by ID. Each instance has a next member, which is a time_point that indicates when it needs to fire next.

The implementation uses a multiset called queue to sort the timers by the next member, using a functor.

The queue multiset stores reference_wrapper objects that refer directly to the Instance objects. This allows the queue's comparator to refer directly to the instances.

A worker thread is created to service the timer queue. A mutex and condition_variable are used for synchronization. The condition variable is used to notify the worker thread when a timer is created or destroyed, and to request worker thread shutdown.

The worker thread uses wait to wait for notification when there are no timers, and uses wait_for to wait until earliest notification needs to fire, or until awakened.

The lock is released during the callback when a timer fires.

The destructor sets the done flag to true, notifies the condition variable, releases the lock, then joins with the worker to wait for it to exit.

EDIT: I realized that there was a race condition that occurs if a timer is destroyed while its callback is in progress. I solved that by having a running flag on each instance. The worker thread checks it when a callback returns to see if that Instance needs to b

Solution

I like your syntax, for example C++11 style such as std::reference_wrapper.

I like your algorithm, for example:

  • Sort the queue so that you only need to check the first one



  • Unlock before doing the timer callback (and re-lock afterwards)



Minor things about the style that made it harder to read:

  • Two private sections in Timer, containing a mix of type definitions and field definitions



  • Fields (instance data) defined at the top of the Timer class but at the end of the Instance class



  • Inline code in timer.h (users of the class might find it easier if lines of code were hidden away in timer.cpp).



The exists method is strange: what do you think users will use it for? It might return true but then have the timer go off immediately, i.e. it may return stale information.

I don't understand the use case for std::function&& handler and std::move(handler). It's probably cleverer than I am, maybe a comment or an example test case which uses it would help me.

Context

StackExchange Code Review Q#40473, answer score: 6

Revisions (0)

No revisions yet.