Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makes task_timer generic #897

Merged
merged 3 commits into from
Sep 29, 2024

Conversation

StefanoPetrilli
Copy link
Contributor

This pull request attempts to solve #884 by making taks_timer generic and introducing default_task_timer and
milliseconds_task_timer.
default_task_timer just replaces all the previous instances of task timer, while milliseconds_task_timer replaces the use of task_timer in the test suite.
Using this approach, we no longer need to wait several seconds for the task_timer to complete.

@gittiver gittiver linked an issue Sep 10, 2024 that may be closed by this pull request
mrozigor
mrozigor previously approved these changes Sep 11, 2024
@StefanoPetrilli
Copy link
Contributor Author

I pushed a new version where I increased the timeout of the tasks in the tests. The previous delays were just a handful of milliseconds, and this made the test give false negative sometimes.

@gittiver
Copy link
Member

gittiver commented Sep 13, 2024

Hi Stefano,

I would prefer to have the tick length as constructor parameter like this:

class task_timer
        {
        public:
            using task_type = std::function<void()>;
            using identifier_type = size_t;

        private:
            using clock_type = std::chrono::steady_clock;
            using time_type = clock_type::time_point;
        public:
            task_timer(asio::io_service& io_service,
                       const std::chrono::milliseconds tick_length = std::chrono::seconds(1)):
              io_service_(io_service), timer_(io_service_), tick_length_ms(tick_length)
            {
                timer_.expires_after(tick_length_ms);
                timer_.async_wait(
                  std::bind(&task_timer::tick_handler, this, std::placeholders::_1));
            }

            ~task_timer() { timer_.cancel(); }

            void cancel(identifier_type id)
            {
                tasks_.erase(id);
                CROW_LOG_DEBUG << "task_timer cancelled: " << this << ' ' << id;
            }

            /// Schedule the given task to be executed after the default amount of ticks.

            ///
            /// \return identifier_type Used to cancel the thread.
            /// It is not bound to this task_timer instance and in some cases could lead to
            /// undefined behavior if used with other task_timer objects or after the task
            /// has been successfully executed.
            identifier_type schedule(const task_type& task)
            {
                tasks_.insert(
                  {++highest_id_,
                   {clock_type::now() + tick_length_ms * get_default_timeout(),
                    task}});
                CROW_LOG_DEBUG << "task_timer scheduled: " << this << ' ' << highest_id_;
                return highest_id_;
            }

            /// Schedule the given task to be executed after the given time.

            ///
            /// \param timeout The amount of ticks to wait before execution.
            ///
            /// \return identifier_type Used to cancel the thread.
            /// It is not bound to this task_timer instance and in some cases could lead to
            /// undefined behavior if used with other task_timer objects or after the task
            /// has been successfully executed.
            identifier_type schedule(const task_type& task, std::uint8_t timeout)
            {
                tasks_.insert({++highest_id_,
                               {clock_type::now() + (timeout * tick_length_ms), task}});
                CROW_LOG_DEBUG << "task_timer scheduled: " << this << ' ' << highest_id_;
                return highest_id_;
            }

            /// Set the default timeout for this task_timer instance. (Default: 5)

            ///
            /// \param timeout The amount of ticks (seconds) to wait before execution.
            void set_default_timeout(std::uint8_t timeout) { default_timeout_ = timeout; }

            /// Get the default timeout. (Default: 5)
            std::uint8_t get_default_timeout() const { return default_timeout_; }

        private:
            void process_tasks()
            {
                time_type current_time = clock_type::now();
                std::vector<identifier_type> finished_tasks;

                for (const auto& task : tasks_)
                {
                    if (task.second.first < current_time)
                    {
                        (task.second.second)();
                        finished_tasks.push_back(task.first);
                        CROW_LOG_DEBUG << "task_timer called: " << this << ' ' << task.first;
                    }
                }

                for (const auto& task : finished_tasks)
                    tasks_.erase(task);

                // If no task is currently scheduled, reset the issued ids back to 0.
                if (tasks_.empty()) highest_id_ = 0;
            }

            void tick_handler(const error_code& ec)
            {
                if (ec) return;

                process_tasks();

                timer_.expires_after(tick_length_ms);
                timer_.async_wait(
                  std::bind(&task_timer::tick_handler, this, std::placeholders::_1));
            }

        private:
            asio::io_service& io_service_;
            asio::basic_waitable_timer<clock_type> timer_;
            std::map<identifier_type, std::pair<time_type, task_type>> tasks_;

            // A continuously increasing number to be issued to threads to identify them.
            // If no tasks are scheduled, it will be reset to 0.
            identifier_type highest_id_{0};
            std::chrono::milliseconds tick_length_ms;
            std::uint8_t default_timeout_{5};

        };

this spares the templated code and is more flexible as it allows also units other than 1 for the std::chrono::duration

@StefanoPetrilli
Copy link
Contributor Author

@gittiver I did not understand when you commented on the issue, but now that I see your solution I like it much more than mine. I just pushed a commit that implements your solution.

The macOS workflow seems to give false negatives with short interval of times, I set the ticks to be 100 milliseconds, let's see if it's enough.

include/crow/task_timer.h Outdated Show resolved Hide resolved
include/crow/task_timer.h Outdated Show resolved Hide resolved
@gittiver gittiver merged commit b8ddff6 into CrowCpp:master Sep 29, 2024
11 checks passed
@StefanoPetrilli
Copy link
Contributor Author

@gittiver thank you for solving the comments, I haven't had time to do it myself. 👍

@gittiver
Copy link
Member

you are welcome. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor task_timer to allow mocking the timer
3 participants