Use Corouitne/Task id#431
Conversation
loongs-zhang
force-pushed
the
add-id
branch
3 times, most recently
from
4a6ddef to
bf88689
Compare
There was a problem hiding this comment.
Pull request overview
This PR aims to replace string-based coroutine/task identifiers with numeric IDs across the scheduler, coroutine pool, and event-loop APIs (including the C hook layer), reducing string handling and avoiding name lifetime/leak patterns.
Changes:
- Switch coroutine cancellation/resumption and scheduler result keys from coroutine name (
&str/String) tou64coroutine IDs. - Switch task submission/join/cancellation from task name strings to
u64task IDs (includingJoinHandleand the C hook bindings). - Minor selector registration fix (
Result::or_else) and coroutine-pool stop/emptiness logic adjustments.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| hook/src/lib.rs | Updates C-facing cancellation to use JoinHandle task IDs. |
| core/tests/scheduler.rs | Updates scheduler cancel test to use coroutine IDs. |
| core/tests/co_pool.rs | Updates pool cancel test to use task IDs. |
| core/src/scheduler.rs | Converts internal syscall/cancel tracking and schedule results to coroutine IDs. |
| core/src/net/selector/mod.rs | Makes register fallback lazy via or_else to avoid eager evaluation. |
| core/src/net/mod.rs | Plumbs task ID through EventLoops::{submit_task, try_cancel_task} and JoinHandle. |
| core/src/net/join.rs | Changes JoinHandle storage/validation from C string name to u64 task ID. |
| core/src/net/event_loop.rs | Changes token derivation and resume path to use coroutine IDs / hashing. |
| core/src/coroutine/mod.rs | Exposes a coroutine id() accessor. |
| core/src/coroutine/korosensei.rs | Adds id: u64 to coroutine and initializes it from a hash of the name. |
| core/src/co_pool/task.rs | Adds id: u64 to tasks; renames get_name() to name() and adds id(). |
| core/src/co_pool/mod.rs | Converts pool tracking/results/waits/cancel structures to task IDs; tweaks stop loop and is_empty(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub(crate) fn err(pool: &'static Arc<EventLoop<'static>>) -> Self { | ||
| Self::new(pool, "") | ||
| Self::new(pool, 0) | ||
| } | ||
|
|
||
| /// create `JoinHandle` instance. | ||
| pub(crate) fn new(pool: &'static Arc<EventLoop<'static>>, name: &str) -> Self { | ||
| let boxed: &'static mut CString = Box::leak(Box::from( | ||
| CString::new(name).expect("init JoinHandle failed!"), | ||
| )); | ||
| let cstr: &'static CStr = boxed.as_c_str(); | ||
| JoinHandle(pool, cstr.as_ptr()) | ||
| pub(crate) fn new(pool: &'static Arc<EventLoop<'static>>, task_id: u64) -> Self { | ||
| JoinHandle(pool, task_id) | ||
| } | ||
|
|
||
| /// get the task name. | ||
| /// get the task id. | ||
| /// | ||
| /// # Errors | ||
| /// if the task name is invalid. | ||
| pub fn get_name(&self) -> std::io::Result<&str> { | ||
| unsafe { CStr::from_ptr(self.1) } | ||
| .to_str() | ||
| .map_err(|_| Error::new(ErrorKind::InvalidInput, "Invalid task name")) | ||
| /// if the task id is invalid. | ||
| pub fn id(&self) -> std::io::Result<u64> { | ||
| if 0 == self.1 { | ||
| return Err(Error::new(ErrorKind::InvalidInput, "Invalid task id")); | ||
| } | ||
| Ok(self.1) |
There was a problem hiding this comment.
JoinHandle treats task_id == 0 as invalid, but task IDs are now generated via hashing, which can legitimately produce 0. This would make a valid task un-joinable and prevent cleanup in Drop. Consider using Option<u64>/NonZeroU64 for the sentinel, or ensure generated task IDs are never 0 (e.g., remap 0 to another value).
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| let mut hasher = DefaultHasher::new(); | ||
| name.hash(&mut hasher); | ||
| let id = hasher.finish(); | ||
| Task { | ||
| id, | ||
| name, |
There was a problem hiding this comment.
Task IDs are derived from DefaultHasher(name), which is not collision-free. Because task_id is now used as the unique key in global maps (RUNNING_TASKS, CANCEL_TASKS, results, waits), a collision would cause incorrect cancellation/result delivery. Prefer generating IDs from a monotonic counter/UUID (or at least detect and resolve collisions when inserting into maps).
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| let mut hasher = DefaultHasher::new(); | ||
| name.hash(&mut hasher); | ||
| let id = hasher.finish(); | ||
| #[allow(unused_mut)] | ||
| let mut co = Coroutine { | ||
| id, | ||
| name, |
There was a problem hiding this comment.
Coroutine IDs are derived from DefaultHasher(name). Since co_id is used as the unique key for cancellation, syscall tables, and resumption, any hash collision would conflate two coroutines. Prefer a collision-free ID source (e.g., atomic counter or UUID bytes) instead of a hash, or add collision detection when inserting into syscall/RUNNING_COROUTINES/CANCEL_COROUTINES.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
|
@copilot go to work base on our conversation, fix the bugs I marked and need your feedback. |
// Describe your PR here; eg. Fixes #issueNo
Make sure that: