C++11 prototype app design multithreading issue

Question

I have designed a prototype app with the following classes:

Ticker class (discussed here) that has an assignable callback that will be executed every tick (specified by _tickInterval) on a separate thread.
SongPosition class that basically represents a position in a song.
BeatClock class that assigns a callback to _ticker which is supposed to update _songpos and here I have a problem...

Here is what Ticker class implementation looks like:

#include <cstdint>
#include <functional>
#include <chrono>
#include <thread>
#include <future>
#include <mutex>

class Ticker {
public:
/* TYPES: */
    // Tick interval type in nanoseconds
    typedef std::chrono::nanoseconds tick_interval_type;
    // OnTick callback type
    typedef std::function<void()> on_tick_type;

/* CONSTANTS: */
    // Default ticker interval equal to 120 BPM
    static const tick_interval_type kDefaultTickerInterval;

/* INIT: */
    // Constructs object
    explicit Ticker (tick_interval_type tickInterval) 
    : _onTick ()
    , _tickInterval (tickInterval)
    , _running (false) {}
    // Destructs object
    ~Ticker () {}

/* PROPERTIES: */
    // Sets tick interval to a new value
    void setTickInterval (tick_interval_type tickInterval) {
        _tickIntervalMutex.lock();
        _tickInterval = tickInterval;
        _tickIntervalMutex.unlock();
    }
    // Sets on-tick callback to a new value
    void setOnTick (on_tick_type onTick) { _onTick = onTick; }

/* PUBLIC METHODS: */
    // Starts the ticker
    void start () {
        if (_running) return;
        _running = true;
        std::thread run( &Ticker::loop, this );
        run.detach();
    }
    // Stops the ticker
    void stop () { _running = false; }

private:
/* FIELDS: */
    // OnTick callback, called every tick
    on_tick_type            _onTick;
    // Tick interval between every tick
    tick_interval_type      _tickInterval;
    // Ticker running flag
    volatile bool           _running;
    // Tick interval mutex for thread safety
    std::mutex              _tickIntervalMutex;

/* PRIVATE METHODS: */
    // Inner loop executed on a separate thread to produce ticks
    void loop () {
        tick_interval_type ti;
        while (_running) {
            std::thread run (_onTick);
            run.detach();

            _tickIntervalMutex.lock();
            ti = _tickInterval;
            _tickIntervalMutex.unlock();

            std::this_thread::sleep_for( ti );
        }
    }
};

// Default ticker interval equal to 120 BPM
const Ticker::tick_interval_type Ticker::kDefaultTickerInterval (625000000UL / 120UL);

Here is what SongPosition class implementation looks like:

#include <cstdint>

class SongPosition {
public:
/* INIT: */
    // Constructs song position instance: Ticks | Beats | Measures
    SongPosition (uint8_t ticks = 0u, uint8_t beats = 0u, uint16_t measures = 0u) {
        _ticks = ticks;
        _beats = beats;
        _measures = measures;
    }
    ~SongPosition () {}

/* PROPERTIES:: */
    // Counts from 0..95
    uint8_t ticks () const { return _ticks; }
    // Counts the quarter notes 0..3
    uint8_t beats () const { return _beats; }
    // Counts the measures 0..65535
    uint16_t measures () const { return _measures; }
    // Current meter location, sixteens-notes counter
    uint16_t meter () const { return (_beats << 2) | (_measures << 4); }

/* METHODS: */
    // Resets meter counters
    void reset () {
        _ticks = 0u;
        _beats = 0u;
        _measures = 0u;
    }
    // Resets ticks counter
    void resetTicks () { _ticks = 0u; }
    // Resets beats counter
    void resetBeats () { _beats = 0u; }
    // Increments ticks counter
    void incrementTicks () { ++_ticks; }
    // Increments beats counter
    void incrementBeats () { ++_beats; }
    // Increments measures counter
    void incrementMeasures () { ++_measures; }
    // Decrements measures counter
    void decrementMeasures () { --_measures; }

    bool operator== (const SongPosition& other) const {
        if (_ticks == other.ticks() 
            && _beats == other.beats() 
            && _measures == other.measures())   return true;
        else return false;
    }
    bool operator!= (const SongPosition& other) const {
        return !(*this == other);
    }

private:
/* FIELDS: */
    // Counts from 0..95
    uint8_t     _ticks;
    // Counts the quarter notes 0..3
    uint8_t     _beats;
    // Counts the measures 0..65535
    uint16_t    _measures;
};

inline bool operator<  (const SongPosition& lhs, const SongPosition& rhs) {
    if (lhs.measures() < rhs.measures()) return true;
    else if (lhs.measures() == rhs.measures()) {
        if (lhs.beats() < rhs.beats()) return true;
        else if (lhs.beats() == rhs.beats()) {
            if (lhs.ticks() < rhs.ticks()) return true;
            else return false;
        }
        else return false;
    }
    else return false;
}
inline bool operator>  (const SongPosition& lhs, const SongPosition& rhs) {
    return rhs < lhs;
}
inline bool operator<= (const SongPosition& lhs, const SongPosition& rhs) {
    return !(lhs > rhs);
}
inline bool operator>= (const SongPosition& lhs, const SongPosition& rhs) {
    return !(lhs < rhs);
}

And finally here is what BeatClock class implementation looks like (the problem one):

#include <cstdint>
#include "SongPosition.h"
#include "Ticker.h"

class BeatClock {
public:
/* INNER CLASSES: */
    // Inner state of beat clock
    enum class State : uint8_t {
        kStopped,
        kRunning,
        kPaused
    };

/* CONSTANTS: */
    // Clock pre-start delay to give a slave time to prepare (1ms)
    static const std::chrono::milliseconds kPreStartDelay;

/* CONSTRUCTORS/DISTRUCTORS: */
    // Constructs object
    BeatClock () 
    : _bpm (120UL)
    , _ticker (Ticker::kDefaultTickerInterval)
    , _songpos ()
    , _precount () {
        _ticker.setOnTick( [&] {
            _tickerMutex.lock();
            _songpos.incrementTicks();
            if (_songpos.ticks() == 96U) {      // next beat...
                _songpos.resetTicks();
                _songpos.incrementBeats();
                if (_songpos.beats() == 4U) {   // next measure...
                    _songpos.resetBeats();
                    _songpos.incrementMeasures();
                }
            }
            if (_precount == 0U) {
                _precount = 4U;
                printf( "Send - RealTimeClock: %2i | %1i | %05i", 
                    _songpos.ticks(), _songpos.beats(), _songpos.ticks() );
            }
            ++_precount;
            _tickerMutex.unlock();
        } );
        _state = State::kStopped;
    }
    // Destructs object
    ~BeatClock () {}

/* PROPERTIES: */
    // Gets current BPM value 
    uint64_t bpm () const { return _bpm; }
    // Sets current BPM to a new value
    void setBpm (uint64_t bpm) { 
        _bpm = bpm;
        Ticker::tick_interval_type span (625000000UL / _bpm);
        _ticker.setTickInterval( span );
    }

/* PUBLIC METHODS: */
    // Starts if currently stopped / Pauses if currently running / Resumes if currently paused
    void start () {
        switch (_state)
        {
        case State::kStopped:   // start...
            _songpos.reset();
            this->sendMeter();
            printf( "Send - RealTimeStart" );
            std::this_thread::sleep_for( kPreStartDelay );
            _state = State::kRunning;
            _ticker.start();
            break;
        case State::kRunning:   // pause...
            _ticker.stop();
            _state = State::kPaused;
            break;
        case State::kPaused:    // continue...
            printf( "Send - RealTimeContinue" );
            _state = State::kRunning;
            _ticker.start();
            break;
        }
    }
    // Stops if currently running or paused / Resets if currently stopped
    void stop () {
        switch (_state)
        {
        case State::kStopped:   // reset...
            _songpos.reset();
            this->sendMeter();
            printf( "Send - RealTimeStop" );
            break;
        case State::kRunning:
        case State::kPaused:    // stop...
            printf( "Send - RealTimeStop" );
            _ticker.stop();
            _precount = 0U;
            _state = State::kStopped;
            break;
        }
    }
    // Increments measure and resets sub-counters
    void forward () {
        if (_state == State::kRunning) _ticker.stop();
        _songpos.resetTicks();
        _songpos.resetBeats();
        _songpos.incrementMeasures();
        _precount = 0U;
        this->sendMeter();
        if (_state == State::kRunning) _ticker.start();
    }
    // Decrements measure and resets sub-counters
    void rewind () {
        if (_state == State::kRunning) _ticker.stop();
        _songpos.resetTicks();
        _songpos.resetBeats();
        if (_songpos.measures() > 0U) _songpos.decrementMeasures();
        _precount = 0U;
        this->sendMeter();
        if (_state == State::kRunning) _ticker.start();
    }

private:
/* FIELDS: */
    // Current BPM value
    uint64_t        _bpm;
    // Used for generating tick events.
    Ticker          _ticker;
    // Current song position
    SongPosition    _songpos;
    // Clock precount to scale from 96 to 24 ppqn (0..3)
    uint8_t         _precount;
    // Transport state  
    State           _state;
    // Ticker mutex for thread safety
    std::mutex      _tickerMutex;

/* PRIVATE METHODS: */
    // Sends Song-Position MIDI message
    void sendMeter () {
        printf( "Send - SongPosition: %i", _songpos.meter() );
    }
};

// Clock pre-start delay to give a slave time to prepare (1ms)
const std::chrono::milliseconds BeatClock::kPreStartDelay (1);

_onTick callback is not working as expected, updating _songpos randomly (because of data-races?) Can you please advice how to update _songpos in controllable way from a callback (which is running on a separate thread)?

Here is what I'm expecting to be logged (that is 24 ticks per beat and 4 beats per measure):

Send - SongPosition: 0
Send - RealTimeStart            // begin...
Send - RealTimeClock:  2 | 0 | 00000
Send - RealTimeClock:  5 | 0 | 00000
Send - RealTimeClock:  9 | 0 | 00000
Send - RealTimeClock: 13 | 0 | 00000
Send - RealTimeClock: 17 | 0 | 00000
Send - RealTimeClock: 21 | 0 | 00000
Send - RealTimeClock: 25 | 0 | 00000
Send - RealTimeClock: 29 | 0 | 00000
Send - RealTimeClock: 33 | 0 | 00000
Send - RealTimeClock: 37 | 0 | 00000
Send - RealTimeClock: 41 | 0 | 00000
Send - RealTimeClock: 45 | 0 | 00000
Send - RealTimeClock: 49 | 0 | 00000
Send - RealTimeClock: 53 | 0 | 00000
Send - RealTimeClock: 57 | 0 | 00000
Send - RealTimeClock: 61 | 0 | 00000
Send - RealTimeClock: 65 | 0 | 00000
Send - RealTimeClock: 69 | 0 | 00000
Send - RealTimeClock: 73 | 0 | 00000
Send - RealTimeClock: 77 | 0 | 00000
Send - RealTimeClock: 81 | 0 | 00000
Send - RealTimeClock: 85 | 0 | 00000
Send - RealTimeClock: 89 | 0 | 00000
Send - RealTimeClock: 93 | 0 | 00000
Send - RealTimeClock:  1 | 1 | 00000    // next beat..
Send - RealTimeClock:  5 | 1 | 00000
Send - RealTimeClock:  9 | 1 | 00000
Send - RealTimeClock: 13 | 1 | 00000
Send - RealTimeClock: 17 | 1 | 00000
Send - RealTimeClock: 21 | 1 | 00000
Send - RealTimeClock: 25 | 1 | 00000
Send - RealTimeClock: 29 | 1 | 00000
Send - RealTimeClock: 33 | 1 | 00000
Send - RealTimeClock: 37 | 1 | 00000
Send - RealTimeClock: 41 | 1 | 00000
Send - RealTimeClock: 45 | 1 | 00000
Send - RealTimeClock: 49 | 1 | 00000
Send - RealTimeClock: 53 | 1 | 00000
Send - RealTimeClock: 57 | 1 | 00000
Send - RealTimeClock: 61 | 1 | 00000
Send - RealTimeClock: 65 | 1 | 00000
Send - RealTimeClock: 69 | 1 | 00000
Send - RealTimeClock: 73 | 1 | 00000
Send - RealTimeClock: 77 | 1 | 00000
Send - RealTimeClock: 81 | 1 | 00000
Send - RealTimeClock: 85 | 1 | 00000
Send - RealTimeClock: 89 | 1 | 00000
Send - RealTimeClock: 93 | 1 | 00000
Send - RealTimeClock:  1 | 2 | 00000    // next beat..
Send - RealTimeClock:  5 | 2 | 00000
Send - RealTimeClock:  9 | 2 | 00000
Send - RealTimeClock: 13 | 2 | 00000
Send - RealTimeClock: 17 | 2 | 00000
Send - RealTimeClock: 21 | 2 | 00000
Send - RealTimeClock: 25 | 2 | 00000
Send - RealTimeClock: 29 | 2 | 00000
Send - RealTimeClock: 33 | 2 | 00000
Send - RealTimeClock: 37 | 2 | 00000
Send - RealTimeClock: 41 | 2 | 00000
Send - RealTimeClock: 45 | 2 | 00000
Send - RealTimeClock: 49 | 2 | 00000
Send - RealTimeClock: 53 | 2 | 00000
Send - RealTimeClock: 57 | 2 | 00000
Send - RealTimeClock: 61 | 2 | 00000
Send - RealTimeClock: 65 | 2 | 00000
Send - RealTimeClock: 69 | 2 | 00000
Send - RealTimeClock: 73 | 2 | 00000
Send - RealTimeClock: 77 | 2 | 00000
Send - RealTimeClock: 81 | 2 | 00000
Send - RealTimeClock: 85 | 2 | 00000
Send - RealTimeClock: 89 | 2 | 00000
Send - RealTimeClock: 93 | 2 | 00000
Send - RealTimeClock:  1 | 3 | 00000    // next beat..
Send - RealTimeClock:  5 | 3 | 00000
Send - RealTimeClock:  9 | 3 | 00000
Send - RealTimeClock: 13 | 3 | 00000
Send - RealTimeClock: 17 | 3 | 00000
Send - RealTimeClock: 21 | 3 | 00000
Send - RealTimeClock: 25 | 3 | 00000
Send - RealTimeClock: 29 | 3 | 00000
Send - RealTimeClock: 33 | 3 | 00000
Send - RealTimeClock: 37 | 3 | 00000
Send - RealTimeClock: 41 | 3 | 00000
Send - RealTimeClock: 45 | 3 | 00000
Send - RealTimeClock: 49 | 3 | 00000
Send - RealTimeClock: 53 | 3 | 00000
Send - RealTimeClock: 57 | 3 | 00000
Send - RealTimeClock: 61 | 3 | 00000
Send - RealTimeClock: 65 | 3 | 00000
Send - RealTimeClock: 69 | 3 | 00000
Send - RealTimeClock: 73 | 3 | 00000
Send - RealTimeClock: 77 | 3 | 00000
Send - RealTimeClock: 81 | 3 | 00000
Send - RealTimeClock: 85 | 3 | 00000
Send - RealTimeClock: 89 | 3 | 00000
Send - RealTimeClock: 93 | 3 | 00000
Send - RealTimeClock:  1 | 0 | 00001    // next measure and once again...
Send - RealTimeStop         // enough. :)

Thank you very much!

I cannot find what data type `_songpos` is. Consider making that an `std::atomic`. You can find data races by checking if any variable can potentially be written by 2 threads or written and read by 2 threads concurrently. I am too lazy to do that for you. — nwp, Jul 17 '14 at 10:35
Please turn this into a runnable program. It should not be hard, I've done it in like 5 minutes, and my version exhibits no random behaviour you describe. — n. m. could be an AI, Jul 17 '14 at 10:38
@nwp `_songpos` is instance of `SongPosition` class I described. I suppose that data-race it's exactly what is happening with this variable. Thanks for advice, I'll try to wrap it in `std::atomic` — mikeg, Jul 17 '14 at 10:54
@n.m. it is a runnable program (on windows). That's how I know that there is a problem... You ran it on windows? Can you show me the log output you get? — mikeg, Jul 17 '14 at 10:56
I didn't run in on Windows. I don't have cocos2d, have no idea what it is and why it is needed here, and no desire to install it. Remove reference to cocos2d, replace `log()` with `printf()` and add main(), so that it is pure standard c++11. Also add desired output. — n. m. could be an AI, Jul 17 '14 at 11:28
@n.m. sorry, that is my fault, cocos2d is unnecessary in the implementation. I have updated it. — mikeg, Jul 17 '14 at 11:55

score 0 · Accepted Answer · answered Jul 17 '14 at 11:23

I have tested your code and it works fine and I haven't noticed any race condition. Consider this string log( "Send - RealTimeClock: %2i | %1i | %05i", _songpos.ticks(), _songpos.beats(), _songpos.ticks() ); It writes ticks | beats | ticks about every one second. Perhaps, you mean measures instead one of ticks? Proper output looks like the following (the last value is a measure)

Send - SongPosition: 0,
Send - RealTimeStart
Send - RealTimeClock: 1 | 0 | 0
Send - RealTimeClock: 61 | 2 | 0
Send - RealTimeClock: 25 | 1 | 1
Send - RealTimeClock: 85 | 3 | 1
Send - RealTimeClock: 49 | 2 | 2
Send - RealTimeClock: 13 | 1 | 3

And it is ok. You can check if it is race condition just changing thread start to simple function invocation.

And one remark. _tickerMutex appears to be unsafe in your case. If you use mutex to protect some data from simultaneous change then use it in every case when the data is changed. So _songpos is protected in your ticker lambda function but is unsafe in all functions like start, stop etc.

Hvarnah, there is a mistake in printing, it should be `ticks | beats | measures` of course, thank you. Now it's make more sense but it's still not what I expected to be printed out. I'm expecting to see 24 ticks per beat and 4 beats per measure but the printing is partial or just not printed entirely... really don't know... Please look at the question, I'll add what I expect to see there. — mikeg, Jul 17 '14 at 12:45
@mikeg I see, you want to output values every 4 ticks but count from 4 to 0. I think it is supposed to be from 0 to 4 `if (_precount == 0U) { _precount = 4U; .. ++_precount;` — Hvarnah, Jul 17 '14 at 14:55
Hvarnah, thanks, of course!!! this should be `--_precount` instead! Well, I need to take a break some time and let the eyes rest... Thank you very much! — mikeg, Jul 17 '14 at 16:49

C++11 prototype app design multithreading issue

1 Answers1

Linked