-
Notifications
You must be signed in to change notification settings - Fork 3
Update swoole lock #27
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
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe PR removes the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. 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 |
| } | ||
| public function testInitOutsideCoroutineNotThrowAnyError() | ||
| { | ||
| $pool = new Pool(new Swoole(), 'test', 1, fn () => 'x'); |
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.
similar to the appwrite Realtime pubsub
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 13-18: The Swoole Coroutine Lock (Swoole\Coroutine\Lock) is used
in the Swoole.php class (property $lock and initialize method) but requires
Swoole >= 6.0.1; update composer.json to require "swoole": ">=6.0.1" or add a
runtime guard in the Swoole.php initialize() (and the class property usage) that
checks Swoole::VERSION or defined constant and either instantiates
Swoole\Coroutine\Lock only when the version is >= 6.0.1 or falls back to an
alternative (e.g., a noop mutex or a userland lock) and throw a clear error if
no compatible lock is available; reference the property $lock, the
initialize(int $size): static method and the Swoole\Coroutine\Lock symbol when
making changes.
🧹 Nitpick comments (3)
tests/Pools/Adapter/SwooleTest.php (1)
356-360: Good coverage for the initialization scenario.The test validates that
Poolinitialization with the Swoole adapter no longer throws "must be accessed inside coroutine" errors. Consider extending this test to verify that basic operations (like calling methods that usesynchronized()) also work outside a coroutine context, as that would more fully validate the fix.src/Pools/Pool.php (1)
171-190: Docblock now misleading:setSynchronizationTimeoutno longer affects lock acquisition.The docblock at lines 172-177 states this sets "the lock timeout for adapters that support synchronized locking." However, with this PR,
synchronized()no longer accepts a timeout parameter—the$synchronizedTimeoutvalue is only used forpop()operations (line 286). Consider updating the docblock to clarify the actual usage or renaming the property/methods to better reflect their purpose (e.g.,setPopTimeout).src/Pools/Adapter/Swoole.php (1)
48-71: Docblock inconsistency: timeout reference no longer applicable.Line 57 states "@throws \RuntimeException If the lock cannot be acquired within the timeout" but the method no longer accepts or uses a timeout parameter. The
Lock::lock()method blocks indefinitely until the lock is acquired (unless deadlock occurs).📝 Suggested docblock fix
/** * Executes a callback while holding a lock. * * The lock is acquired before invoking the callback and is always released * afterward, even if the callback throws an exception. * * `@param` callable $callback Callback to execute within the critical section. * `@return` mixed The value returned by the callback. * - * `@throws` \RuntimeException If the lock cannot be acquired within the timeout. + * `@throws` \RuntimeException If the lock cannot be acquired. */
Coroutine based lock creating problems when pool is init outside the coroutine leading to the errors like 'must be accessed inside coroutine'
Summary by CodeRabbit
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.