Modify patch to retry writes once if they fail, then throw if not

Seldaek · Nov 11, 2024 · d768636 · d768636
1 parent 6d4ba0e
commit d768636
Show file tree

Hide file tree

Showing 2 changed files with 85 additions and 46 deletions.
diff --git a/src/Monolog/Handler/StreamHandler.php b/src/Monolog/Handler/StreamHandler.php
@@ -45,6 +45,8 @@ class StreamHandler extends AbstractProcessingHandler
     protected $fileOpenMode;
     /** @var true|null */
     private $dirCreated = null;
+    /** @var bool */
+    private $retrying = false;
 
     /**
      * @param resource|string $stream         If a missing path can't be created, an UnexpectedValueException will be thrown on first write
@@ -168,40 +170,32 @@ protected function write(array $record): void
             flock($stream, LOCK_EX);
         }
 
-        $this->streamWrite($stream, $record);
-
-        if ($this->useLocking) {
-            flock($stream, LOCK_UN);
-        }
-    }
-
-    /**
-     * {@inheritDoc}
-     */
-    public function handle(array $record): bool
-    {
-        $result = parent::handle($record);
-
-        // close the resource if possible to reopen it after we are done writing
-        if ($this->url !== null && $this->url !== 'php://memory') {
-            $this->close();
+        $this->errorMessage = null;
+        set_error_handler(function (...$args) {
+            return $this->customErrorHandler(...$args);
+        });
+        try {
+            $this->streamWrite($stream, $record);
+        } finally {
+            restore_error_handler();
         }
+        if ($this->errorMessage !== null) {
+            $error = $this->errorMessage;
+            // close the resource if possible to reopen it, and retry the failed write
+            if (!$this->retrying && $this->url !== null && $this->url !== 'php://memory') {
+                $this->retrying = true;
+                $this->close();
+                $this->write($record);
+
+                return;
+            }
 
-        return $result;
-    }
-
-    /**
-     * {@inheritDoc}
-     */
-    public function handleBatch(array $records): void
-    {
-        foreach ($records as $record) {
-            parent::handle($record);
+            throw new \UnexpectedValueException('Writing to the log file failed: '.$error . Utils::getRecordMessageForException($record));
         }
 
-        // close the resource if possible to reopen it after we are done writing
-        if ($this->url !== null && $this->url !== 'php://memory') {
-            $this->close();
+        $this->retrying = false;
+        if ($this->useLocking) {
+            flock($stream, LOCK_UN);
         }
     }
 
@@ -219,7 +213,7 @@ protected function streamWrite($stream, array $record): void
 
     private function customErrorHandler(int $code, string $msg): bool
     {
-        $this->errorMessage = preg_replace('{^(fopen|mkdir)\(.*?\): }', '', $msg);
+        $this->errorMessage = preg_replace('{^(fopen|mkdir|fwrite)\(.*?\): }', '', $msg);
 
         return true;
     }

diff --git a/tests/Monolog/Handler/StreamHandlerTest.php b/tests/Monolog/Handler/StreamHandlerTest.php
@@ -55,25 +55,15 @@ public function testCloseKeepsExternalHandlersOpen()
     /**
      * @covers Monolog\Handler\StreamHandler::close
      */
-    public function testHandlerOwnedHandlesAreClosedAfterEachWrite()
+    public function testClose()
     {
-        $handler = new StreamHandler(__DIR__.'/test.log');
+        $handler = new StreamHandler('php://memory');
         $handler->handle($this->getRecord(Logger::WARNING, 'test'));
         $stream = $handler->getStream();
 
-        $this->assertFalse(is_resource($stream));
-    }
-
-    /**
-     * @covers Monolog\Handler\StreamHandler::close
-     */
-    public function testHandlerOwnedHandlesAreClosedAfterEachBatchWrite()
-    {
-        $handler = new StreamHandler(__DIR__.'/test.log');
-        $handler->handleBatch([$this->getRecord(Logger::WARNING, 'test')]);
-        $stream = $handler->getStream();
-
-        $this->assertFalse(is_resource($stream));
+        $this->assertTrue(\is_resource($stream));
+        $handler->close();
+        $this->assertFalse(\is_resource($stream));
     }
 
     /**
@@ -221,6 +211,61 @@ public function testWriteNonExistingFileResource()
         $handler->handle($this->getRecord());
     }
 
+    /**
+     * @covers Monolog\Handler\StreamHandler::write
+     */
+    public function testWriteErrorDuringWriteRetriesWithClose()
+    {
+        $handler = $this->getMockBuilder(StreamHandler::class)
+            ->onlyMethods(['streamWrite'])
+            ->setConstructorArgs(['file://'.sys_get_temp_dir().'/bar/'.rand(0, 10000).DIRECTORY_SEPARATOR.rand(0, 10000)])
+            ->getMock();
+
+        $refs = [];
+        $handler->expects($this->exactly(2))
+            ->method('streamWrite')
+            ->willReturnCallback(function ($stream) use (&$refs) {
+                $refs[] = $stream;
+                if (\count($refs) === 2) {
+                    self::assertNotSame($stream, $refs[0]);
+                }
+                if (\count($refs) === 1) {
+                    trigger_error('fwrite(): Write of 378 bytes failed with errno=32 Broken pipe', E_USER_ERROR);
+                }
+            });
+
+        $handler->handle($this->getRecord());
+        self::assertIsClosedResource($refs[0]);
+        self::assertIsResource($refs[1]);
+    }
+
+    /**
+     * @covers Monolog\Handler\StreamHandler::write
+     */
+    public function testWriteErrorDuringWriteRetriesButThrowsIfStillFails()
+    {
+        $handler = $this->getMockBuilder(StreamHandler::class)
+            ->onlyMethods(['streamWrite'])
+            ->setConstructorArgs(['file://'.sys_get_temp_dir().'/bar/'.rand(0, 10000).DIRECTORY_SEPARATOR.rand(0, 10000)])
+            ->getMock();
+
+        $refs = [];
+        $handler->expects($this->exactly(2))
+            ->method('streamWrite')
+            ->willReturnCallback(function ($stream) use (&$refs) {
+                $refs[] = $stream;
+                if (\count($refs) === 2) {
+                    self::assertNotSame($stream, $refs[0]);
+                }
+                trigger_error('fwrite(): Write of 378 bytes failed with errno=32 Broken pipe', E_USER_ERROR);
+            });
+
+        self::expectException(\UnexpectedValueException::class);
+        self::expectExceptionMessage('Writing to the log file failed: Write of 378 bytes failed with errno=32 Broken pipe
+The exception occurred while attempting to log: test');
+        $handler->handle($this->getRecord());
+    }
+
     /**
      * @covers Monolog\Handler\StreamHandler::__construct
      * @covers Monolog\Handler\StreamHandler::write