Fix mruby-task for PicoRuby Integration #6699
Conversation
With this PR, I can remove the original task.c in picoruby/picoruby and future development will be much easier. ## Add ### General - C API functions exported with MRB_API for external integration: - mrb_execute_proc_synchronously() for synchronous proc execution - Task control APIs (mrb_create_task, mrb_suspend_task, mrb_resume_task, mrb_terminate_task, mrb_stop_task, mrb_task_value, mrb_task_status) - Task context management APIs for picoruby-sandbox (mrb_task_init_context, mrb_task_reset_context, mrb_task_proc_set) - Task.tick class method to get current tick count - Comprehensive C API documentation with WASM integration examples ### For PicoRuby.wasm - WASM/Emscripten support: Disable SIGALRM timer when __EMSCRIPTEN__ is defined, as JavaScript handles tick calls via setInterval - Scheduler lock mechanism to prevent asynchronous task operations during synchronous execution (scheduler_lock counter in mrb_task_state) - mrb_task_run_once() for single-step execution (event loop integration) ## Fix ### task.c - Replace MRB_FIBER_TERMINATED with MRB_TASK_STOPPED just for clarity - Allow suspending DORMANT and WAITING tasks in mrb_task_suspend (See comment in the source) - Task context initialization by removing dummy callinfo push/pop *NOTE* With the dummy callinfo code that I deleted, IRB in PicoRuby ended SEGV. If that code is mandatory, we need to discuss how to solve my problem. ### vm.c - Handle MRB_TASK_CREATED status in VM's NORMAL_RETURN phase to properly stop tasks ---- These changes are necessary to make PicoRuby work. Nevertheless, even with this patch, MicroRuby for Raspberry Pi Pico 2 is still unstable. I would like to merge this PR anyway to make development easier by involving the PicoRuby community.
Summary of ChangesHello @hasumikin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will po 10000 st my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the mruby-task gem, primarily to facilitate its integration with PicoRuby, especially in WASM environments. It introduces a robust set of C APIs for direct task creation, control, and context management, alongside a new mechanism for synchronous execution that prevents race conditions. Key changes include tailored support for Emscripten/WASM by disabling conflicting timers and enabling single-step task execution, as well as crucial fixes to task lifecycle management and context initialization to improve stability and prevent segmentation faults. These updates aim to streamline PicoRuby development and broaden the applicability of mruby-task in embedded and web-based contexts. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to mruby-task for better integration with PicoRuby, particularly for WASM environments. The changes include adding a scheduler lock for synchronous execution, providing new C APIs for task management, and adapting the POSIX HAL for Emscripten. The code is well-structured, and the documentation updates are comprehensive. I have a few suggestions to improve consistency, safety, and maintainability.
And refactoring to consolidate duplicate code ref PR mruby#6699
6a4caf7 to
40eb5e9
Compare
And refactoring to consolidate duplicate code ref PR mruby#6699
40eb5e9 to
8a02630
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to mruby-task for better integration with PicoRuby, especially for WASM environments. The changes include a scheduler lock mechanism, conditional compilation for Emscripten, and an extensive new C API for task management. The code is generally well-structured, and the documentation updates are excellent. I've identified a few areas for improvement, mainly concerning potential memory leaks in the new C API, and some inconsistencies between the new C API and the existing Ruby API behavior. Addressing these points will improve the robustness and predictability of the new APIs.
|
(I'll check these comments and fix if necessary) |
Wrap sync task by mrb_gc_arena_save/restore to release objects from arena that a sync task allocated so that they can be freed in GC cycle
- Fix inconsistency of MRB_API functions and Ruby methods - Get rid of duplication - Adjust error handling
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is a substantial pull request that significantly enhances mruby-task for integration with PicoRuby, especially for WASM environments. The changes are extensive, including the addition of a scheduler lock for synchronous execution, new C APIs for finer-grained task control, and refactoring for better clarity and maintainability. The documentation has been updated comprehensively to reflect these new features. The removal of the dummy callinfo logic, coupled with the change in vm.c, is a critical fix for PicoRuby, and I appreciate the detailed explanation. The refactoring of task state management functions into internal and public API versions is a great improvement. I have a couple of suggestions for improving clarity and API robustness.
I am having trouble creating individual review comments. Click here to see my feedback.
mrbgems/mruby-task/src/task.c (1005-1008)
The check that prevented suspending tasks that are not READY or RUNNING has been removed. The PR description mentions this is intentional to allow suspending DORMANT and WAITING tasks. This is a significant change in behavior.
To make this clear for future maintainers, I suggest adding a comment to the new suspend_task_internal function explaining this new behavior and its rationale. For example:
/*
* Suspend a task. This can be called on tasks in any state (e.g., READY,
* RUNNING, WAITING, DORMANT), except for tasks that are already SUSPENDED.
* Suspending a WAITING task is useful to prevent it from becoming READY
* even if its wait condition (like a timer) is met. It will need to be
* resumed manually later.
*/mrbgems/mruby-task/src/task.c (1127)
The function mrb_execute_proc_synchronously accepts argc and argv but they are currently unused, which could be misleading for developers. If passing arguments is not intended to be supported at this time, it would be clearer to add a check to ensure argc is 0 and raise an E_ARGUMENT_ERROR if it's not. If it is a planned feature, a TODO comment would be helpful.
For example, you could add this at the beginning of the function:
if (argc > 0) {
mrb_raise(mrb, E_ARGUMENT_ERROR, "passing arguments to mrb_execute_proc_synchronously is not yet supported");
}- Add mrb_gc_protect() after arena_restore to prevent result from being collected before returning to caller - Add comment to suspend_task_internal explaining why WAITING and DORMANT tasks can also be suspended - Move argc/argv cast at the beginning of function with comment
|
@matz Could you review it again, please? |
With this PR, I can remove the original task.c in picoruby/picoruby and future development will be much easier.
Add to task.c
General purpose
For PicoRuby.wasm
Fix in task.c
Important:
With the dummy callinfo code that I deleted in
task_init_context()andexecute_task(), IRB in PicoRuby ended SEGV. If that code is mandatory, we need to discuss how to solve my problem.Fix in vm.c
(I suspect that we may have to separate MRB_TASK_CREATED and MRB_TASK_STOPPED from MRB_FIBER_CREATED and MRB_FIBER_TERMINATED instead of aliasing in mruby.h)
These changes are necessary to make PicoRuby work. Nevertheless, even with this patch, MicroRuby for Raspberry Pi Pico 2 is still unstable. I would like to merge this PR anyway to make development easier by involving the PicoRuby community.