[BUGFIX] Close file handle only if unused in FileWriter 04/57204/8
authorSusanne Moog <susanne.moog@typo3.org>
Tue, 12 Jun 2018 14:19:57 +0000 (16:19 +0200)
committerBenni Mack <benni@typo3.org>
Thu, 14 Jun 2018 04:31:00 +0000 (06:31 +0200)
Keep track of open file handles for resources across instances and
close the handle only on destructing the last known instance using it.

Resolves: #85245
Releases: master
Change-Id: I896f630521136474f843a271b5bd7c752a2efdaf
Reviewed-on: https://review.typo3.org/57204
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
Reviewed-by: Benni Mack <benni@typo3.org>
Tested-by: Benni Mack <benni@typo3.org>
typo3/sysext/core/Classes/Log/Writer/FileWriter.php
typo3/sysext/core/Tests/Unit/Log/Writer/FileWriterTest.php

index 74100c2..4717487 100644 (file)
@@ -1,4 +1,5 @@
 <?php
+
 namespace TYPO3\CMS\Core\Log\Writer;
 
 /*
@@ -52,6 +53,18 @@ class FileWriter extends AbstractWriter
     protected static $logFileHandles = [];
 
     /**
+     * Keep track of used file handles by different fileWriter instances
+     * As the logger gets instantiated by class name but the resources
+     * are shared via the static $logFileHandles we need to track usage
+     * of file handles to avoid closing handles that are still needed
+     * by different instances. Only if the count is zero may the file
+     * handle be closed.
+     *
+     * @var array
+     */
+    protected static $logFileHandlesCount = [];
+
+    /**
      * Constructor, opens the log file handle
      *
      * @param array $options
@@ -71,7 +84,10 @@ class FileWriter extends AbstractWriter
      */
     public function __destruct()
     {
-        $this->closeLogFile();
+        self::$logFileHandlesCount[$this->logFile]--;
+        if (self::$logFileHandlesCount[$this->logFile] <= 0) {
+            $this->closeLogFile();
+        }
     }
 
     /**
@@ -88,7 +104,10 @@ class FileWriter extends AbstractWriter
         if (false === strpos($logFile, '://') && !PathUtility::isAbsolutePath($logFile)) {
             $logFile = GeneralUtility::getFileAbsFileName($logFile);
             if ($logFile === null) {
-                throw new InvalidLogWriterConfigurationException('Log file path "' . $relativeLogFile . '" is not valid!', 1444374805);
+                throw new InvalidLogWriterConfigurationException(
+                    'Log file path "' . $relativeLogFile . '" is not valid!',
+                    1444374805
+                );
             }
         }
         $this->logFile = $logFile;
@@ -153,6 +172,11 @@ class FileWriter extends AbstractWriter
      */
     protected function openLogFile()
     {
+        if (isset(self::$logFileHandlesCount[$this->logFile])) {
+            self::$logFileHandlesCount[$this->logFile]++;
+        } else {
+            self::$logFileHandlesCount[$this->logFile] = 1;
+        }
         if (isset(self::$logFileHandles[$this->logFile]) && is_resource(self::$logFileHandles[$this->logFile] ?? false)) {
             return;
         }
@@ -225,7 +249,6 @@ class FileWriter extends AbstractWriter
 
     /**
      * Returns the path to the default log file.
-     *
      * Uses the defaultLogFileTemplate and replaces the %s placeholder with a short MD5 hash
      * based on a static string and the current encryption key.
      *
@@ -235,4 +258,23 @@ class FileWriter extends AbstractWriter
     {
         return Environment::getVarPath() . sprintf($this->defaultLogFileTemplate, substr(GeneralUtility::hmac($this->defaultLogFileTemplate, 'defaultLogFile'), 0, 10));
     }
+
+    /**
+     * Allow serialization of logger - reinitialize log file on unserializing
+     */
+    public function __wakeup()
+    {
+        self::$logFileHandlesCount[$this->logFile]++;
+        $this->setLogFile($this->logFile ?: $this->getDefaultLogFileName());
+    }
+
+    /**
+     * Property 'logFile' should be kept
+     *
+     * @return array
+     */
+    public function __sleep(): array
+    {
+        return ['logFile'];
+    }
 }
index da1fc22..01e16a9 100644 (file)
@@ -197,4 +197,28 @@ class FileWriterTest extends UnitTestCase
         $firstWriter->setLogFile($this->getDefaultFileName($logFilePrefix));
         $secondWriter->setLogFile($this->getDefaultFileName($logFilePrefix));
     }
+
+    /**
+     * @test
+     */
+    public function fileHandleIsNotClosedIfSecondFileWriterIsStillUsingSameFile()
+    {
+        $this->setUpVfsStream();
+
+        $firstWriter = $this->getMockBuilder(FileWriter::class)
+            ->setMethods(['closeLogFile'])
+            ->getMock();
+        $secondWriter = $this->getMockBuilder(FileWriter::class)
+            ->setMethods(['closeLogFile'])
+            ->getMock();
+
+        $firstWriter->expects($this->never())->method('closeLogFile');
+        $secondWriter->expects($this->once())->method('closeLogFile');
+
+        $logFilePrefix = $this->getUniqueId('unique');
+        $firstWriter->setLogFile($this->getDefaultFileName($logFilePrefix));
+        $secondWriter->setLogFile($this->getDefaultFileName($logFilePrefix));
+        $firstWriter->__destruct();
+        $secondWriter->__destruct();
+    }
 }