Creating a ticker thread - version 2

Question

In this question, I asked for feedback on a class that provided my program with periodical signals. I have rewritten that class based on the feedback from the accepted answer. As this again uses a complex library I've never worked with before, I'd really appreciate some feedback!

The class runs a function in a separate thread using std::async. Every interval seconds, this function sends a 'tick' to the other parts of my program using my ZeroMQ wrapper classes Client and Message. This is thread-safe, because all other behaviour in my program is also triggered via messages read by the same ZeroMQ socket that is reading these 'ticks'.

Some notes:

• In the accepted answer to the previous question, it was suggested to use chrono literals, but as I am restricted to gcc 4.8.5 and thus c++11, I can't do this, so I am using an int respresenting a number of seconds.
• I instantiate Client in the asynchronously called function so it cannot be used from another thread, but I should probably move the instantiation to the constructor as it uses a ZeroMQ context that is created in another thread.
• The accepted answer to the previous question included some generalizations. I left these out of my implementation, because they were unnecessary in this application and this kept the code simpler.
• async_future is not used, but I need to store it, otherwise async won't return.

And finally, the actual code:

Ticker.hpp

#ifndef TICKER_HPP
#define TICKER_HPP

#include <future>
#include "ipc/Client.hpp"
#include "ipc/Message.hpp"

namespace sdp {

    class Ticker {
    private:
        const std::chrono::seconds interval;
        std::promise<void> done_promise;
        std::shared_future<void> done_future;
        std::future<void> async_future;
    public:
        Ticker(unsigned int interval);
        Ticker(const Ticker& orig) = delete;
        virtual ~Ticker();
        void stop();
    private:
        void tickfunction();
    };
}

#endif /* TICKER_HPP */

Ticker.cpp

#include "Ticker.hpp"
#include <chrono>

using namespace std;
using namespace sdp;

Ticker::Ticker(unsigned int interval) 
: interval(interval), done_promise(), done_future(done_promise.get_future()) {
    async_future = async(launch::async, &Ticker::tickfunction, this);
}

Ticker::~Ticker() {
    stop();
}

void Ticker::stop() {
    done_promise.set_value();
}

void Ticker::tickfunction() {    
    Client socket("inproc://ticker");

    std::future_status status;
    do {
        status = done_future.wait_for(interval);
        if (status == std::future_status::timeout) {
            Message message;
            message.addEmptyPart();
            message.add("tick");
            socket.send(message);
        }
    } while (status != std::future_status::ready);
}

Answer 1

Here are some things that may help you further improve your code.

Clearly separate interface and implementation

The Ticker.hpp includes two headers, ipc/Client.hpp and ipc/Message.hpp which are not actually part of the interface. They're part of the implementation and so those #includes should be moved to the .cpp file instead.

Don't throw an exception from a destructor

A destructor really really shouldn't throw an exception, but yours might. Consider this simple main():

int main() {
    sdp::Ticker t(1);
    t.stop();
}

This will throw an error because stop attempts to set the done_promise value and then the destructor calls stop and attempts to set it again. That second attempt is guaranteed to throw an error.

Set an exception in the promise

It seems likely that your code for constructing and sending a message could throw an exception. If they do, the program will seem to silently fail. Consider using std::promise::set_exception to propagate the error out to the caller so that it could be handled.

In particular, the code for tickfunction might include something like this:

    try { 
        throw runtime_error("aaaah!");
    } catch (...) {
        valuePosted = true;
        throw;
    }

The calling function could then include something like this:

try {
    t.get();
} catch (std::exception &err) {
    std::cout << err.what() << '\n';
}

This allows you to catch an exception that might have been thrown within the spawned thread.

Consider making the class generic

One can easily imagine that doing something on a regular basis (i.e. every x seconds) might be useful beyond this singular usage. For that reason, it might be useful to think about either passing a void function into the constructor or creating a template that uses a Callable type for later invocation.

Answer 2

Looks much better than the previous one, congratulations!

One thing remaining is how you use your namespaces.

using namespace std; is considered bad practice. Short code is not a requirement in C++, clear code is preferred. Especially when using multiple namespaces and/or when there's a risk of the compiler getting confused (the same function name being used in both namespaces with different implementation, for example).

I'd reconsider adding the generalisations. You say you don't need them, but it makes your code more versatile and easier to use for next projects. If your next project does something similar but a little different without using generalized code, you get code duplication in your code base. Once your code base grows large enough, you'll see that's a problem.

I could place a final, small remark about your inconsistent blank lines. Use blank lines for visually structuring your code. This only works if your usage is consistent.