-
Notifications
You must be signed in to change notification settings - Fork 3
refactor: replace Lock with Channel for coroutine safety in Swoole ad… #26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughReplaces a process-based Lock with a coroutine Channel as the synchronization primitive in the Swoole connection pool adapter. The Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Pools/Adapter/Swoole.php`:
- Around line 17-21: The inline comments above the Channel mutex initialization
($this->lock = new Channel(1); $this->lock->push(true);) contain typos and
unclear phrasing; update them to a clear, concise explanation (e.g., note that a
coroutine mutex is used because OS-level locks block all coroutines on the same
event loop, so we use a Channel-based mutex which is coroutine-safe and does not
block other coroutines). Replace "loop)s" with "loop)" and rewrite "coroutine is
coroutine safe" to something like "Channel-based mutex is coroutine-safe" while
keeping the explanation that lock() is process/OS-level and would block
coroutines.
- Line 74: The call to $this->lock->push(true, $timeout) in Swoole adapter can
fail on timeout and be ignored, causing a lost mutex token and permanent
deadlock; update the release logic (the push call on the Channel instance
referenced by $this->lock, used in the synchronized()/unlock flow) to either
perform a blocking push (use a -1 timeout) so it cannot fail, or check the
boolean return and throw/log/assert on failure so the failure is not silent—make
the change where $this->lock->push(...) is invoked (in the Swoole Pool/Adapter
synchronized/unlock code) to ensure the push cannot silently fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Pools/Adapter/Swoole.php`:
- Around line 74-75: Update the inline comment above the call to
$this->lock->push(true) in the Swoole adapter: correct the typo "guranteed" to
"guaranteed" and rewrite the sentence for clarity (e.g., "guaranteed to have
space so no timeout; otherwise there will be no token and results can deadlock"
-> "guaranteed to have space to avoid timeouts; otherwise no token will be
available and results may deadlock"). Ensure the improved comment remains
concise and directly precedes $this->lock->push(true).
…apter
Better concurrency as the current coroutine suspends and other coroutines keep functioning instead of getting frozen. So a coroutine suspends and gives a better yield of control
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.