From 37490865cc9eea65f7fbc028ad03bbe295d82397 Mon Sep 17 00:00:00 2001 From: Vladyslav Vildanov <117659936+vladvildanov@users.noreply.github.com> Date: Mon, 14 Sep 2026 21:32:59 +0300 Subject: [PATCH] Fixed Stream::write()/read() leaving a dead connection when a host error handler throws exception (#1726) --- CHANGELOG.md | 1 + src/Connection/Resource/Stream.php | 10 +- .../Predis/Connection/Resource/StreamTest.php | 147 ++++++++++++++++++ 3 files changed, 155 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7bd64c4..14790054 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ ### Fixed - Fixed RESP3 double parsing returning positive `INF` for `-inf` payloads (#1716) - Fixed `client_info` connection parameter being ignored (#1722) +- Fixed `Stream::write()`/`read()` leaving a dead connection when a host error handler throws exception (#1725) ## v3.6.0 (2026-08-14) ### Added diff --git a/src/Connection/Resource/Stream.php b/src/Connection/Resource/Stream.php index 36a504fd..1448dbd9 100644 --- a/src/Connection/Resource/Stream.php +++ b/src/Connection/Resource/Stream.php @@ -205,7 +205,10 @@ class Stream implements StreamInterface throw new RuntimeException('Cannot write to a non-writable stream'); } - $result = fwrite($this->stream, $string); + // Suppressed: some error handlers (Laravel, Symfony, Laminas) convert engine + // notices/warnings into thrown exceptions, which would otherwise bypass the + // return-value handling below and leave a dead connection looking "connected". + $result = @fwrite($this->stream, $string); if ($result === false || $result === 0) { $metadata = $this->getMetadata(); @@ -265,10 +268,11 @@ class Stream implements StreamInterface return ''; } + // Suppressed: see the note in write() above. if ($length === -1) { - $string = fgets($this->stream); + $string = @fgets($this->stream); } else { - $string = fread($this->stream, $length); + $string = @fread($this->stream, $length); } if (false === $string) { diff --git a/tests/Predis/Connection/Resource/StreamTest.php b/tests/Predis/Connection/Resource/StreamTest.php index 2cb0e36c..bf121498 100644 --- a/tests/Predis/Connection/Resource/StreamTest.php +++ b/tests/Predis/Connection/Resource/StreamTest.php @@ -12,6 +12,7 @@ namespace Predis\Connection\Resource; +use ErrorException; use InvalidArgumentException; use PHPUnit\Framework\TestCase; use RuntimeException; @@ -431,6 +432,99 @@ class StreamTest extends TestCase $stream->write(''); } + /** + * @return void + */ + public function testWriteSuppressesEngineWarningUnderThrowingErrorHandler(): void + { + $this->registerEngineWarningWrapper(); + $this->installThrowingErrorHandler(); + + $stream = new Stream(fopen('predis-test-engine-warning://x', 'r+')); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('Unable to write to stream'); + + $stream->write('data'); + } + + /** + * @return void + */ + public function testReadSuppressesEngineWarningUnderThrowingErrorHandler(): void + { + $this->registerEngineWarningWrapper(); + $this->installThrowingErrorHandler(); + + $stream = new Stream(fopen('predis-test-engine-warning://x', 'r+')); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('Unable to read from stream'); + + // Use the fgets() path (length = -1): PHP 7.2's fread() coerces a + // user stream wrapper's `false` return into an empty string instead + // of preserving it, which would make this assertion PHP-version + // dependent; fgets() doesn't have that quirk. + $stream->read(-1); + } + + /** + * Mimics error handlers installed by Laravel, Symfony and Laminas, which + * convert engine notices/warnings into thrown exceptions unless the call + * site suppressed them with `@` (see GH-1725). The runner's own ambient + * error_reporting() level is irrelevant to what we want to assert here, + * so it's pinned to a known, fully-enabled value for the duration of the + * test rather than trusted as-is. + * + * @return void + */ + private function installThrowingErrorHandler(): void + { + $this->originalErrorReporting = error_reporting(E_ALL); + + set_error_handler(static function ($level, $message, $file = '', $line = 0) { + if (error_reporting() & $level) { + throw new ErrorException($message, 0, $level, $file, $line); + } + + return false; + }); + + $this->registeredErrorHandler = true; + } + + /** + * @var bool + */ + private $registeredErrorHandler = false; + + /** + * @var int|null + */ + private $originalErrorReporting; + + /** + * @return void + */ + protected function tearDown(): void + { + if ($this->registeredErrorHandler) { + restore_error_handler(); + error_reporting($this->originalErrorReporting); + $this->registeredErrorHandler = false; + } + } + + /** + * @return void + */ + private function registerEngineWarningWrapper(): void + { + if (!in_array('predis-test-engine-warning', stream_get_wrappers(), true)) { + stream_wrapper_register('predis-test-engine-warning', EngineWarningStreamWrapperFixture::class); + } + } + public function writableModeProvider(): array { return [ @@ -478,3 +572,56 @@ class StreamTest extends TestCase ]; } } + +/** + * Stream wrapper fixture that raises an engine-style warning from + * stream_write()/stream_read(), used to verify that Stream::write()/read() + * suppress it instead of letting a host-installed error handler turn it + * into an uncaught exception (see GH-1725). + */ +class EngineWarningStreamWrapperFixture +{ + /** + * @var resource + */ + public $context; + + public function stream_open($path, $mode, $options, &$openedPath): bool + { + return true; + } + + /** + * @return int|bool + */ + public function stream_write(string $data) + { + trigger_error('fwrite(): synthetic broken pipe', E_USER_WARNING); + + return false; + } + + /** + * @return string|bool + */ + public function stream_read(int $count) + { + trigger_error('fread(): synthetic broken pipe', E_USER_WARNING); + + return false; + } + + public function stream_eof(): bool + { + return false; + } + + public function stream_stat() + { + return []; + } + + public function stream_close(): void + { + } +}