CrowCpp / Crow

A Fast and Easy to use microframework for the web.
https://crowcpp.org
Other
3.17k stars 350 forks source link

Makes task_timer generic #897

Closed StefanoPetrilli closed 3 days ago

StefanoPetrilli commented 3 weeks ago

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.

StefanoPetrilli commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

@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.

StefanoPetrilli commented 2 days ago

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

gittiver commented 2 days ago

you are welcome. 👍