fix: improve device listener thread management#911
Merged
robertkill merged 1 commit intolinuxdeepin:masterfrom Sep 28, 2025
Merged
fix: improve device listener thread management#911robertkill merged 1 commit intolinuxdeepin:masterfrom
robertkill merged 1 commit intolinuxdeepin:masterfrom
Conversation
Reviewer's GuideThe PR refactors the device listener to use joinable threads with a flag-based exit for safe synchronization and cleanup, and overhauls the event loop to poll with XPending and insert sleeps when idle to prevent blocking and high CPU usage. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `inputdevices1/listen.c:48` </location>
<code_context>
}
+ // 主线程等待子线程主动退出
pthread_join(_thrd, NULL);
return 0;
</code_context>
<issue_to_address>
**issue (bug_risk):** Joining the thread in both start_device_listener and end_device_listener may cause double join issues.
Ensure pthread_join is only called once per thread to avoid undefined behavior or hangs. Refactor to manage thread lifecycle consistently.
</issue_to_address>
### Comment 2
<location> `inputdevices1/listen.c:57-48` </location>
<code_context>
- pthread_cancel(_thrd);
- }
+ // 设置退出标志，通知子线程退出
+ _thrd_exit_flag = 1;
+ // 主线程等待子线程退出
+ pthread_join(_thrd, NULL);
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Setting the exit flag and immediately joining may not guarantee prompt thread termination.
If the child thread is blocked in XPending or XNextEvent, it may not exit promptly, causing pthread_join to hang. Consider an additional signal, such as XCloseDisplay, to ensure the thread wakes and checks the exit flag.
Suggested implementation:
```c
// 设置退出标志，通知子线程退出
```
```c
_thrd_exit_flag = 1;
// 关闭Display，唤醒阻塞的XPending/XNextEvent
if (_display != NULL) {
XCloseDisplay(_display);
_display = NULL;
}
// 主线程等待子线程退出
pthread_join(_thrd, NULL);
}
```
- Ensure that `_display` is a global or otherwise accessible variable that holds the `Display*` used by the child thread.
- The child thread should handle the case where `XNextEvent` or `XPending` returns an error due to the closed display, and exit cleanly when `_thrd_exit_flag` is set.
- If `_display` is not currently available in this scope, you will need to refactor your code to make it accessible.
</issue_to_address>
### Comment 3
<location> `inputdevices1/listen.c:47` </location>
<code_context>
return -1;
}
+ // 主线程等待子线程主动退出
pthread_join(_thrd, NULL);
</code_context>
<issue_to_address>
**issue (complexity):** Consider restructuring the thread lifecycle to join only once at shutdown and use a blocking event loop with a wakeup message.
```suggestion
You can simplify this by
1. removing the immediate `pthread_join()` from _start_ (so the thread really runs in the background),
2. joining exactly once in `end_device_listener()`, and
3. restoring a blocking `XNextEvent()` loop, waking it up with a single dummy `ClientMessage` on shutdown.
Start function (no join here):
```c
int start_device_listener() {
_thrd_exit_flag = 0;
int xi_opcode = has_xi2();
if (xi_opcode < 0) {
_thrd_exit_flag = 1;
return -1;
}
pthread_attr_t attr;
pthread_attr_init(&attr);
pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE);
int ret = pthread_create(&_thrd, &attr,
listen_device_thread,
(void*)&xi_opcode);
pthread_attr_destroy(&attr);
if (ret != 0) {
fprintf(stderr, "Create device event listen thread failed\n");
_thrd_exit_flag = 1;
return -1;
}
return 0;
}
```
End function (set flag, wake thread, join exactly once):
```c
void end_device_listener() {
_thrd_exit_flag = 1;
// wake up listener (dummy ClientMessage)
Atom wakeup = XInternAtom(_disp, "WAKEUP_LISTENER", False);
XClientMessageEvent ev = { .type = ClientMessage,
.window = DefaultRootWindow(_disp),
.message_type = wakeup,
.format = 32 };
XSendEvent(_disp, DefaultRootWindow(_disp),
False, NoEventMask, (XEvent*)&ev);
XFlush(_disp);
pthread_join(_thrd, NULL);
if (_disp) XCloseDisplay(_disp);
}
```
Listener thread (blocking, break on either exit‐flag or our wakeup event):
```c
static void* listen_device_thread(void *data) {
int xi_opcode = *(int*)data;
_disp = XOpenDisplay(NULL);
// ... XISelectEvents, etc. ...
Atom wakeup = XInternAtom(_disp, "WAKEUP_LISTENER", False);
while (!_thrd_exit_flag) {
XEvent ev;
XNextEvent(_disp, &ev);
if (ev.type == ClientMessage &&
ev.xclient.message_type == wakeup) {
break;
}
XGenericEventCookie *cookie = &ev.xcookie;
if (cookie->type == GenericEvent &&
cookie->extension == xi_opcode &&
XGetEventData(_disp, cookie)) {
if (cookie->evtype == XI_HierarchyChanged) {
handleDeviceChanged();
}
XFreeEventData(_disp, cookie);
}
}
XCloseDisplay(_disp);
return NULL;
}
```
This removes the double‐join, avoids `nanosleep()` loops, and keeps all original functionality.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
inputdevices1/listen.c
Outdated
| return -1; | ||
| } | ||
|
|
||
| // 主线程等待子线程主动退出 |
There was a problem hiding this comment.
issue (complexity): Consider restructuring the thread lifecycle to join only once at shutdown and use a blocking event loop with a wakeup message.
Suggested change
| // 主线程等待子线程主动退出 | |
| You can simplify this by | |
| 1. removing the immediate `pthread_join()` from _start_ (so the thread really runs in the background), | |
| 2. joining exactly once in `end_device_listener()`, and | |
| 3. restoring a blocking `XNextEvent()` loop, waking it up with a single dummy `ClientMessage` on shutdown. | |
| Start function (no join here): | |
| ```c | |
| int start_device_listener() { | |
| _thrd_exit_flag = 0; | |
| int xi_opcode = has_xi2(); | |
| if (xi_opcode < 0) { | |
| _thrd_exit_flag = 1; | |
| return -1; | |
| } | |
| pthread_attr_t attr; | |
| pthread_attr_init(&attr); | |
| pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE); | |
| int ret = pthread_create(&_thrd, &attr, | |
| listen_device_thread, | |
| (void*)&xi_opcode); | |
| pthread_attr_destroy(&attr); | |
| if (ret != 0) { | |
| fprintf(stderr, "Create device event listen thread failed\n"); | |
| _thrd_exit_flag = 1; | |
| return -1; | |
| } | |
| return 0; | |
| } |
End function (set flag, wake thread, join exactly once):
void end_device_listener() {
_thrd_exit_flag = 1;
// wake up listener (dummy ClientMessage)
Atom wakeup = XInternAtom(_disp, "WAKEUP_LISTENER", False);
XClientMessageEvent ev = { .type = ClientMessage,
.window = DefaultRootWindow(_disp),
.message_type = wakeup,
.format = 32 };
XSendEvent(_disp, DefaultRootWindow(_disp),
False, NoEventMask, (XEvent*)&ev);
XFlush(_disp);
pthread_join(_thrd, NULL);
if (_disp) XCloseDisplay(_disp);
}Listener thread (blocking, break on either exit‐flag or our wakeup event):
static void* listen_device_thread(void *data) {
int xi_opcode = *(int*)data;
_disp = XOpenDisplay(NULL);
// ... XISelectEvents, etc. ...
Atom wakeup = XInternAtom(_disp, "WAKEUP_LISTENER", False);
while (!_thrd_exit_flag) {
XEvent ev;
XNextEvent(_disp, &ev);
if (ev.type == ClientMessage &&
ev.xclient.message_type == wakeup) {
break;
}
XGenericEventCookie *cookie = &ev.xcookie;
if (cookie->type == GenericEvent &&
cookie->extension == xi_opcode &&
XGetEventData(_disp, cookie)) {
if (cookie->evtype == XI_HierarchyChanged) {
handleDeviceChanged();
}
XFreeEventData(_disp, cookie);
}
}
XCloseDisplay(_disp);
return NULL;
}This removes the double‐join, avoids nanosleep() loops, and keeps all original functionality.
There was a problem hiding this comment.
Pull Request Overview
This PR improves the device listener thread management by replacing pthread_cancel with a flag-based exit mechanism and optimizing the event loop to reduce CPU usage. The changes enable proper thread synchronization and cleanup while preventing blocking behavior.
- Convert thread from detached to joinable for proper synchronization
- Replace pthread_cancel with flag-based exit mechanism for safer thread termination
- Add XPending check and nanosleep to prevent blocking and reduce CPU consumption
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
inputdevices1/listen.c
Outdated
Comment on lines
47
to
48
| // 主线程等待子线程主动退出 | ||
| pthread_join(_thrd, NULL); |
There was a problem hiding this comment.
The pthread_join call in start_device_listener() will block indefinitely waiting for the thread to exit, but the thread only exits when _thrd_exit_flag is set to 1, which happens in end_device_listener(). This creates a deadlock situation where start_device_listener() never returns.
Suggested change
| // 主线程等待子线程主动退出 | |
| pthread_join(_thrd, NULL); | |
| // 线程已启动，返回控制权给主程序 |
Comment on lines
+57
to
+59
| _thrd_exit_flag = 1; | ||
| // 主线程等待子线程退出 | ||
| pthread_join(_thrd, NULL); |
There was a problem hiding this comment.
Setting _thrd_exit_flag without proper synchronization (mutex or atomic operations) can lead to race conditions. The thread may not see the updated value immediately due to memory visibility issues.
inputdevices1/listen.c
Outdated
| XFreeEventData(_disp, cookie); | ||
| } else { | ||
| // 没有事件时休眠，避免 CPU 占用 | ||
| struct timespec ts = {0, 10000000}; // 10ms |
There was a problem hiding this comment.
The nanosleep call with 10ms sleep duration may be too frequent for a device listener, potentially causing delays in device change detection. Consider using a longer sleep duration or implementing event-driven notification.
Suggested change
| struct timespec ts = {0, 10000000}; // 10ms | |
| struct timespec ts = {0, 100000000}; // 100ms |
mhduiy
reviewed
Sep 25, 2025
| // 设置退出标志，通知子线程退出 | ||
| _thrd_exit_flag = 1; | ||
| // 主线程等待子线程退出 | ||
| pthread_join(_thrd, NULL); |
Contributor
There was a problem hiding this comment.
这里的join是不是可以去掉，不然和上面start的join死锁
Suggested change
| pthread_join(_thrd, NULL); |
robertkill
force-pushed
the
master
branch
2 times, most recently
from
6d93987 to
e9477e4
Compare
mhduiy
previously approved these changes
Sep 25, 2025
fly602
requested changes
Sep 25, 2025
inputdevices1/listen.c
Outdated
| XFreeEventData(_disp, cookie); | ||
| } else { | ||
| // 没有事件时休眠，避免 CPU 占用 | ||
| struct timespec ts = {0, 10000000}; // 10ms |
Contributor
There was a problem hiding this comment.
10ms轮询会一直消耗CPU，方案不太好。
当前修改没有造成崩溃是因为自测时，X11退出时刚好处在这个sleep中，没有在上面的逻辑中。
建议在斟酌一下方案。
Contributor
|
TAG Bot New tag: 6.1.57 |
mhduiy
reviewed
Sep 28, 2025
Changed thread creation from detached to joinable to allow proper thread synchronization and cleanup. Added proper exit mechanism using a flag instead of pthread_cancel. Improved event handling by using XPending to avoid blocking and added sleep when no events are available to reduce CPU usage. The previous implementation used detached threads which made proper cleanup difficult. The new joinable thread approach allows the main thread to wait for the listener thread to exit cleanly. Replaced pthread_cancel with a flag-based exit mechanism for safer thread termination. The event loop now checks for pending events before blocking, and sleeps when idle to reduce CPU consumption. Influence: 1. Test device listener startup and shutdown sequences 2. Verify thread properly exits when end_device_listener is called 3. Check CPU usage during idle periods 4. Test device change detection functionality 5. Verify no memory leaks during thread lifecycle refactor: 改进设备监听器线程管理 将线程创建从分离状态改为可连接状态，以实现正确的线程同步和清理。使用标志 位替代pthread_cancel实现正确的退出机制。通过使用XPending避免阻塞改进事件 处理，并在无事件时添加休眠以减少CPU占用。 之前的实现使用分离线程使得正确清理变得困难。新的可连接线程方法允许主线程 等待监听器线程干净退出。使用基于标志位的退出机制替代pthread_cancel以实现 更安全的线程终止。事件循环现在在阻塞前检查待处理事件，并在空闲时休眠以减 少CPU消耗。 Influence: 1. 测试设备监听器的启动和关闭序列 2. 验证调用end_device_listener时线程正确退出 3. 检查空闲期间的CPU使用率 4. 测试设备变更检测功能 5. 验证线程生命周期中无内存泄漏 PMS: BUG-335529
mhduiy
approved these changes
Sep 28, 2025
fly602
approved these changes
Sep 28, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fly602, mhduiy, robertkill 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changed thread creation from detached to joinable to allow proper thread synchronization and cleanup. Added proper exit mechanism using a flag instead of pthread_cancel. Improved event handling by using XPending to avoid blocking and added sleep when no events are available to reduce CPU usage.
The previous implementation used detached threads which made proper cleanup difficult. The new joinable thread approach allows the main thread to wait for the listener thread to exit cleanly. Replaced pthread_cancel with a flag-based exit mechanism for safer thread termination. The event loop now checks for pending events before blocking, and sleeps when idle to reduce CPU consumption.
Influence:
refactor: 改进设备监听器线程管理
将线程创建从分离状态改为可连接状态，以实现正确的线程同步和清理。使用标志
位替代pthread_cancel实现正确的退出机制。通过使用XPending避免阻塞改进事件
处理，并在无事件时添加休眠以减少CPU占用。
之前的实现使用分离线程使得正确清理变得困难。新的可连接线程方法允许主线程
等待监听器线程干净退出。使用基于标志位的退出机制替代pthread_cancel以实现
更安全的线程终止。事件循环现在在阻塞前检查待处理事件，并在空闲时休眠以减
少CPU消耗。
Influence:
PMS: BUG-335529
Summary by Sourcery
Improve thread lifecycle management and event loop efficiency in the device listener
Enhancements: