feat: workerstart and coroutines 2 #21
Conversation
WalkthroughThis pull request introduces a worker-start callback mechanism to the DNS server infrastructure. The changes add a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
Pull Request Overview
This PR adds worker start callback functionality and enables coroutine support for the DNS server, allowing initialization logic to run when worker processes start.
Key Changes:
- Adds
onWorkerStartcallback support across all adapter implementations (Swoole and Native) - Enables Swoole coroutines via
Runtime::enableCoroutine()in the Swoole adapter - Refactors callback properties with proper PHPStan type hints for better type safety
- Updates Dockerfile with uppercase
ASkeyword following Docker best practices
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/DNS/Adapter.php | Adds abstract onWorkerStart method to adapter interface with type hints |
| src/DNS/Adapter/Swoole.php | Implements onWorkerStart callback, enables coroutines, and stores packet callback as property |
| src/DNS/Adapter/Native.php | Implements onWorkerStart with array storage and invokes callbacks in start method |
| src/DNS/Server.php | Adds public onWorkerStart method that wraps adapter callback with server context |
| tests/resources/server.php | Demonstrates onWorkerStart usage with console logging |
| Dockerfile | Updates AS keyword to uppercase for Docker style consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/DNS/Adapter/Swoole.php (1)
26-36: Consider using anonymous parameter syntax.The implementation correctly wires Swoole's WorkerStart event to the adapter contract. However, the
$serverparameter is unused and could be replaced with an underscore to suppress static analysis warnings.Apply this diff to use anonymous parameter syntax:
- $this->server->on('WorkerStart', function ($server, $workerId) use ($callback) { + $this->server->on('WorkerStart', function ($_, $workerId) use ($callback) { \call_user_func($callback, $workerId); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Dockerfile(2 hunks)src/DNS/Adapter.php(1 hunks)src/DNS/Adapter/Native.php(4 hunks)src/DNS/Adapter/Swoole.php(3 hunks)src/DNS/Server.php(3 hunks)tests/resources/server.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/DNS/Adapter.php (3)
src/DNS/Adapter/Native.php (1)
onWorkerStart(44-47)src/DNS/Adapter/Swoole.php (1)
onWorkerStart(31-36)src/DNS/Server.php (1)
onWorkerStart(122-129)
src/DNS/Adapter/Swoole.php (3)
src/DNS/Adapter.php (3)
Adapter(5-34)onPacket(21-21)onWorkerStart(13-13)src/DNS/Server.php (3)
Server(60-308)onPacket(171-283)onWorkerStart(122-129)src/DNS/Adapter/Native.php (2)
onPacket(53-56)onWorkerStart(44-47)
tests/resources/server.php (4)
src/DNS/Adapter.php (1)
onWorkerStart(13-13)src/DNS/Adapter/Native.php (1)
onWorkerStart(44-47)src/DNS/Adapter/Swoole.php (1)
onWorkerStart(31-36)src/DNS/Server.php (2)
onWorkerStart(122-129)Server(60-308)
src/DNS/Server.php (3)
src/DNS/Adapter.php (2)
onWorkerStart(13-13)onPacket(21-21)src/DNS/Adapter/Native.php (2)
onWorkerStart(44-47)onPacket(53-56)src/DNS/Adapter/Swoole.php (2)
onWorkerStart(31-36)onPacket(42-58)
src/DNS/Adapter/Native.php (3)
src/DNS/Adapter.php (2)
onPacket(21-21)onWorkerStart(13-13)src/DNS/Adapter/Swoole.php (2)
onPacket(42-58)onWorkerStart(31-36)src/DNS/Server.php (2)
onPacket(171-283)onWorkerStart(122-129)
🪛 PHPMD (2.15.0)
src/DNS/Adapter/Swoole.php
33-33: Avoid unused parameters such as '$server'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (17)
Dockerfile (1)
1-1: ✓ Capitalization fixes for Docker best practices.The changes correctly standardize the
ASkeyword to uppercase in multi-stage build directives, which aligns with Docker conventions and improves consistency.Also applies to: 14-14
tests/resources/server.php (2)
5-5: LGTM!The import is correctly added to support the worker-start callback logging below.
57-59: LGTM!The worker-start callback correctly demonstrates the new feature and provides useful logging for debugging and monitoring worker lifecycle.
src/DNS/Server.php (3)
64-66: LGTM!The error handler collection property is properly typed and initialized, supporting the existing error handling infrastructure.
115-129: LGTM!The worker-start callback registration is well-designed, providing a clean API that delegates to the adapter while enriching the callback with the server instance. Method chaining and comprehensive type annotations enhance usability and type safety.
300-302: LGTM!The typed closure approach using first-class callable syntax enhances type safety and makes the callback signature explicit for static analysis.
src/DNS/Adapter.php (2)
7-13: LGTM!The abstract worker-start method establishes a clear contract with comprehensive type annotations, enabling adapters to implement worker lifecycle hooks consistently.
15-21: LGTM!The enhanced docblock makes the packet callback contract explicit with comprehensive type annotations, improving static analysis and developer understanding.
src/DNS/Adapter/Swoole.php (4)
5-5: LGTM!The Runtime import is correctly added to support coroutine enablement during server startup.
12-14: LGTM!The packet callback property is properly annotated with the expected callable signature, enabling deferred invocation within the Swoole event handler.
38-58: LGTM!The packet handler correctly stores and invokes the callback, with improved type annotations. The explicit empty string check is more precise than a generic empty check.
65-65: LGTM!Enabling coroutines before server startup is correctly placed and aligns with the PR objectives to enhance async capabilities.
src/DNS/Adapter/Native.php (5)
12-17: LGTM!The callback storage properties are well-typed and initialized correctly. Supporting multiple worker-start callbacks provides flexibility for the Native adapter.
38-47: LGTM!The implementation correctly supports multiple worker-start callbacks with proper type annotations.
49-56: LGTM!The packet callback storage with enhanced type annotations aligns with the adapter contract and enables deferred invocation.
67-69: LGTM!Worker-start callbacks are correctly invoked before entering the main loop. Using workerId 0 is appropriate for the single-worker Native adapter.
79-79: LGTM!The stored packet callback is correctly invoked with the expected parameters.
Summary by CodeRabbit
New Features
onWorkerStartcallback support for DNS servers, enabling custom logic execution when worker processes initialize.Bug Fixes
Improvements