Code review comment for lp:~andreas-pokorny/mir/add-dispatchable-alarm-factory

Ok.

I've got one guaranteed needs-fixing, which is that the TimerFd interface is incorrect - it combines both a factory for TimerFds and the TimerFd interface itself, with the result that there's nothing stopping you allocating a mir::Fd from one TimerFd implementation and using it on another.

It should be split into TimerFdProvider and TimerFd interfaces.

Secondarily, I'm pretty sure the simplification would work - here's mostly complete code:

class AlarmFactory : public Dispatchable, public time::AlarmFactory
{
public:
    AlarmFactory(std::shared_ptr<time::TimerFdProvider> const& timer_source);
    std::unique_ptr<time::Alarm> create_alarm(std::function<void()> const& callback) override;
    std::unique_ptr<time::Alarm> create_alarm(std::unique_ptr<LockableCallback>&& callback) override;

    Fd watch_fd() const override;
    bool dispatch(FdEvents events) override;
    FdEvents relevant_events() const override;
private:
    std::shared_ptr<time::TimerFdProvider> const timer_source;
    MultiplexingDispatchable multiplexer;
};

class Alarm : public mt::Alarm
{
public:
    Alarm(
        std::unique_ptr<mt::TimerFd>&& timer,
        std::unique_ptr<mir::LockableCallback>&& callback,
        md::MultiplexingDispatchable& multiplexer)
        : timer{std::move(timer)},
          callback{callback.release(),
              [this](auto* victim)
              {
                  destroyed.notify_all();
                  delete victim;
              }}
    {
        std::weak_ptr<mir::LockableCallback> weak_cb = this->callback;
        auto internal_callback = [this, weak_cb]()
            {
                if (auto cb = weak_cb.lock())
                {
                    std::lock_guard<decltype(state_mutex)> lock{state_mutex};

                    if (state_holder == mt::Alarm::State::pending)
                    {
                        state_holder = mt::Alarm::State::triggered;
                        (*cb)();
                    }
                }
                return !weak_cb.expired();
            }
        multiplexer.add_watch(this->timer->fd(), internal_callback);
    }

    ~Alarm()
    {
        {
            std::lock_guard<decltype(state_mutex)> lock(state_mutex);

            if (state_holder == triggered)
            {
                // Either there are no pending calls or we're currently in the callback.
                // In either case, we don't have to wait.
                return;
            }
        }

        cancel();
        std::weak_ptr<mir::LockableCallback> canary = callback;
        callback.reset();

        // We need to be triggered once more to remove us from the MultiplexingDispatchable.
        reschedule_in(0ms);

        // Might be better to take shared ownership of the MultiplexingDispatchable and
        // manually remove_watch() us. Would let us not have to wait for a dispatch() iteration.
        if (!canary.expired())
        {
            std::unique_lock<decltype(state_mutex)> lock(state_mutex);

            destroyed.wait(lock, [this]() { canary.expired(); });
        }

    }

    bool cancel() override
    {
        std::unique_lock<decltype(state_mutex)> lock(state_mutex);

        if (state_holder != triggered)
        {
            state_holder = cancelled;
            timer->cancel();
        }
        return state_holder == cancelled;
    }

    State state() const override
    {
        // Obvious thing goes here.
    }

    bool reschedule_in(std::chrono::milliseconds delay) override
    {
        // Obvious thing goes here
    }

    bool reschedule_for(mt::Timestamp timeout) override
    {
        timer->schedule_for(timeout);
    }

private:
    std::unique_ptr<mt::TimerFd> timer;
    std::recursive_mutex state_mutex;
    mt::Alarm::State state_holder;
    std::condition_variable destroyed;
    std::shared_ptr<mir::LockableCallback> callback;
};