refactor: use GCancellable for timer cancellation#202
refactor: use GCancellable for timer cancellation#202lzwind merged 2 commits intolinuxdeepin:develop/snipefrom
Conversation
- Replace std::atomic<bool> stop_timer_ with GCancellable* cancellable_ - Add cancellable_wait helper function for polling cancellation with timeout - Refactor timer_worker loop to use cancellable_wait instead of sleep loop - Include necessary <gio/gio.h> header - Adjust Qt header order in main.cpp for compilation error This change improves cancellation responsiveness and reduces CPU usage by leveraging GLib's cancellation mechanism. Bug: https://pms.uniontech.com/bug-view-339241.html
|
TAG Bot TAG: 7.0.36 |
Reviewer's GuideRefactors the base_event_handler timer thread to use a GLib GCancellable and pollable FD for cooperative cancellation instead of a custom atomic flag and std::this_thread::sleep_for, and wires in the necessary GLib/GIO headers and object lifetime management. Class diagram for updated base_event_handler timer cancellationclassDiagram
class base_event_handler {
+base_event_handler(event_handler_config config)
+~base_event_handler()
+void terminate_processing()
+void timer_worker(int64_t interval)
-- timer_and_jobs_state --
-anything::index_manager index_manager_
-std::size_t batch_size_
-anything::thread_pool pool_
-GCancellable* cancellable_
-bool delay_mode_
-bool index_dirty_
-bool volatile_index_dirty_
-std::vector<std::string> pending_paths_
-std::vector<anything::index_job> jobs_
-std::mutex jobs_mtx_
-std::mutex pending_mtx_
-std::thread timer_
-std::atomic<bool> stop_scan_directory_
}
class GCancellable {
+GCancellable* g_cancellable_new()
+void g_cancellable_cancel(GCancellable* cancellable)
+gboolean g_cancellable_is_cancelled(GCancellable* cancellable)
+int g_cancellable_get_fd(GCancellable* cancellable)
+void g_cancellable_release_fd(GCancellable* cancellable)
}
class GLibFunctions {
+gint g_poll(GPollFD* fds, guint nfds, gint timeout_ms)
+void g_usleep(gulong microseconds)
}
base_event_handler --> GCancellable : uses
base_event_handler --> GLibFunctions : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
timer_worker,g_cancellable_release_fd(cancellable_)is called unconditionally even ifg_cancellable_get_fd()returned-1; per the GLib API you should only callg_cancellable_release_fd()when a valid fd was obtained, so guard the release call withif (cancellable_fd != -1). - Consider moving the
g_cancellable_get_fd(cancellable_)call insidetimer_workercloser to where it's needed and handling the-1case explicitly (e.g., by falling back tog_usleepor skippingg_poll), to make the control flow and behaviour clearer when an fd-backed cancellable isn't available.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `timer_worker`, `g_cancellable_release_fd(cancellable_)` is called unconditionally even if `g_cancellable_get_fd()` returned `-1`; per the GLib API you should only call `g_cancellable_release_fd()` when a valid fd was obtained, so guard the release call with `if (cancellable_fd != -1)`.
- Consider moving the `g_cancellable_get_fd(cancellable_)` call inside `timer_worker` closer to where it's needed and handling the `-1` case explicitly (e.g., by falling back to `g_usleep` or skipping `g_poll`), to make the control flow and behaviour clearer when an fd-backed cancellable isn't available.
## Individual Comments
### Comment 1
<location> `src/daemon/src/core/base_event_handler.cpp:324-329` </location>
<code_context>
- // }
- // }
- // std::this_thread::sleep_for(std::chrono::milliseconds(interval));
+ int cancellable_fd = g_cancellable_get_fd(cancellable_);
+ while(!cancellable_wait(cancellable_, cancellable_fd, (gint)interval)) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard g_cancellable_release_fd() with a check for a valid fd from g_cancellable_get_fd().
Per the GLib docs, g_cancellable_release_fd() must only be called if g_cancellable_get_fd() returned a non-negative value. In timer_worker(), release_fd() is invoked unconditionally, which can cause undefined behavior when cancellable_fd == -1. Please guard the release call with a check (e.g., only call it when cancellable_fd != -1).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
As title. Log: Update version to 7.0.36
deepin pr auto review我来对这个diff进行详细的代码审查:
总体来说,这是一个质量较高的代码改进,提高了代码的可靠性和响应性。使用GLib的标准机制替代自定义的原子变量是一个明智的选择。 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lzwind, wangrong1069 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary by Sourcery
Replace the custom timer stop flag in the event handler with a GCancellable-based cancellation mechanism for the background timer worker.
Enhancements: