Implementing timeout of a resource (in this case a database connection)

Code

• Acquire not aquire.
• The scoped_thread move constructor is missing a &, and should have the signature scoped_thread(scoped_thread&&) = delete.
• system_clock is the wrong clock type to use, as it's not monotonic (the user can adjust it whenever they choose). steady_clock would probably be the right choice here.
• Since scoped_thread stores the thread object as a member, the scoped_thread constructor could use variable template arguments and perfect forwarding, rather than passing in a std::thread object. This makes the user code slightly shorter.
• When declaring time constants (tlim, sleeping_time), it would be better to use std::chrono types at the point of declaration, rather than later on in the code, so that the constant has units associated with it directly.

• (Style/Opinion:) Vertical spacing makes code easier to read:

  using clock_type = chrono::system_clock;
  using shared_deadline = std::atomic<clock_type::time_point>;
  
  constexpr auto tlim=100;
  
  template<typename T> void timeout(T& shared_resource,const shared_deadline& deadline)
  

  With a little extra spacing, it's a lot easier to parse this visually as two using declarations, a constant and the start of a function.

Design - locking

There are a couple of places where the resource is locked and then unlocked again rapidly, e.g.:

    if((*shared_resource.aquire())==0)
    {
        *shared_resource.aquire()=1; // will replace by database connect
        std::cout << "Setting resource to " << (*shared_resource.aquire()) << "\n";

Maintaining the lock would be better, otherwise what's printed to std::cout may not be what you expect:

    auto proxy = shared_resource.aquire();

    if (*proxy == 0)
    {
        *proxy = 1; // will replace by database connect
        std::cout << "Setting resource to " << *proxy << "\n";

Design - cancelling / timeout behaviour

• If you comment out every line after creating the timeout object:

  auto th = std::make_unique<scoped_thread>(std::thread(timeout<LockWrapper<int>>, std::ref(shared_resource), std::cref(deadline)));
  

  main() won't complete until the timeout finishes. This is probably not desirable behaviour.

• There is no way to cancel the timeout without manually setting a deadline zero seconds in the future (i.e. dodgy hacks). Furthermore, the fixed sleep of 1 second inside timeout() means you may have to wait up to a second anyway.

• Sleeping for one second in a loop seems like strange behaviour, since we know exactly how long we actually need to sleep for before waking up.

• It is error-prone to manually increase the deadline every time an operation that requires the connection is used.

Changes

Some of the problems above can be solved by encapsulating the timeout functionality in a class, and using a std::condition_variable to do the timed waiting. This also allows the callback to be cancelled (or called immediately) if desired.

class ThreadedCallback
{
public:

    using clock_t = chrono::steady_clock;
    using duration_t = clock_t::duration;
    using action_t = std::function<void()>;

    ThreadedCallback(duration_t timeout, action_t const& action)
    {
        start(timeout, action);
    }

    ~ThreadedCallback()
    {
        cancel();
    }

    ThreadedCallback(ThreadedCallback&&) = delete;
    ThreadedCallback(ThreadedCallback const&) = delete;

    void restart(duration_t timeout, action_t const& action)
    {
        cancel();

        start(timeout, action);
    }

    void cancel()
    {
        {
            std::lock_guard<std::mutex> lock(m_mutex);

            if (!m_cancelled)
            {
                m_cancelled = true;
                m_cv.notify_one();
            }
        }

        assert(m_thread.joinable());
        m_thread.join();
    }

private:

    void start(duration_t timeout, action_t const& action)
    {
        assert(!m_thread.joinable());
        m_thread = std::thread([=] () { run(timeout, action); });
    }

    void run(duration_t timeout, action_t const& action)
    {
        std::unique_lock<std::mutex> lock(m_mutex);

        m_cancelled = false;
        m_cv.wait_for(lock, timeout, [&] () { return m_cancelled; });

        // note: this check can be removed (and cancel() renamed to call_now()) 
        // or a boolean flag added to cancel() to indicate whether the action should still be called anyway
        if (!m_cancelled)
            action();
    }

    std::thread m_thread;
    std::mutex m_mutex;
    std::condition_variable m_cv;
    bool m_cancelled;
};

int main()
{
    int int_value = 1;
    LockWrapper<int> shared_resource(&int_value);

    auto disconnect = [&] ()
    {
        auto proxy = shared_resource.acquire();
        *proxy = 0; // will replace by database disconnect
        std::cout << "Setting resource to " << *proxy << " after timeout occurred in thread " << std::this_thread::get_id() << ".\n";
    };

    auto timeout = std::chrono::seconds(5);
    ThreadedCallback callback(timeout, disconnect);

    constexpr auto tlim = std::chrono::seconds(50);
    constexpr auto sleeping_time = std::chrono::seconds(2);
    for (auto t = std::chrono::seconds(0); t < tlim; t += sleeping_time) // This loop simulates repeated database access
    {
        std::cout << "Slept for " << t.count() << " seconds. Sleeping for another " << sleeping_time.count() << " seconds\n";
        std::this_thread::sleep_for(sleeping_time);

        auto proxy = shared_resource.acquire();

        if (*proxy == 0)
        {
            *proxy = 1; // will replace by database connect
            std::cout << "Setting resource to " << *proxy << "\n";

            callback.restart(timeout, disconnect);
        }
        else
        {
            callback.restart(timeout, disconnect);
        }
    }

    std::cout << "done!" << std::endl;
}

This still doesn't solve the issue of manually restarting the timer with every access operation. Perhaps a timeout class like this would be better kept in the resource (or LockWrapper) itself.

For example, since we want the time after the last access, the Proxy class destructor could start the timeout (e.g. call a function in the LockWrapper to create a ThreadedCallback in a unique_pointer), and the Proxy constructor could call a function to clear that pointer (cancelling the disconnect operation).

Then the timeout would be directly tied to the access itself, and not have to be reset manually by user code.