fix: guard against empty frame data in Swoole onMessage handler#29
fix: guard against empty frame data in Swoole onMessage handler#29
Conversation
Skip invoking the message callback when frame data is empty or null, preventing downstream errors when consumers try to parse empty messages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis change adds a guard clause to the Swoole adapter's onMessage handler that returns early when a frame contains empty or null data, preventing the callback from being invoked in these cases. Corresponding unit tests are introduced to verify this behavior by mocking the Swoole adapter and Server, using reflection to access private properties, and simulating frame reception to confirm callback invocation patterns for both empty and valid data scenarios. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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🧪 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 |
![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
🧹 Nitpick comments (1)
src/WebSocket/Adapter/Swoole.php (1)
111-115: Keep empty-message handling consistent across adapters.
Swoole::onMessage()now drops empty payloads, butsrc/WebSocket/Adapter/Workerman.php:122-135still forwards empty strings. That makes the shared adapter behavior depend on which backend is selected. Please either mirror this guard in Workerman or document the divergence explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WebSocket/Adapter/Swoole.php` around lines 111 - 115, The Swoole adapter's message handler now returns early for empty payloads (check using $frame->data === '' || $frame->data === null) causing inconsistent behavior with the Workerman adapter; update the Workerman adapter's onMessage handler (method/class symbol: Workerman::onMessage or its message callback) to perform the same guard and return early for empty string/null payloads before calling the shared callback, ensuring both adapters drop empty messages consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/SwooleAdapterTest.php`:
- Around line 12-54: The test testOnMessageSkipsEmptyData currently only covers
an empty-string payload; add an explicit null-payload regression for the same
behavior by reusing the registered callback invocation logic: create a Frame,
set $frame->fd = 1 and $frame->data = null, invoke
$registeredCallback($mockServer, $frame) and assert the $callbackInvoked flag
remains false. Ensure this new assertion sits alongside the existing
empty-string case so both branches of the production guard in Swoole::onMessage
are covered.
---
Nitpick comments:
In `@src/WebSocket/Adapter/Swoole.php`:
- Around line 111-115: The Swoole adapter's message handler now returns early
for empty payloads (check using $frame->data === '' || $frame->data === null)
causing inconsistent behavior with the Workerman adapter; update the Workerman
adapter's onMessage handler (method/class symbol: Workerman::onMessage or its
message callback) to perform the same guard and return early for empty
string/null payloads before calling the shared callback, ensuring both adapters
drop empty messages consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b21bb1d-f80f-4aed-8ff9-38c80f16789e
📒 Files selected for processing (2)
src/WebSocket/Adapter/Swoole.phptests/unit/SwooleAdapterTest.php
tests/unit/SwooleAdapterTest.php
Outdated
| public function testOnMessageSkipsEmptyData(): void | ||
| { | ||
| $adapter = $this->getMockBuilder(Swoole::class) | ||
| ->disableOriginalConstructor() | ||
| ->onlyMethods([]) | ||
| ->getMock(); | ||
|
|
||
| // Use reflection to access the server property and simulate the onMessage behavior | ||
| $reflection = new \ReflectionClass(Swoole::class); | ||
| $serverProperty = $reflection->getProperty('server'); | ||
| $serverProperty->setAccessible(true); | ||
|
|
||
| $mockServer = $this->getMockBuilder(Server::class) | ||
| ->disableOriginalConstructor() | ||
| ->getMock(); | ||
|
|
||
| // Capture the callback registered with $server->on('message', ...) | ||
| $registeredCallback = null; | ||
| $mockServer->expects($this->once()) | ||
| ->method('on') | ||
| ->with('message', $this->callback(function ($callback) use (&$registeredCallback) { | ||
| $registeredCallback = $callback; | ||
| return true; | ||
| })); | ||
|
|
||
| $serverProperty->setValue($adapter, $mockServer); | ||
|
|
||
| $callbackInvoked = false; | ||
| $adapter->onMessage(function (int $connection, string $message) use (&$callbackInvoked) { | ||
| $callbackInvoked = true; | ||
| }); | ||
|
|
||
| $this->assertNotNull($registeredCallback, 'Server on() should have been called'); | ||
|
|
||
| // Simulate a frame with empty data | ||
| $frame = new Frame(); | ||
| $frame->fd = 1; | ||
| $frame->data = ''; | ||
|
|
||
| $registeredCallback($mockServer, $frame); | ||
|
|
||
| $this->assertFalse($callbackInvoked, 'Callback should not be invoked for empty message data'); | ||
| } |
There was a problem hiding this comment.
Add a regression case for null frame data.
This suite only exercises the empty-string branch, but the production guard also skips null. Please add an explicit null payload case so both sides of the new condition are covered.
🧰 Tools
🪛 GitHub Actions: Tests
[error] 17-17: During: docker compose exec tests vendor/bin/phpunit --debug -> Error: Call to undefined function enum_exists(). This may indicate the PHP version or environment does not support enums.
🪛 PHPMD (2.15.0)
[warning] 40-40: Avoid unused parameters such as '$connection'. (undefined)
(UnusedFormalParameter)
[warning] 40-40: Avoid unused parameters such as '$message'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/SwooleAdapterTest.php` around lines 12 - 54, The test
testOnMessageSkipsEmptyData currently only covers an empty-string payload; add
an explicit null-payload regression for the same behavior by reusing the
registered callback invocation logic: create a Frame, set $frame->fd = 1 and
$frame->data = null, invoke $registeredCallback($mockServer, $frame) and assert
the $callbackInvoked flag remains false. Ensure this new assertion sits
alongside the existing empty-string case so both branches of the production
guard in Swoole::onMessage are covered.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix Confidence: 82/100The fix correctly guards against empty/null frame data in the Swoole adapter's onMessage handler, which is the entry point shown in the stack trace. The error occurs when downstream code (realtime.php) parses empty data and passes null to getDocument(). By preventing the callback from being called with empty data, we prevent the TypeError. The fix is minimal and all CI checks pass. Confidence is not higher because the actual error occurs in the consumer (realtime.php in appwrite/server-ce) and there could be edge cases where valid but malformed (non-empty) data also triggers the issue. |
1 participant
Summary
$frame->datain the Swoole adapter'sonMessagehandler, skipping the callback invocation when there is no meaningful data to processTypeErrorwhen consumers (e.g., Appwrite realtime) try to parse empty WebSocket frame data, which results in null values being passed to typed parametersonMessagebehavior with both empty and valid frame dataContext
Fixes Sentry issue CLOUD-3JPZ (650 events).
The error occurs when Swoole receives a WebSocket frame with empty data (e.g., control frames or malformed messages). The
onMessagehandler passes this empty data to the callback, which then fails when trying to extract fields from the parsed message.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests