Conversation
waruqi
commented
Oct 4, 2025
- Before adding new features and new modules, please go to issues to submit the relevant feature description first.
- Write good commit messages and use the same coding conventions as the rest of the project.
- Please commit code to dev branch and we will merge into master branch in feature
- Ensure your edited codes with four spaces instead of TAB.
- 增加新特性和新模块之前，请先到issues提交相关特性说明，经过讨论评估确认后，再进行相应的代码提交，避免做无用工作。
- 编写友好可读的提交信息，并使用与工程代码相同的代码规范，代码请用4个空格字符代替tab缩进。
- 请提交代码到dev分支，如果通过，我们会在特定时间合并到master分支上。
- 为了规范化提交日志的格式，commit消息，不要用中文，请用英文描述。
Summary of ChangesHello @waruqi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust coroutine semaphore mechanism to the project's scheduler, enabling more sophisticated control over concurrent coroutine execution. This feature allows developers to manage access to shared resources or limit the number of active coroutines, thereby improving the handling of asynchronous operations and resource contention. The changes include the core semaphore implementation, its integration into the scheduler, and necessary sandbox adaptations, alongside new test cases to validate its functionality and performance. Highlights
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 a coroutine semaphore, a useful synchronization primitive for concurrent programming. The implementation is mostly solid, but I've found a few issues, including a critical memory leak in the wait function on timeout and a bug in __tostring. I've also suggested some improvements for clarity and efficiency in the post method and pointed out an issue in the sandboxed wrapper. Additionally, there's a minor issue in one of the new test files.
| if timeout then | ||
| break | ||
| end |
There was a problem hiding this comment.
If the wait operation times out, the coroutine is not removed from the self._WAITING set. This is a memory leak and can lead to incorrect behavior, as the coroutine will remain in the waiting queue. You should remove the coroutine from the waiting set when a timeout occurs.
if timeout then
self._WAITING:remove(running)
break
end
xmake/core/base/scheduler.lua
Outdated
|
|
||
| -- tostring(semaphore) | ||
| function _semaphore:__tostring() | ||
| return string.format("<sem: %s/%d>", self:name(), self:value()) |
There was a problem hiding this comment.
The __tostring metamethod calls self:value(), but this method is not defined for _semaphore. This will cause a runtime error. You should either implement the value() method or directly access self._VALUE.
return string.format("<sem: %s/%d>", self:name(), self._VALUE)
| function sandbox_core_base_scheduler_semaphore.post(semaphore, value) | ||
| local result, errors = semaphore:_post(value) | ||
| if result < 0 and errors then | ||
| raise(errors) | ||
| end | ||
| return result | ||
| end |
There was a problem hiding this comment.
The error handling for the wrapped post function appears to be incorrect. The original _semaphore:post function returns the new semaphore value and does not return an error tuple. Consequently, errors will always be nil, and the condition result < 0 and errors will never be true. This error check is likely dead code and should be removed.
function sandbox_core_base_scheduler_semaphore.post(semaphore, value)
return semaphore:_post(value)
end
| local value = semaphore:wait(-1) | ||
| print("[%d]: -> triggered, value: %d ..", id, value) | ||
| end | ||
| print("[%d]: end", id) |
There was a problem hiding this comment.
This print statement is unreachable because it is placed after an infinite while true loop. If the loop is intended to run forever, this line should be removed to avoid confusion.
| function _semaphore:post(value) | ||
| value = self._VALUE + value | ||
| self._VALUE = value | ||
| if value > 0 then | ||
| local pending = {} | ||
| local waiting = self._WAITING | ||
| for item in waiting:items() do | ||
| if #pending < value then | ||
| table.insert(pending, item) | ||
| end | ||
| end | ||
| for _, item in ipairs(pending) do | ||
| scheduler:co_resume(item) | ||
| end | ||
| end | ||
| return value | ||
| end |
There was a problem hiding this comment.
The implementation of post has a couple of areas for improvement:
- The
valuevariable is reused for both the input parameter and the new semaphore value, which can be confusing. Using a separate variable for the new value would improve clarity. - The loop that collects waiting coroutines iterates over all of them, even if enough have been found. It would be more efficient to break the loop once the required number of pending coroutines is reached.
function _semaphore:post(value_to_add)
local new_value = self._VALUE + value_to_add
self._VALUE = new_value
if new_value > 0 then
local pending = {}
local waiting = self._WAITING
for item in waiting:items() do
if #pending < new_value then
table.insert(pending, item)
else
break
end
end
for _, item in ipairs(pending) do
scheduler:co_resume(item)
end
end
return new_value
end