6
\$\begingroup\$

The simple but generic timeout class to be used watching for network connections, user input, filesystem events, and is intended to have a very simple interface specific to only our use cases (i.e. no satisfy-all attitude).

Intended steps to use:

  1. Construct
  2. Activate
  3. Potentially react to a timeout, deactivates itself
  4. Re-activate
  5. Destruct cleanly

After triggering the alarm the guard is expected to be inactive until explicitly activated. The code expected to be reasonably tested (that is one of the issues).

This posted code as a testable Visual Studio 2013 project lives on the GitHub.

Besides a general feedback on the code quality - or if, please, please, please, you see a bug, I would love to hear about these areas:

  1. Destruction. Although I did my best to tell the guard thread to end, I am still concerned about having to join() in the destructor. Generally I love my destructors short and sweet for emergency landings - is it possible here? Is there an STL way to brutally kill that thread?

  2. Tests. Existing ones test for intended simple scenarios. I am not sure this is enough to claim that the code works as intended. Is it? I did not find a better way to test timing edge cases. Also, as tests are time-dependant, they occasionally spuriously fail when run on slow VMs. Or are they? Is it sufficient for code like this to know that if tests run somewhere consistently? If I increase timeouts the spurious fails go away, but that lengthens the overall project test run.

Below are the current header, implementation, and tests files to save the GitHub trip.

Header:

#pragma once

namespace utility
{
    /**
        The `clock` alias is for easy switching to `steady_clock` once Microsoft fixes it
    */
    typedef std::chrono::system_clock clock;

    /**
        The `TimeoutGuard` class triggers the `alarm` callback from the `guard_thread`
        if `touch` was not called for at least the `timeout` duration.

        Because of the way the `guard_thread` sleeps, the actual detection may happen
        as late as after `timeout` + `naptime` duration. Hence it is possible that the alarm
        will not be called if the `TimeoutGuard` instance is touched within the
        'timeout` and `timeout` + `naptime` timeframe.

        If not provided, by default the `naptime` is same as `timeout`.

        The `TimeoutGuard` is not active after construction, whicn means, that the
        `guard_thread` will block until it is activated by calling the `watch` method.

        The `TimeoutGuard` class is not copyable and not moveable.
    */
    class TimeoutGuard
    {
    public:
        TimeoutGuard(
            clock::duration timeout,
            std::function<void( void )> alarm,
            clock::duration naptime
        );

        TimeoutGuard(
            clock::duration timeout,
            std::function<void( void )> alarm
        );

        ~TimeoutGuard();

        TimeoutGuard( const TimeoutGuard & ) = delete;
        TimeoutGuard & operator=(const TimeoutGuard & ) = delete;

        TimeoutGuard( TimeoutGuard && ) = delete;
        TimeoutGuard & operator=( TimeoutGuard && ) = delete;

        void watch();
        void touch();

    private:

        void guard();

        clock::duration timeout;
        clock::duration naptime;
        std::function<void( void )> alarm;

        std::atomic_bool idle;
        std::atomic_bool live;

        std::atomic<clock::time_point> touched;

        std::thread guard_thread;
        std::mutex guard_mutex;
        std::condition_variable wakeup;
    };
}

Here is the implementation:

#include "stdafx.h"
#include "TimeoutGuard.h"

namespace utility
{
    TimeoutGuard::TimeoutGuard(
        clock::duration timeout,
        std::function<void( void )> alarm,
        clock::duration naptime
    )
        : timeout( timeout )
        , alarm( alarm )
        , naptime( naptime )
    {
        idle.store( true );
        live.store( true );

        guard_thread = std::thread( std::bind( &TimeoutGuard::guard, this ) );
    }

    TimeoutGuard::TimeoutGuard(
        clock::duration timeout,
        std::function<void( void )> alarm
    )
    : TimeoutGuard( timeout, alarm, timeout )
    {};

    TimeoutGuard::~TimeoutGuard()
    {
        live.store( false );
        wakeup.notify_all();
        guard_thread.join();
    }

    void TimeoutGuard::guard()
    {
        while ( live.load() )
        {
            if ( idle.load() )
            {
                // Sleep indefinitely until either told to become active or destruct
                std::unique_lock<std::mutex> live_lock( guard_mutex );
                wakeup.wait( live_lock, [this]() { return ! this->idle.load() || ! this->live.load(); } );
            };

            // quit the loop if destructing
            if ( ! live.load() ) break;

            // the actual timeout checking
            auto now = clock::now();

            if ( ( now - touched.load() ) > timeout )
            {
                idle.store( true );
                alarm();
                continue; // skip waiting for next timeout
            }

            {
                // sleep until next timeout check or destruction
                std::unique_lock<std::mutex> live_lock( guard_mutex );
                wakeup.wait_for( live_lock, naptime, [this](){ return ! this->live.load(); } );
            }
        };
    }

    void TimeoutGuard::watch()
    {
        touch();
        idle.store( false );
        wakeup.notify_all();
    }

    void TimeoutGuard::touch()
    {
        touched.store( clock::now() );
    }
}

And, finally, existing tests:

#include "stdafx.h"
#include "CppUnitTest.h"

#include "TimeoutGuard.h"

using namespace Microsoft::VisualStudio::CppUnitTestFramework;

namespace utility
{       
    TEST_CLASS( TimeoutGuardTest )
    {
    public:

        bool triggered = false;

        void shoud_trigger()
        {
            triggered = true;
        }

        TEST_METHOD( TimeoutGuardExpiration )
        {
            TimeoutGuard tg{
                std::chrono::milliseconds{ 5 },
                std::bind( &TimeoutGuardTest::shoud_trigger, this )
            };

            triggered = false;
            tg.watch();
            std::this_thread::sleep_for( std::chrono::milliseconds{ 10 } );
            Assert::IsTrue( triggered, L"Failed to call the timeout alarm on the first run", LINE_INFO() );

            triggered = false;
            tg.watch();
            std::this_thread::sleep_for( std::chrono::milliseconds{ 10 } );
            Assert::IsTrue( triggered, L"Failed to call the timeout alarm on the second run", LINE_INFO() );
        }


        TEST_METHOD( TimeoutGuardNoAlarm )
        {
            TimeoutGuard tg{
                std::chrono::milliseconds{ 5 },
                std::bind( &TimeoutGuardTest::shoud_trigger, this )
            };

            triggered = false;
            tg.watch();
            std::this_thread::sleep_for( std::chrono::milliseconds{ 1 } );
            Assert::IsFalse( triggered, L"Wrongly called the timeout alarm on the first run", LINE_INFO() );

            triggered = false;
            tg.watch();
            for (auto i = 0; i < 10; ++i)
            {
                std::this_thread::sleep_for( std::chrono::milliseconds{ 1 } );
                tg.touch();
            }
            Assert::IsFalse( triggered, L"Wrongly called the timeout alarm on the second run", LINE_INFO() );
        }
    };
}
\$\endgroup\$
1

1 Answer 1

1
\$\begingroup\$

Header files should be independent

You haven't shown us your "stdafx.h" header, but TimeoutGuard.h seems to depend on it including several standard headers. It's much easier for users if headers include everything they need, so that they can be included in any order without mysterious failures.

I had to add

#include <atomic>
#include <chrono>
#include <condition_variable>
#include <functional>
#include <mutex>
#include <thread>

Conditionalise the platform workaround

This comment suggests that most platforms should be using the standard steady clock:

/**
    The `clock` alias is for easy switching to `steady_clock` once Microsoft fixes it
*/

I would replace with

#ifdef WIN32  // TODO: is this the correct macro?
    // Microsoft's steady_clock is unsuitable because …
    using clock = std::chrono::system_clock;
#else
    using clock = std::chrono::steady_clock;
#endif

No need to delete move operations

If copy constructor is deleted, then move constructor is also deleted; similarly for assignment operators.

In fact, we don't need to delete copy construction or assignment, because the non-copyable member variables prevent any of those functions being provided by the compiler.


Write initialisers in the order they will run

We have

    clock::duration timeout;
    clock::duration naptime;
    std::function<void( void )> alarm;

but

TimeoutGuard::TimeoutGuard(
    clock::duration timeout,
    std::function<void( void )> alarm,
    clock::duration naptime
)
    : timeout( timeout )
    , alarm( alarm )
    , naptime( naptime )

That's misleading to readers - initialise members in the same order they are declared, as that's the order in which they execute (it doesn't matter here just now, but becomes important when they have dependencies on each other).


Initialise the atomics

Default-constructed std::atomic_bool is uninitialised, and needs std::atomic_init() rather than .store(). It's simpler to construct with a value:

        std::atomic_bool idle{true};
        std::atomic_bool live{true};

Prefer value-initialisation to default+assignment

We default-initialise guard_thread but immediately overwrite in the body of the constructor. It's better to initialise with the value:

    TimeoutGuard::TimeoutGuard(clock::duration timeout,
                               std::function<void(void)> alarm,
                               clock::duration naptime)
        : timeout{timeout},
          naptime{naptime},
          alarm{alarm},
          guard_thread{std::bind(&TimeoutGuard::guard, this)}
    {}

Use correct consistency model

live.load() (with no arguments) can be written more simply as live(), and live.store(false) is exactly equivalent to store = false. But full sequential consistency is more than we need here. We should be using appropriate std::memory_order values with these loads and stores.


Consider more immediate checking

Users might dislike this:

    /**
        Because of the way the `guard_thread` sleeps, the actual detection may happen
        as late as after `timeout` + `naptime` duration.

Instead we might eliminate naptime, and always sleep until touched + timeout. That will slightly increase our wakeups, but by no more than once per timeout.


Allow suspending and restarting

The wait for idle to become true could be moved outside of the loop, as it never becomes false later.

However, it's probably better to provide a function to put the thread back into the idle state, so we can re-use the guard rather than having to construct afresh:

void TimeoutGuard::suspend()
{
    idle.store(true, std::memory_order_release);
    wakeup.notify_all();
}

Allow changing the timeout

If we store the next wake-up time instead of the last touched time, we can simplify by replacing the idle status with a "wait forever".

That also allows us to change the interface so that we can specify a different timeout in the touch() call. Then users can vary the timeout according to the phase of operation, rather than having to create several timeout guards.


Use std::jthread to simplify implementation

Since C++20, we have std::jthread which provides request_stop() and automatically joins when destructed.

That simplifies our destructor and eliminates the need for live member.


Minor things

  • There's a stray ; after the body of the second constructor.
  • There's an empty statement after if (idle.load()) {…} in guard().
  • timeout and alarm could be declared const.
  • join() can throw an exception, so might be dangerous in the destructor.

Tests

I don't have your test framework, so I re-wrote the tests for Google Test:

#include <gtest/gtest.h>

using namespace std::chrono_literals;

struct watcher {
    bool triggered = false;

    void trigger() {
        triggered = true;
    }
};

utility::TimeoutGuard make_tg(utility::clock::duration t, watcher& w)
{
    return {t, std::bind(&watcher::trigger, std::ref(w))};
}

TEST(TimeoutGuard, Expiration)
{
    auto w = watcher{};
    auto tg = make_tg(5ms, w);

    tg.watch();
    std::this_thread::sleep_for(10ms);
    EXPECT_TRUE(w.triggered);

    w.triggered = false;
    tg.watch();
    std::this_thread::sleep_for(10ms);
    EXPECT_TRUE(w.triggered);
}

TEST(TimeoutGuard, NoAlarm)
{
    auto w = watcher{};
    auto tg = make_tg(5ms, w);

    tg.watch();
    for (auto i = 0; i < 10; ++i) {
        std::this_thread::sleep_for(1ms);
        tg.touch();
        EXPECT_FALSE(w.triggered);
    }
}

There was no test that the alarm isn't repeatedly called, so I added one:

TEST(TimeoutGuard, OneTriggerOnly)
{
    auto w = watcher{};
    auto tg = make_tg(5ms, w);

    tg.watch();
    std::this_thread::sleep_for(2ms);
    EXPECT_FALSE(w.triggered);
    std::this_thread::sleep_for(5ms);
    EXPECT_TRUE(w.triggered);

    w.triggered = false;
    std::this_thread::sleep_for(10ms);
    EXPECT_FALSE(w.triggered);
}

And a test for my new suspend():

TEST(TimeoutGuard, Suspend)
{
    auto w = watcher{};
    auto tg = make_tg(5ms, w);

    tg.watch();
    std::this_thread::sleep_for(2ms);
    EXPECT_FALSE(w.triggered);

    tg.suspend();
    std::this_thread::sleep_for(10ms);
    EXPECT_FALSE(w.triggered);
}

One problem with the tests, as you have observed, is that they are not isolated from the real environment. This can cause execution speed variation to affect the results, and also means the tests are very time consuming (tens of milliseconds, just with the few tests we have so far).

When I worked with a system that had time dependencies that could span several hours, using real time was obviously unworkable. Instead, I created a mockable time abstraction that wraps the clock and the wait operations, and made the code use that abstraction. The mock stored a time-ordered list of future events and had a step() function to activate the next event. The work involved is a lot for a single class such as this, but if you are writing a library with much more time-related code to test, then it will pay back handsomely.


Updated code

#include <atomic>
#include <chrono>
#include <condition_variable>
#include <functional>
#include <mutex>
#include <thread>

namespace utility
{
#ifdef WIN32  // TODO: is this the correct macro?
    // Microsoft's steady_clock is unsuitable because …
    using clock = std::chrono::system_clock;
#else
    using clock = std::chrono::steady_clock;
#endif

    /**
       The `TimeoutGuard` class triggers the `alarm` callback from
       the `guard_thread` if `touch` was not called for at least the
       `timeout` duration.

       The `TimeoutGuard` is not active after construction, whicn
       means that the `guard_thread` will block until it is first
       touched.

       The `TimeoutGuard` class is not copyable and not moveable.
    */
    class TimeoutGuard
    {
    public:
        TimeoutGuard(clock::duration timeout,
                     std::function<void(void)> alarm);

        ~TimeoutGuard();

        void watch();
        void suspend();
        void touch();
        void touch(clock::duration next_timeout);

    private:
        void guard(std::stop_token);

        const clock::duration timeout;
        const std::function<void(void)> alarm;

        std::jthread guard_thread;

        std::atomic<clock::time_point> next_wake = clock::time_point::max();

        std::condition_variable_any wakeup = {};
    };
}
utility::TimeoutGuard::TimeoutGuard(clock::duration timeout,
                                    std::function<void(void)> alarm)
    : timeout{timeout},
      alarm{alarm},
      guard_thread{[this](std::stop_token stoken){ guard(stoken); }}
{}

utility::TimeoutGuard::~TimeoutGuard()
{
    guard_thread.request_stop();
}

void utility::TimeoutGuard::guard(std::stop_token token)
{
    std::mutex guard_mutex = {};
    do {
        // overdue?
        if (clock::now() >= next_wake.load(std::memory_order_acquire)) {
            alarm();
            next_wake.store(clock::time_point::max(), std::memory_order_relaxed);
        }

        // sleep until timeout expires or is reduced, or thread is cancelled
        auto const wake_time = next_wake.load(std::memory_order_acquire);

        auto const watching = [this,&wake_time]{
            // wake if the timeout got reduced
            return next_wake.load(std::memory_order_acquire) < wake_time;
        };
        std::unique_lock<std::mutex> lock(guard_mutex);
        wakeup.wait_until(lock, token, wake_time, watching);
    } while (!token.stop_requested());
}

void utility::TimeoutGuard::watch()
{
    touch();
}

void utility::TimeoutGuard::suspend()
{
    next_wake.store(clock::time_point::max(), std::memory_order_release);
    // no need to notify - guard thread will notice when it next wakes
}

void utility::TimeoutGuard::touch()
{
    touch(timeout);
}

void utility::TimeoutGuard::touch(clock::duration next_timeout)
{
    auto due = clock::now() + next_timeout;;
    if (due < next_wake.exchange(due, std::memory_order_release)) {
        wakeup.notify_all();
    }
}
```
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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