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.

• Oebele: About std::promise::set_exception: how do I do that in a thread-safe manner? Because the caller thread can call set_value. This sounds like shared resources and thus a data race. Suppose stop is called, but set_exception is called from the ticker thread before the caller thread calls set_value, then that would cause an exception.
• Edward: You would either set the exception or set the value but never both. Allowing the caller to set the value is a design error in my view.
• Oebele: Huh... that was the whole point of your suggestion in the previous question.
• Edward: You misunderstand the difference then. In the design I showed, it was a shared_future that was owned and set exclusively outside the Ticker class. In your design, you have a shared_future (which really could simply be a future) that is owned by the class instance. The design error is in allowing it to be set by both the class itself and outside the class through a member function. In the design I showed, one could share a single shared_future among many Ticker instances; in yours each Ticker has its own.
• Oebele: Yes, but in your design you still call set_value in the thread that creates the other threads, which would still conflict with calling set_exception from the ticker threads. By the way, I don't see how calling set_exception when something goes wrong in the ticker thread solves anything, as there is no logical place to check whether it is set at all.

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.

• Oebele: Personally I find code hard to read when it is full with std::: I perceive that as a lot of distracting clutter. Of course I would never put any using statement in a header file. I realize that even in a .cpp file it can go wrong, especially now that I import two namespaces (though my types all start with a capital letter). Perhaps I should only 'use' specific things from a namespace that clutter my code too much? Perhaps I should just get used to it as well.
• Oebele: Probably you are right about the generalization, but I used to do that too much (taking too much time). Perhaps I am going overboard with preventing that now!