Fix infinite loop in parseResponse() when fread() returns empty string on broken socket#291
Merged
terabytesoftw merged 4 commits intoyiisoft:masterfrom Mar 23, 2026
Conversation
When a Redis node dies mid-response (e.g. during cluster failover or node
replacement), fread() returns '' (empty string), not false. The bulk reply
parser only checked for === false, causing the while($length > 0) loop to
spin forever since $length never decreases (strlen('') === 0).
With dataTimeout set, each fread() call blocks for dataTimeout seconds
before returning '', making the loop effectively infinite. This causes
workers to hang permanently with no logs (since log flush happens at end
of iteration).
The fix adds || $block === '' to the existing fread() check, so a
SocketException is thrown and the retry/reconnect logic can take over.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
|
You must correct the tests. |
sendRawCommand is protected, so direct call goes through __call() magic. Use TestCase::invokeMethod() to access it via reflection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes a hang in the Redis bulk-reply parser when a socket breaks mid-response and fread() returns an empty string ('') instead of false, allowing existing reconnect/retry logic to run.
Changes:
- Update
Connection::parseResponse()to treatfread()returning''as a socket read failure. - Add a PHPUnit test intended to reproduce the partial bulk-reply scenario.
- Document the fix in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/Connection.php |
Prevents infinite loop by failing fast when fread() returns '' during bulk reply reads. |
tests/RedisConnectionTest.php |
Adds coverage for broken-socket mid bulk reply (currently has issues that need adjustment). |
CHANGELOG.md |
Notes the bugfix in the “under development” section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
fclose($server) breaks both read and write sides, causing fwrite() on the client to fail with "Failed to write to socket" before reaching the fread() path we want to test. Using stream_socket_shutdown($server, STREAM_SHUT_WR) closes only the write side so fwrite() on client still works, but fread() on client gets EOF after reading the partial bulk reply header. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #291 +/- ##
============================================
+ Coverage 80.88% 80.99% +0.11%
- Complexity 368 369 +1
============================================
Files 10 10
Lines 905 905
============================================
+ Hits 732 733 +1
+ Misses 173 172 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Use protocol 0 instead of STREAM_IPPROTO_IP for Unix socket pair - Remove placeholder @see issues/XXX link - Use expectExceptionMessageMatches() for partial match - Add PR yiisoft#291 reference to CHANGELOG entry - Fix PHPStan baseline pattern for sort() to work across PHPStan versions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
@terabytesoftw any updates? |
s1lver
approved these changes
Mar 23, 2026
terabytesoftw
approved these changes
Mar 23, 2026
Member
|
Thks 👍 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When a Redis node dies mid-response (e.g. during cluster failover or node replacement),
fread()returns''(empty string), notfalse.The bulk reply parser in
parseResponse()only checks for=== false:This causes an infinite loop because:
fread()returns''on a broken/timed-out socket (notfalse)mb_strlen('', '8bit')is0, so$lengthnever decreaseswhile ($length > 0)loop never exitsdataTimeoutset, eachfread()blocks for N seconds before returning''Impact
In long-running workers (queue consumers, cron jobs), this causes the process to hang permanently. The retry/reconnect logic in
executeCommand()never gets a chance to run because we're stuck insidesendRawCommand()→parseResponse().Logs are also never flushed (since flush typically happens at end of iteration), making the issue very hard to diagnose.
Reproduction scenario
$200\r\n) then the node diesfgets()reads the header successfully (small, fits in TCP buffer)fread()tries to read the 202-byte body → broken socket → returns''Fix
This allows
SocketExceptionto be thrown, which triggers the existing retry/reconnect logic inexecuteCommand().Test
Added
testParseResponseBulkReplyBrokenSocket— usesstream_socket_pair()to simulate a partial bulk reply (header sent, server closed before body), verifying thatSocketExceptionis thrown instead of hanging.