Feat- worker start callback#6
Conversation
WalkthroughThe changes introduce new optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Adapter
participant NativeAdapter
participant SwooleAdapter
participant SwooleServer
User->>Adapter: onWorkerStart(callback)
Adapter-->>User: (default: does nothing)
User->>NativeAdapter: onWorkerStart(callback)
NativeAdapter->>User: callback(server_resource, 0)
User->>SwooleAdapter: onWorkerStart(callback)
SwooleAdapter->>SwooleServer: on('WorkerStart', callback)
SwooleServer->>User: callback(server_instance, workerId)
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 phpcs (3.7.2)src/DNS/Adapter/Swoole.phpWarning: PHP Startup: Invalid date.timezone value 'UTC', using 'UTC' instead in Unknown on line 0 Fatal error: Uncaught Error: Class "Phar" not found in /usr/local/bin/phpcs:3 ✨ 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Dockerfile(2 hunks)docker-compose.yml(0 hunks)src/DNS/Adapter.php(1 hunks)src/DNS/Adapter/Native.php(1 hunks)src/DNS/Adapter/Swoole.php(2 hunks)src/DNS/Server.php(1 hunks)
💤 Files with no reviewable changes (1)
- docker-compose.yml
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/DNS/Server.php (3)
src/DNS/Adapter.php (1)
onWorkerStart(31-35)src/DNS/Adapter/Swoole.php (1)
onWorkerStart(41-47)src/DNS/Adapter/Native.php (1)
onWorkerStart(79-86)
src/DNS/Adapter.php (2)
src/DNS/Adapter/Swoole.php (1)
onWorkerStart(41-47)src/DNS/Adapter/Native.php (1)
onWorkerStart(79-86)
src/DNS/Adapter/Native.php (2)
src/DNS/Adapter.php (1)
onWorkerStart(31-35)src/DNS/Adapter/Swoole.php (1)
onWorkerStart(41-47)
src/DNS/Adapter/Swoole.php (3)
src/DNS/Adapter.php (1)
onWorkerStart(31-35)src/DNS/Adapter/Native.php (1)
onWorkerStart(79-86)src/DNS/Server.php (1)
Server(53-460)
🪛 PHPMD (2.15.0)
src/DNS/Server.php
158-158: Avoid using static access to class '\Utopia\CLI\Console' in method 'start'. (Clean Code Rules)
(StaticAccess)
src/DNS/Adapter/Native.php
83-85: The method onWorkerStart uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (Clean Code Rules)
(ElseExpression)
🪛 GitHub Actions: CodeQL
src/DNS/Adapter/Native.php
[error] 83-83: PHPStan: Else branch is unreachable because previous condition is always true.
src/DNS/Adapter/Swoole.php
[error] 13-13: PHPStan: Property Utopia\DNS\Adapter\Swoole::$workerStartCallback has no type specified.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Unit & E2E
🔇 Additional comments (5)
Dockerfile (1)
1-1: LGTM! Good Docker formatting improvement.Capitalizing the
ASkeyword in multi-stage builds follows Docker best practices.Also applies to: 14-14
src/DNS/Adapter.php (1)
25-35: LGTM! Well-designed extensibility pattern.The default empty implementation allows adapters to optionally override worker start behavior while maintaining backward compatibility.
src/DNS/Server.php (1)
157-159: LGTM! Useful worker start logging.The callback provides valuable diagnostic information for monitoring worker processes. The logged details (worker ID, PID, timestamp, memory usage) will be helpful for debugging and performance monitoring.
src/DNS/Adapter/Swoole.php (2)
36-47: LGTM! Proper Swoole event integration.The implementation correctly registers the callback with Swoole's 'WorkerStart' event and follows the expected callback signature pattern.
49-57: LGTM! Useful server accessor method.The
getServer()method provides necessary access to the internal Swoole server instance for external configuration and monitoring.
| protected Server $server; | ||
| protected string $host; | ||
| protected int $port; | ||
| protected $workerStartCallback = null; |
There was a problem hiding this comment.
Add type annotation for the property.
The $workerStartCallback property lacks type specification, causing static analysis failures.
Apply this diff to fix the type annotation:
-protected $workerStartCallback = null;
+protected ?callable $workerStartCallback = null;🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 13-13: PHPStan: Property Utopia\DNS\Adapter\Swoole::$workerStartCallback has no type specified.
🤖 Prompt for AI Agents
In src/DNS/Adapter/Swoole.php at line 13, the property $workerStartCallback
lacks a type annotation, causing static analysis issues. Add an appropriate type
declaration to the property, such as specifying it as a callable or nullable
callable, to ensure proper type checking and resolve the static analysis
failures.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/DNS/Adapter/Swoole.php (1)
48-56: Consider the trade-offs of exposing the server instance.While this method provides flexibility for advanced server configuration, it breaks encapsulation by exposing internal state. Consider whether the intended use cases could be addressed through more specific configuration methods instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/DNS/Adapter/Swoole.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/DNS/Adapter/Swoole.php (3)
src/DNS/Adapter.php (2)
onWorkerStart(31-35)start(17-17)src/DNS/Adapter/Native.php (2)
onWorkerStart(79-82)start(40-61)src/DNS/Server.php (2)
Server(53-456)start(142-320)
🪛 PHPMD (2.15.0)
src/DNS/Adapter/Swoole.php
63-63: Avoid using static access to class '\Swoole\Runtime' in method 'start'. (Clean Code Rules)
(StaticAccess)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Unit & E2E
🔇 Additional comments (3)
src/DNS/Adapter/Swoole.php (3)
5-5: LGTM! Import statement correctly added.The
Swoole\Runtimeimport is necessary for the coroutine enablement in thestart()method.
36-46: Well-implemented worker start callback registration.The implementation correctly registers the callback with Swoole's
WorkerStartevent system, which will trigger when workers actually start. This differs from the Native adapter's immediate execution but is the appropriate approach for Swoole's event-driven architecture.
63-63: Coroutine enablement is correctly implemented.The
Runtime::enableCoroutine()call is the standard and documented way to enable Swoole's coroutine support. The static analysis warning about static access is a false positive in this context.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/DNS/Adapter/Swoole.php(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/DNS/Adapter/Swoole.php (3)
src/DNS/Adapter/Native.php (3)
__construct(20-30)onWorkerStart(79-82)start(40-61)src/DNS/Server.php (3)
__construct(70-75)Server(53-456)start(142-320)src/DNS/Adapter.php (2)
onWorkerStart(31-35)start(17-17)
🪛 PHPMD (2.15.0)
src/DNS/Adapter/Swoole.php
145-145: Avoid using static access to class '\Swoole\Runtime' in method 'start'. (Clean Code Rules)
(StaticAccess)
🪛 GitHub Actions: CodeQL
src/DNS/Adapter/Swoole.php
[error] 14-14: PHPStan: Property Utopia\DNS\Adapter\Swoole::$settings type has no value type specified in iterable type array.
[error] 39-39: PHPStan: Method Utopia\DNS\Adapter\Swoole::setSettings() has parameter $settings with no value type specified in iterable type array.
[error] 63-63: PHPStan: Method Utopia\DNS\Adapter\Swoole::getSettings() return type has no value type specified in iterable type array.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Unit & E2E
🔇 Additional comments (5)
src/DNS/Adapter/Swoole.php (5)
5-5: LGTM!The
Swoole\Runtimeimport is necessary for enabling coroutine support.
21-31: Well-configured default settings!The default server configuration is appropriate for a DNS server:
- Worker calculation (2x CPU cores) is suitable for I/O-bound workloads
- High coroutine limit (10,000) enables good concurrency
- Worker restart after 10k requests helps prevent memory leaks
- Dispatch mode 2 is correct for UDP servers
115-125: Correct implementation of worker start callback!The method properly registers the callback with Swoole's 'WorkerStart' event and passes the expected parameters (server instance and worker ID).
127-136: Good addition for advanced configuration!Exposing the server instance allows users to configure additional Swoole-specific settings if needed.
140-147: Correct implementation of server startup!The method properly applies settings before starting the server and enables coroutine support as intended. The static analysis warning about
Runtime::enableCoroutine()can be safely ignored as this is the standard Swoole API for enabling coroutines.
| public function getSettings(): array | ||
| { | ||
| return $this->settings; | ||
| } |
There was a problem hiding this comment.
Add value type specification for the return type.
Apply this diff to fix the type specification:
+/**
+ * @return array<string, mixed>
+ */
public function getSettings(): array📝 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 getSettings(): array | |
| { | |
| return $this->settings; | |
| } | |
| /** | |
| * @return array<string, mixed> | |
| */ | |
| public function getSettings(): array | |
| { | |
| return $this->settings; | |
| } |
🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 63-63: PHPStan: Method Utopia\DNS\Adapter\Swoole::getSettings() return type has no value type specified in iterable type array.
🤖 Prompt for AI Agents
In src/DNS/Adapter/Swoole.php around lines 63 to 66, the getSettings method's
return type is specified as array but lacks a value type specification. Update
the return type declaration to specify the type of values contained in the
array, for example, array<string, mixed> or the appropriate value type based on
the settings property, to improve type safety and clarity.
| public function setSettings(array $settings): self | ||
| { | ||
| $this->settings = array_merge($this->settings, $settings); | ||
| return $this; | ||
| } |
There was a problem hiding this comment.
Add value type specification for the settings parameter.
Apply this diff to fix the type specification:
-public function setSettings(array $settings): self
+/**
+ * @param array<string, mixed> $settings
+ */
+public function setSettings(array $settings): self📝 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 setSettings(array $settings): self | |
| { | |
| $this->settings = array_merge($this->settings, $settings); | |
| return $this; | |
| } | |
| /** | |
| * @param array<string, mixed> $settings | |
| */ | |
| public function setSettings(array $settings): self | |
| { | |
| $this->settings = array_merge($this->settings, $settings); | |
| return $this; | |
| } |
🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 39-39: PHPStan: Method Utopia\DNS\Adapter\Swoole::setSettings() has parameter $settings with no value type specified in iterable type array.
🤖 Prompt for AI Agents
In src/DNS/Adapter/Swoole.php around lines 39 to 43, the setSettings method's
parameter $settings lacks a value type specification. Update the method
signature to specify the type of values expected in the $settings array, for
example, array<string, mixed> or the appropriate value type, to improve type
safety and clarity.
| protected Server $server; | ||
| protected string $host; | ||
| protected int $port; | ||
| protected array $settings = []; |
There was a problem hiding this comment.
Add value type specification for the array property.
The static analysis indicates that the array type lacks value type specification. Since the settings array contains mixed types (int, bool), specify the value type explicitly.
Apply this diff to fix the type specification:
-protected array $settings = [];
+/** @var array<string, mixed> */
+protected array $settings = [];📝 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.
| protected array $settings = []; | |
| /** @var array<string, mixed> */ | |
| protected array $settings = []; |
🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 14-14: PHPStan: Property Utopia\DNS\Adapter\Swoole::$settings type has no value type specified in iterable type array.
🤖 Prompt for AI Agents
In src/DNS/Adapter/Swoole.php at line 14, the property $settings is declared as
an array without specifying the value types, which causes static analysis
warnings. Update the property declaration to specify the value types explicitly
as int|bool, for example, declare it as protected array<string, int|bool>
$settings to indicate it is an associative array with string keys and values of
type int or bool.
|
#21 |
2 participants
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Chores