Revert "feat: add workerStop handling" #49
Conversation
WalkthroughThis update modifies event handling and job processing in the queue system. It removes callback storage and constant-based event names in the Swoole adapter, updates the AMQP broker's close logic, and enhances the server's job consumption loop with expanded hook execution and error handling. A new worker stop registration method is also introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Adapter
participant Broker
Client->>Server: start()
loop Forever
Server->>Broker: consume(callbacks)
Broker-->>Server: job
Server->>Server: run global/group init hooks
Server->>Server: execute job action
Server->>Server: run global/group shutdown hooks
Server->>Server: log success
alt Error occurs
Server->>Server: log error, set error resource
Server->>Server: run error hooks
end
end
Possibly related PRs
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/Queue/Adapter/Swoole.php (1)
43-50: Critical:workerStop()method is listening to the wrong eventThe
workerStop()method is registering a listener for'WorkerStart'instead of'WorkerStop'. This will cause worker stop callbacks to be triggered on worker start events instead of stop events.Apply this fix:
public function workerStop(callable $callback): self { - $this->pool->on('WorkerStart', function (Pool $pool, string $workerId) use ($callback) { + $this->pool->on('WorkerStop', function (Pool $pool, string $workerId) use ($callback) { call_user_func($callback, $workerId); }); return $this; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Queue/Adapter/Swoole.php(2 hunks)src/Queue/Broker/AMQP.php(0 hunks)src/Queue/Server.php(3 hunks)
💤 Files with no reviewable changes (1)
- src/Queue/Broker/AMQP.php
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Adapter Tests (SwooleRedisCluster)
- GitHub Check: Adapter Tests (Workerman)
- GitHub Check: Adapter Tests (Swoole)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (AMQP)
🔇 Additional comments (1)
src/Queue/Server.php (1)
220-287: Verify graceful shutdown behavior with infinite loopThe infinite
while (true)loop for continuous job consumption has no break condition. This could prevent graceful shutdown of workers when the server needs to stop.Consider adding a mechanism to break the loop during shutdown, such as checking a shutdown flag or responding to stop signals.
| public function workerStop(?callable $callback = null): self | ||
| { | ||
| try { | ||
| $this->adapter->workerStop(function (string $workerId) use ($callback) { | ||
| Console::success("[Worker] Worker {$workerId} is ready!"); | ||
| if (!is_null($callback)) { | ||
| call_user_func($callback); | ||
| } | ||
| }); | ||
| } catch (Throwable $error) { | ||
| self::setResource('error', fn () => $error); | ||
| foreach ($this->errorHooks as $hook) { | ||
| call_user_func_array($hook->getAction(), $this->getArguments($hook)); | ||
| } | ||
| } | ||
|
|
||
| return $this; | ||
| } |
There was a problem hiding this comment.
Incorrect log message in workerStop() method
The log message indicates the worker is "ready" when it should indicate the worker is stopping.
public function workerStop(?callable $callback = null): self
{
try {
$this->adapter->workerStop(function (string $workerId) use ($callback) {
- Console::success("[Worker] Worker {$workerId} is ready!");
+ Console::success("[Worker] Worker {$workerId} is stopping!");
if (!is_null($callback)) {
call_user_func($callback);
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function workerStop(?callable $callback = null): self | |
| { | |
| try { | |
| $this->adapter->workerStop(function (string $workerId) use ($callback) { | |
| Console::success("[Worker] Worker {$workerId} is ready!"); | |
| if (!is_null($callback)) { | |
| call_user_func($callback); | |
| } | |
| }); | |
| } catch (Throwable $error) { | |
| self::setResource('error', fn () => $error); | |
| foreach ($this->errorHooks as $hook) { | |
| call_user_func_array($hook->getAction(), $this->getArguments($hook)); | |
| } | |
| } | |
| return $this; | |
| } | |
| public function workerStop(?callable $callback = null): self | |
| { | |
| try { | |
| $this->adapter->workerStop(function (string $workerId) use ($callback) { | |
| Console::success("[Worker] Worker {$workerId} is stopping!"); | |
| if (!is_null($callback)) { | |
| call_user_func($callback); | |
| } | |
| }); | |
| } catch (Throwable $error) { | |
| self::setResource('error', fn () => $error); | |
| foreach ($this->errorHooks as $hook) { | |
| call_user_func_array($hook->getAction(), $this->getArguments($hook)); | |
| } | |
| } | |
| return $this; | |
| } |
🤖 Prompt for AI Agents
In src/Queue/Server.php around lines 327 to 344, the log message inside the
workerStop() method incorrectly states the worker is "ready" when it should
indicate the worker is stopping. Update the Console::success log message to
reflect that the worker with the given ID is stopping, not ready.
2 participants
Reverts #45
Summary by CodeRabbit
New Features
Improvements