10
\$\begingroup\$

The previous code (as displayed in Timer utilizing std::future) is now:

#include <chrono>
#include <functional>
#include <future>
#include <iostream>
#include <list>
#include <mutex>
#include <thread>

// PeriodicFunction
// =============================================================================

namespace Detail {

/// A functor to invoke a function periodically.
template <typename Value, typename Derived>
class PeriodicFunction
{
    // Types
    // =====

    public:
    typedef std::chrono::milliseconds period_type;
    typedef Value value_type;

    protected:
    typedef std::function<Value ()> function_type;
    typedef std::future<Value> future_type;

    // Construction
    // ============

    protected:
    /// Initialize with a function where the arguments are bounded to that function.
    /// \SEE std::bind
    template <typename Callable, typename... Arguments>
    PeriodicFunction(Callable&& callable, Arguments&&... arguments)
    :   m_function(std::bind(
            std::move(callable),
            std::forward<Arguments>(arguments)...)),
        m_period(period_type::zero())
    {}

    public:
    PeriodicFunction(const PeriodicFunction&) = delete;
    PeriodicFunction& operator = (const PeriodicFunction&) = delete;

    /// Calls stop.
    ~PeriodicFunction() { stop(); }

    // Start/Stop
    // ==========

    /// True, if an invocation thread is active.
    bool active() const noexcept { return m_thread.joinable(); }

    /// Start an invocation thread and repeatedly invoke the function (in the given period)
    /// after the given delay.
    /// - If a previous invocation thread is active, no invocation of the function
    ///   takes place.
    /// - After the first invocation of the function (at least one is done):
    ///   - If the period is or has become zero (due to a call to stop) the
    ///     invocation thread stops without any further invocation of the function.
    ///   - As long as an invocation of the function has not finished, the next
    ///     possible invocation is delayed by the given period.
    /// \EXCEPTION All exceptions stop the invocation thread and the exception of the
    ///            last invocation is avaliable through a call to rethorow_exception.
    /// \RETURN    True if no previous call to start is active, otherwise false.
    /// \NOTE      The period is not an exact value (due to interleaving calls between
    ///            each invocation of the function).
    public:
    template <
        typename Period, typename  PeriodRatio,
        typename Delay, typename DelayRatio>
    bool start(
        const std::chrono::duration<Period, PeriodRatio>& period,
        const std::chrono::duration<Delay, DelayRatio>& delay) noexcept;

    /// Start an invocation thread and repeatedly invoke the function without delay.
    template <typename Period, typename PeriodRatio>
    bool start(const std::chrono::duration<Period, PeriodRatio>& period) noexcept {
        return start(period, std::chrono::duration<Period, PeriodRatio>::zero());
    }

    /// Set the period of invocations to zero.
    /// If an invocation thread is active, stop invocations of the function
    /// and wait until the thread has finished.
    /// \SEE std::thread::join
    void stop() {
        m_period = period_type::zero();
        if(active()) m_thread.join();
    }

    // Period
    // ======

    public:
    const period_type& period() const noexcept { return m_period; }

    // Exception
    // =========

    public:
    /// True if an exception occured in the last invocation thread.
    bool exception() const { return bool(m_exception); }
    /// Throw the exception of the last invocation thread, if availabe.
    void rethrow_exception() const {
        if(exception())
            std::rethrow_exception(m_exception);
    }

    // Utility [invoke_synchron]
    // =========================

    private:
    void invoke_synchron() {
        invoke_synchron(std::is_same<value_type, void>());
    }
    // not void
    void invoke_synchron(std::false_type) {
        static_cast<Derived*>(this)->transfer(m_function());
    }
    // void
    void invoke_synchron(std::true_type) {
        m_function();
    }

    // Utility [invoke_asynchron]
    // ==========================

    private:
    void invoke_asynchron() {
        m_future = std::async(std::launch::async, m_function);
    }

    // Utility [transfer_asynchron]
    // ============================

    private:
    void transfer_asynchron() {
        transfer_asynchron(std::is_same<value_type, void>());
    }
    // not void
    void transfer_asynchron(std::false_type) {
        static_cast<Derived*>(this)->transfer(m_future.get());
    }
    // void
    void transfer_asynchron(std::true_type) {
        m_future.get();
    }

    private:
    function_type m_function;
    period_type m_period;
    std::thread m_thread;
    future_type m_future;
    std::exception_ptr m_exception;
};

// Start/Stop
// ==========

template <typename Value, typename Derived>
template <
    typename Period, typename  PeriodRatio,
    typename Delay, typename DelayRatio>
bool PeriodicFunction<Value, Derived>::start(
    const std::chrono::duration<Period, PeriodRatio>& period,
    const std::chrono::duration<Delay, DelayRatio>& delay) noexcept
{
    if(active()) return false;
    try {
        m_exception = std::exception_ptr();
        m_period = std::chrono::duration_cast<period_type>(period);
        m_thread = std::thread([this, delay]() {
            try {
                std::this_thread::sleep_for(delay);
                if(this->m_period == period_type::zero()) this->invoke_synchron();
                else {
                    this->invoke_asynchron();
                    while(true) {
                        std::this_thread::sleep_for(this->m_period);
                        if(this->m_period != period_type::zero()) {
                            if(this->m_future.wait_for(period_type::zero()) == std::future_status::ready) {
                                this->transfer_asynchron();
                                this->invoke_asynchron();
                            }
                        }
                        else {
                            this->m_future.wait();
                            this->transfer_asynchron();
                            break;
                        }
                    }
                }
            }
            catch(...) {
                this->m_exception = std::current_exception();
            }
        });
    }
    catch(...) {
        this->m_exception = std::current_exception();
    }
    return true;
}

} // namespace Detail


// PeriodicFunction
// =============================================================================

template <typename Value>
class PeriodicFunction : public Detail::PeriodicFunction<Value, PeriodicFunction<Value>>
{
    // Types
    // =====

    private:
    typedef Detail::PeriodicFunction<Value, PeriodicFunction<Value>> Base;
    friend Base;

    public:
    typedef typename Base::period_type period_type;
    typedef typename Base::value_type value_type;
    typedef std::list<value_type> result_type;

    private:
    typedef std::mutex mutex;

    // Construction
    // ============

    public:
    template <typename Callable, typename... Arguments>
    PeriodicFunction(Callable&& callable, Arguments&&... arguments)
    :   Base(callable, std::forward<Arguments>(arguments)...)
    {}

    // Result
    // ======

    /// True, if the internal result buffer is empty.
    bool empty() const { return m_result.empty(); }

    /// Return the current result of invocations of the function and clear
    /// the result buffer.
    /// \NOTE If the invoking thread is still running, new results
    ///       might become available.
    result_type result() {
        std::lock_guard<mutex> guard(m_result_mutex);
        return std::move(m_result);
    }

    // Utility
    // =======

    private:
    void transfer(value_type&& result) {
        std::lock_guard<mutex> guard(m_result_mutex);
        m_result.push_back(result);
    }

    private:
    mutex m_result_mutex;
    result_type m_result;
};



// PeriodicFunction<void>
// ======================

template <>
class PeriodicFunction<void> : public Detail::PeriodicFunction<void, PeriodicFunction<void>>
{
    // Types
    // =====

    private:
    typedef Detail::PeriodicFunction<void, PeriodicFunction<void>> Base;
    friend Base;

    public:
    typedef typename Base::period_type period_type;
    typedef typename Base::value_type value_type;


    // Construction
    // ============

    public:
    template <typename Callable, typename... Arguments>
    PeriodicFunction(Callable&& callable, Arguments&&... arguments)
    :   Base(callable, std::forward<Arguments>(arguments)...)
    {}
};


// Test
// ====

#define TEST_OUTPUT 0

class Exception : public std::runtime_error
{
    public:
    Exception() noexcept
    :   std::runtime_error("Test")
    {}
};

const unsigned f_limit = 10;
std::atomic<unsigned> f_count;
char f() {
    ++f_count;
    return '.';
}

const unsigned g_limit = 3;
std::atomic<unsigned> g_count;
void g() {
    if(++g_count == g_limit) throw Exception();
}


int main()
{
    try {
        using std::chrono::milliseconds;
        // With Return Type
        {
            PeriodicFunction<char> invoke(f);
            #if(TEST_OUTPUT)
            std::ostream& stream = std::cout;
            #else
            std::ostream null(0);
            std::ostream& stream = null;
            #endif
            invoke.start(milliseconds(10));
            for(unsigned i = 0; i < f_limit; ++i) {
                std::this_thread::sleep_for(milliseconds(100));
                if(i == f_count - 1) invoke.stop();
                auto result = invoke.result();
                for(const auto& r : result)
                    stream << r;
                stream << '\n';
            }
        }
        // Void
        {
            PeriodicFunction<void> invoke(g);
            invoke.start(milliseconds(10), milliseconds(100));
            // A thread shall throw an exception before stopping
            std::this_thread::sleep_for(milliseconds(200));
            invoke.stop();
            try { invoke.rethrow_exception(); }
            catch(Exception) { return 0; }
        }
    }
    catch(...) {}
    return -1;
}

Fixes:

The stop function will fail if called from the same thread, hence I changed it to:

void stop() {
    m_period = period_type::zero();
    if(is_active() && std::this_thread::get_id() != m_thread.get_id())
        m_thread.join();
}

The PeriodicFunction::result() does not clear the internal state of the result container after the move:

result_type result() {
    std::lock_guard<std::mutex> guard(m_result_mutex);
    result_type result(std::move(m_result));
    m_result.clear();
    return result;
}
\$\endgroup\$
  • \$\begingroup\$ typedef std::mutex mutex;? Also you can't return -1 from main. \$\endgroup\$ – edmz Apr 16 '14 at 13:23
  • \$\begingroup\$ @no1 You can, in fact, return -1 from main. It has the effect of calling std::exit with an argument of -1, which, according to [support.start.term], has implementation-defined behavior. On POSIX systems, this is equivalent to returning 255. \$\endgroup\$ – ruds Apr 28 '14 at 19:21
  • \$\begingroup\$ @ruds I said that because he likely expected a $? = -1, but that won't happen since the shell uses an 8 bit unsigned int (char o whatever) for the return type. Also, in case of failure, you should return a positive value different from zero. \$\endgroup\$ – edmz Apr 28 '14 at 19:40
  • \$\begingroup\$ The EXIT_FAILURE macro, defined in <cstdlib>, is probably a good default if you don't intend to return specific statuses to the shell. \$\endgroup\$ – ruds Apr 28 '14 at 20:44
8
\$\begingroup\$

Generally speaking, the code already seems really good. Therefore, I have nothing more than a few notes:

  • It is more of a matter of taste, but I would use some type aliases instead of the old typedef. The type aliases can be templated (contrary to typedef) and provide a syntax that is close to variable assignements (auto a = 89;) and namespace aliases (namespace foobar = foo::bar;). That means that you can write code which looks more consistent:

    using period_type = std::chrono::milliseconds;
    using value_type = Value;
    
    using function_type = std::function<Value ()>;
    using future_type = std::future<Value>;
    
  • C++ has special syntax to import type names from a base class without having to use a full typedef or type alias, you can use it to reduce the amount of code and make explicit your intent in PeriodicFunction:

    using typename Base::period_type;
    using typename Base::value_type;
    
  • typedef std::mutex mutex; seems useless and potentially confusing. I would drop the typedef and use std::mutex everywhere instead. It shouldn't hinder readability.

  • In their answer to your previous question, @ruds said that Timer::m_results should not be mutable, which is right, but I see no harm in having the corresponding std::mutex be mutable. It does not seem to be currently useful though.

  • Really no more than a tidbit, but namespaces are generally lowercase. Consider replacing Detail by detail.

  • Functions that return a bool tend to be more understandable when their name is more than just a name. For example, bool exception() {} makes me think that the function returns an std::exception or a derived class (which would be odd). Consider renaming it exception_occured instead. Also, consider renaming active to is_active.

    Unfortunately, the standard library containers have a function empty while is_empty would have been a better name ("empty" somehow means "empty that container" to me); I understand that you keep this particular name for consistency.

Discussing what can be improved is good, but I also want to put a note about what you did right: tag dispatching with std::true_type and std::false_type is great. It helps to write simple and readable code and to avoid some std::enable_if and template boilerplate. You also correctly implemented perfect forwarding and used proper scoped locks. Kudos for that :)

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.