[add][thread]Add assertions for duplicate deletion across threads and warnings for incorrect usage. #10658
Conversation
📌 Code Review Assignment🏷️ Tag: kernelReviewers: GorrayLi ReviewSun hamburger-os lianux-mm wdfk-prog xu18838022837 Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2025-09-03 09:59 CST)
📝 Review Instructions
|
Rbb666
force-pushed
the
th_del
branch
2 times, most recently
from
d30aeaf to
1e0e726
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds safety checks to prevent duplicate thread deletion and warns about potentially unsafe thread deletion scenarios. The changes aim to improve thread lifecycle management robustness by detecting common programming errors.
- Added assertion to prevent duplicate deletion of already closed threads
- Added warning for deletion of active (running/ready) threads that may cause resource leaks
- Improved thread status checking before deletion operations
| rt_exit_critical_safe(critical_level); | ||
| LOG_W("Warning: Deleting active thread [%s] in %s state may cause resource leak", | ||
| thread->parent.name, | ||
| (thread_status == RT_THREAD_RUNNING) ? "RUNNING" : "READY"); | ||
| critical_level = rt_enter_critical(); |
There was a problem hiding this comment.
🟠 Major/主要: Critical section violation in warning path / 关键区域违规的警告路径
English: The code exits and re-enters the critical section just to print a warning message. This creates a potential race condition where the thread state could change between the exit and re-entry, making the warning message potentially inaccurate and violating real-time constraints.
中文：代码为了打印警告消息而退出并重新进入关键区域。这会产生潜在的竞争条件，线程状态可能在退出和重新进入之间发生变化，使警告消息可能不准确并违反实时约束。
Example/示例:
/* Move warning outside critical section */
rt_uint8_t thread_status;
rt_bool_t should_warn = RT_FALSE;
critical_level = rt_enter_critical();
thread_status = rt_sched_thread_get_stat(thread);
RT_ASSERT(thread_status != RT_THREAD_CLOSE);
if ((thread_status == RT_THREAD_RUNNING) || (thread_status == RT_THREAD_READY))
{
should_warn = RT_TRUE;
}
error = rt_thread_close(thread);
/* ... rest of critical section ... */
rt_exit_critical_safe(critical_level);
if (should_warn)
{
LOG_W(\"Warning: Deleting active thread [%s] may cause resource leak\",
thread->parent.name);
}
src/thread.c
Outdated
| /* assert thread is not already closed to prevent duplicate deletion */ | ||
| RT_ASSERT(thread_status != RT_THREAD_CLOSE); |
There was a problem hiding this comment.
🟡 Minor/次要: Assertion placement in critical section / 关键区域中的断言放置
English: While functionally correct, the assertion occurs within a critical section. In RT-Thread development, it's generally better to perform such checks before entering critical sections to minimize interrupt latency, unless the check itself requires atomic access to thread state.
中文：虽然功能上正确，但断言发生在关键区域内。在RT-Thread开发中，通常最好在进入关键区域之前执行此类检查以最小化中断延迟，除非检查本身需要原子访问线程状态。
Example/示例:
/* Consider moving basic validation outside critical section if possible */
RT_ASSERT(thread != RT_NULL);
critical_level = rt_enter_critical();
thread_status = rt_sched_thread_get_stat(thread);
RT_ASSERT(thread_status != RT_THREAD_CLOSE);
… warnings for incorrect usage.
| stat = rt_sched_thread_get_stat(thread); | ||
|
|
||
| /* assert thread is not already closed to prevent duplicate deletion */ | ||
| RT_ASSERT(stat != RT_THREAD_CLOSE); |
2 participants
拉取/合并请求描述：(PR description)
[
为什么提交这份PR (why to submit this PR)
你的解决方案是什么 (what is your solution)
请提供验证的bsp和config (provide the config and bsp)
]
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality：
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0代码，不包含已经被注释了的代码 All redundant code is removed and cleaned up