A substantial rewrite of the three file appenders:
- a file will not be created on activation, but on first append (LOG4PHP-130)
- compression in rolling appender is done in chunks (LOG4PHP-164)
- general improvements in writing logic, better error handling
git-svn-id: https://svn.apache.org/repos/asf/logging/log4php/trunk@1374786 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/pom.xml b/pom.xml
index 8f48d92..cfcaa0b 100644
--- a/pom.xml
+++ b/pom.xml
@@ -166,71 +166,7 @@
</dependencies>
<executions>
- <execution>
- <id>pear-package</id>
- <phase>package</phase>
- <configuration>
- <target>
- <ant target="pear-package" />
- </target>
- </configuration>
- <goals>
- <goal>run</goal>
- </goals>
- </execution>
-
- <execution>
- <id>pre-site</id>
- <phase>pre-site</phase>
- <configuration>
- <target>
- <ant target="google-search" />
- <ant target="api-docs" />
- </target>
- </configuration>
- <goals>
- <goal>run</goal>
- </goals>
- </execution>
-
- <execution>
- <id>unit-testing</id>
- <phase>test</phase>
- <configuration>
- <target>
- <ant target="unit-testing" />
- </target>
- </configuration>
- <goals>
- <goal>run</goal>
- </goals>
- </execution>
-
- <execution>
- <id>post-site</id>
- <phase>post-site</phase>
- <configuration>
- <target>
- <ant target="post-site" />
- </target>
- </configuration>
- <goals>
- <goal>run</goal>
- </goals>
- </execution>
-
- <execution>
- <id>site-deploy</id>
- <phase>site-deploy</phase>
- <configuration>
- <target>
- <ant target="site-deploy" />
- </target>
- </configuration>
- <goals>
- <goal>run</goal>
- </goals>
- </execution>
+
</executions>
</plugin>
@@ -267,7 +203,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-release-plugin</artifactId>
- <version>2.2.2</version>
+ <version>2.3.2</version>
<configuration>
<goals>site assembly:assembly</goals>
</configuration>
@@ -276,7 +212,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-site-plugin</artifactId>
- <version>3.0</version>
+ <version>3.1</version>
<configuration>
<!-- Override default site template for achieveing a custom look. -->
@@ -284,59 +220,6 @@
<!-- Configure additional reports on the site -->
<reportPlugins>
-
- <!-- Project info report -->
- <plugin>
- <groupId>org.apache.maven.plugins</groupId>
- <artifactId>maven-project-info-reports-plugin</artifactId>
- <version>2.4</version>
- <reportSets>
- <reportSet>
- <reports>
- <report>cim</report>
- <report>scm</report>
- <report>dependencies</report>
- <report>license</report>
- <report>project-team</report>
- <report>issue-tracking</report>
- <report>mailing-list</report>
- </reports>
- </reportSet>
- </reportSets>
- </plugin>
-
- <!-- Changes report -->
- <plugin>
- <groupId>org.apache.maven.plugins</groupId>
- <artifactId>maven-changes-plugin</artifactId>
- <version>2.6</version>
- <reportSets>
- <reportSet>
- <reports>
- <report>changes-report</report>
- <report>jira-report</report>
- </reports>
- </reportSet>
- </reportSets>
- <configuration>
- <statusIds>Resolved, Closed</statusIds>
- <columnNames>Type,Key,Summary,Assignee,Status,Resolution,Fix Version</columnNames>
- </configuration>
- </plugin>
-
- <!-- Surefire report -->
- <plugin>
- <groupId>org.apache.maven.plugins</groupId>
- <artifactId>maven-surefire-report-plugin</artifactId>
- <version>2.12</version>
- </plugin>
-
- <!-- RAT report -->
- <plugin>
- <groupId>org.apache.rat</groupId>
- <artifactId>apache-rat-plugin</artifactId>
- <version>0.8</version>
- </plugin>
</reportPlugins>
</configuration>
</plugin>
diff --git a/src/main/php/appenders/LoggerAppenderDailyFile.php b/src/main/php/appenders/LoggerAppenderDailyFile.php
index c12aeb2..9d59b05 100644
--- a/src/main/php/appenders/LoggerAppenderDailyFile.php
+++ b/src/main/php/appenders/LoggerAppenderDailyFile.php
@@ -48,42 +48,27 @@
* @var string
*/
protected $datePattern = "Ymd";
-
- /**
- * Similar to parent method, but but replaces "%s" in the file name with
- * the current date in format specified by the 'datePattern' parameter.
- */
+
+ /** Additional validation for the date pattern. */
public function activateOptions() {
- $fileName = $this->getFile();
- $date = date($this->getDatePattern());
- $fileName = sprintf($fileName, $date);
-
- if(!is_file($fileName)) {
- $dir = dirname($fileName);
- if(!is_dir($dir)) {
- mkdir($dir, 0777, true);
- }
- }
+ parent::activateOptions();
- $this->fp = fopen($fileName, ($this->getAppend()? 'a':'w'));
- if($this->fp) {
- if(flock($this->fp, LOCK_EX)) {
- if($this->getAppend()) {
- fseek($this->fp, 0, SEEK_END);
- }
- fwrite($this->fp, $this->layout->getHeader());
- flock($this->fp, LOCK_UN);
- $this->closed = false;
- } else {
- // TODO: should we take some action in this case?
- $this->closed = true;
- }
- } else {
+ if (empty($this->datePattern)) {
+ $this->warn("Required parameter 'datePattern' not set. Closing appender.");
$this->closed = true;
+ return;
}
}
/**
+ * Determines target file. Replaces %s in file path with a date.
+ */
+ protected function getTargetFile() {
+ $date = date($this->datePattern);
+ return str_replace('%s', $date, $this->file);
+ }
+
+ /**
* Sets the 'datePattern' parameter.
* @param string $datePattern
*/
diff --git a/src/main/php/appenders/LoggerAppenderFile.php b/src/main/php/appenders/LoggerAppenderFile.php
index c12b7b5..078212c 100644
--- a/src/main/php/appenders/LoggerAppenderFile.php
+++ b/src/main/php/appenders/LoggerAppenderFile.php
@@ -37,76 +37,134 @@
class LoggerAppenderFile extends LoggerAppender {
/**
- * @var boolean if {@link $file} exists, appends events.
+ * If set to true, the file is locked before appending. This allows
+ * concurrent access. However, appending without locking is faster so
+ * it should be used where appropriate.
+ *
+ * TODO: make this a configurable parameter
+ *
+ * @var boolean
+ */
+ protected $locking = true;
+
+ /**
+ * If set to true, appends to file. Otherwise overwrites it.
+ * @var boolean
*/
protected $append = true;
/**
- * @var string the file name used to append events
+ * Path to the target file.
+ * @var string
*/
protected $file;
/**
- * @var mixed file resource
+ * The file resource.
+ * @var resource
*/
- protected $fp = false;
+ protected $fp;
- public function activateOptions() {
- $fileName = $this->getFile();
+ /**
+ * Helper function which can be easily overriden by daily file appender.
+ */
+ protected function getTargetFile() {
+ return $this->file;
+ }
+
+ /**
+ * Acquires the target file resource, creates the destination folder if
+ * necessary. Writes layout header to file.
+ */
+ protected function openFile() {
+ $file = $this->getTargetFile();
- if (empty($fileName)) {
- $this->warn("Required parameter 'fileName' not set. Closing appender.");
+ // Create the target folder if needed
+ if(!is_file($file)) {
+ $dir = dirname($file);
+
+ if(!is_dir($dir)) {
+ $success = mkdir($dir, 0777, true);
+ if ($success === false) {
+ $this->warn("Failed creating target directory [$dir]. Closing appender.");
+ $this->closed = true;
+ return;
+ }
+ }
+ }
+
+ $mode = $this->append ? 'a' : 'w';
+ $this->fp = fopen($file, $mode);
+ if ($this->fp === false) {
+ $this->warn("Failed opening target file. Closing appender.");
$this->closed = true;
return;
}
- if(!is_file($fileName)) {
- $dir = dirname($fileName);
- if(!is_dir($dir)) {
- mkdir($dir, 0777, true);
- }
+ // Required when appending with concurrent access
+ if($this->append) {
+ fseek($this->fp, 0, SEEK_END);
}
-
- $this->fp = fopen($fileName, ($this->getAppend()? 'a':'w'));
- if($this->fp) {
- if(flock($this->fp, LOCK_EX)) {
- if($this->getAppend()) {
- fseek($this->fp, 0, SEEK_END);
- }
- fwrite($this->fp, $this->layout->getHeader());
- flock($this->fp, LOCK_UN);
- $this->closed = false;
- } else {
- // TODO: should we take some action in this case?
- $this->closed = true;
- }
+
+ // Write the header
+ $this->write($this->layout->getHeader());
+ }
+
+ /**
+ * Writes a string to the target file. Opens file if not already open.
+ * @param string $string Data to write.
+ */
+ protected function write($string) {
+ // Lazy file open
+ if(!isset($this->fp)) {
+ $this->openFile();
+ }
+
+ if ($this->locking) {
+ $this->writeWithLocking($string);
} else {
+ $this->writeWithoutLocking($string);
+ }
+ }
+
+ protected function writeWithLocking($string) {
+ if(flock($this->fp, LOCK_EX)) {
+ if(fwrite($this->fp, $string) === false) {
+ $this->warn("Failed writing to file. Closing appender.");
+ $this->closed = true;
+ }
+ flock($this->fp, LOCK_UN);
+ } else {
+ $this->warn("Failed locking file for writing. Closing appender.");
$this->closed = true;
}
}
- public function close() {
- if($this->closed != true) {
- if($this->fp and $this->layout !== null) {
- if(flock($this->fp, LOCK_EX)) {
- fwrite($this->fp, $this->layout->getFooter());
- flock($this->fp, LOCK_UN);
- }
- fclose($this->fp);
- }
- $this->closed = true;
+ protected function writeWithoutLocking($string) {
+ if(fwrite($this->fp, $string) === false) {
+ $this->warn("Failed writing to file. Closing appender.");
+ $this->closed = true;
}
}
+
+ public function activateOptions() {
+ if (empty($this->file)) {
+ $this->warn("Required parameter 'file' not set. Closing appender.");
+ $this->closed = true;
+ return;
+ }
+ }
+
+ public function close() {
+ if (is_resource($this->fp)) {
+ $this->write($this->layout->getFooter());
+ fclose($this->fp);
+ }
+ $this->closed = true;
+ }
public function append(LoggerLoggingEvent $event) {
- if($this->fp and $this->layout !== null) {
- if(flock($this->fp, LOCK_EX)) {
- fwrite($this->fp, $this->layout->format($event));
- flock($this->fp, LOCK_UN);
- } else {
- $this->closed = true;
- }
- }
+ $this->write($this->layout->format($event));
}
/**
diff --git a/src/main/php/appenders/LoggerAppenderRollingFile.php b/src/main/php/appenders/LoggerAppenderRollingFile.php
index 59296ad..f3d06ce 100644
--- a/src/main/php/appenders/LoggerAppenderRollingFile.php
+++ b/src/main/php/appenders/LoggerAppenderRollingFile.php
@@ -44,19 +44,15 @@
*/
class LoggerAppenderRollingFile extends LoggerAppenderFile {
+ /** Compressing backup files is done in chunks, this determines how large. */
+ const COMPRESS_CHUNK_SIZE = 102400; // 100KB
+
/**
- * Set the maximum size that the output file is allowed to reach
+ * The maximum size (in bytes) that the output file is allowed to reach
* before being rolled over to backup files.
*
- * <p>In configuration files, the <var>MaxFileSize</var> option takes a
- * long integer in the range 0 - 2^63. You can specify the value
- * with the suffixes "KB", "MB" or "GB" so that the integer is
- * interpreted being expressed respectively in kilobytes, megabytes
- * or gigabytes. For example, the value "10KB" will be interpreted
- * as 10240.</p>
- * <p>The default maximum file size is 10MB.</p>
- *
- * <p>Note that MaxFileSize cannot exceed <b>2 GB</b>.</p>
+ * The default maximum file size is 10MB (10485760 bytes). Maximum value
+ * for this option may depend on the file system.
*
* @var integer
*/
@@ -65,36 +61,23 @@
/**
* Set the maximum number of backup files to keep around.
*
- * <p>The <var>MaxBackupIndex</var> option determines how many backup
- * files are kept before the oldest is erased. This option takes
- * a positive integer value. If set to zero, then there will be no
- * backup files and the log file will be truncated when it reaches
- * MaxFileSize.</p>
- * <p>There is one backup file by default.</p>
+ * Determines how many backup files are kept before the oldest is erased.
+ * This option takes a positive integer value. If set to zero, then there
+ * will be no backup files and the log file will be truncated when it
+ * reaches <var>maxFileSize</var>.
+ *
+ * There is one backup file by default.
*
* @var integer
*/
protected $maxBackupIndex = 1;
/**
- * @var string the filename expanded
- */
- private $expandedFileName = null;
-
- /**
* The <var>compress</var> parameter determindes the compression with zlib.
* If set to true, the rollover files are compressed and saved with the .gz extension.
* @var boolean
*/
protected $compress = false;
-
- /**
- * Returns the value of the MaxBackupIndex option.
- * @return integer
- */
- private function getExpandedFileName() {
- return $this->expandedFileName;
- }
/**
* Get the maximum size that the output file is allowed to reach
@@ -108,33 +91,30 @@
/**
* Implements the usual roll over behaviour.
*
- * <p>If MaxBackupIndex is positive, then files File.1, ..., File.MaxBackupIndex -1 are renamed to File.2, ..., File.MaxBackupIndex.
+ * If MaxBackupIndex is positive, then files File.1, ..., File.MaxBackupIndex -1 are renamed to File.2, ..., File.MaxBackupIndex.
* Moreover, File is renamed File.1 and closed. A new File is created to receive further log output.
*
- * <p>If MaxBackupIndex is equal to zero, then the File is truncated with no backup files created.
+ * If MaxBackupIndex is equal to zero, then the File is truncated with no backup files created.
*
* Rollover must be called while the file is locked so that it is safe for concurrent access.
+ *
+ * @throws LoggerException If any part of the rollover procedure fails.
*/
private function rollOver() {
// If maxBackups <= 0, then there is no file renaming to be done.
if($this->maxBackupIndex > 0) {
- $fileName = $this->getExpandedFileName();
-
// Delete the oldest file, to keep Windows happy.
- $file = $fileName . '.' . $this->maxBackupIndex;
- if(is_writable($file)) {
- unlink($file);
+ $file = $this->file . '.' . $this->maxBackupIndex;
+
+ if (file_exists($file) && !unlink($file)) {
+ throw new LoggerException("Unable to delete oldest backup file from [$file].");
}
// Map {(maxBackupIndex - 1), ..., 2, 1} to {maxBackupIndex, ..., 3, 2}
- $this->renameArchievedLogs($fileName);
+ $this->renameArchievedLogs($this->file);
- if (true === $this->compress) {
- file_put_contents('compress.zlib://'.$fileName.'.1.gz', file_get_contents($fileName));
- } else {
- // Backup the active file
- copy($fileName, "$fileName.1");
- }
+ // Backup the active file
+ $this->moveToBackup($this->file);
}
// Truncate the active file
@@ -142,81 +122,116 @@
rewind($this->fp);
}
+ private function moveToBackup($source) {
+ if ($this->compress) {
+ $target = $source . '.1.gz';
+ $this->compressFile($source, $target);
+ } else {
+ $target = $source . '.1';
+ copy($source, $target);
+ }
+ }
+
+ private function compressFile($source, $target) {
+ $target = 'compress.zlib://' . $target;
+
+ $fin = fopen($source, 'rb');
+ if ($fin === false) {
+ throw new LoggerException("Unable to open file for reading: [$source].");
+ }
+
+ $fout = fopen($target, 'wb');
+ if ($fout === false) {
+ throw new LoggerException("Unable to open file for writing: [$target].");
+ }
+
+ while (!feof($fin)) {
+ $chunk = fread($fin, self::COMPRESS_CHUNK_SIZE);
+ if (false === fwrite($fout, $chunk)) {
+ throw new LoggerException("Failed writing to compressed file.");
+ }
+ }
+
+ fclose($fin);
+ fclose($fout);
+ }
+
private function renameArchievedLogs($fileName) {
for($i = $this->maxBackupIndex - 1; $i >= 1; $i--) {
- $file = $fileName . "." . $i;
- if (true === $this->compress) {
- $file = $fileName . "." . $i .'.gz';
+ $source = $fileName . "." . $i;
+ if ($this->compress) {
+ $source .= '.gz';
}
- if(is_readable($file)) {
+ if(file_exists($source)) {
$target = $fileName . '.' . ($i + 1);
- if (true === $this->compress) {
- $target = $fileName . '.' . ($i + 1) . '.gz';
+ if ($this->compress) {
+ $target .= '.gz';
}
- rename($file, $target);
+ rename($source, $target);
}
- }
+ }
}
- public function setFile($fileName) {
- $this->file = $fileName;
- // As LoggerAppenderFile does not create the directory, it has to exist.
- // realpath() fails if the argument does not exist so the filename is separated.
- $this->expandedFileName = realpath(dirname($fileName));
- if ($this->expandedFileName === false) throw new Exception("Directory of $fileName does not exist!");
- $this->expandedFileName .= DIRECTORY_SEPARATOR . basename($fileName);
- }
-
-
/**
- * Set the maximum number of backup files to keep around.
- *
- * <p>The <b>MaxBackupIndex</b> option determines how many backup
- * files are kept before the oldest is erased. This option takes
- * a positive integer value. If set to zero, then there will be no
- * backup files and the log file will be truncated when it reaches
- * MaxFileSize.
- *
- * @param mixed $maxBackups
+ * Writes a string to the target file. Opens file if not already open.
+ * @param string $string Data to write.
*/
- public function setMaxBackupIndex($maxBackups) {
- $this->setPositiveInteger('maxBackupIndex', $maxBackups);
- }
-
-
- public function append(LoggerLoggingEvent $event) {
- if($this->fp and $this->layout !== null) {
- if(flock($this->fp, LOCK_EX)) {
- fwrite($this->fp, $this->layout->format($event));
-
- // Stats cache must be cleared, otherwise filesize() returns cached results
- clearstatcache();
-
- // Rollover if needed
- if (filesize($this->expandedFileName) > $this->maxFileSize) {
- $this->rollOver();
- }
-
- flock($this->fp, LOCK_UN);
- } else {
+ protected function write($string) {
+ // Lazy file open
+ if(!isset($this->fp)) {
+ $this->openFile();
+ }
+
+ // Lock the file while writing and possible rolling over
+ if(flock($this->fp, LOCK_EX)) {
+
+ // Write to locked file
+ if(fwrite($this->fp, $string) === false) {
+ $this->warn("Failed writing to file. Closing appender.");
$this->closed = true;
}
- }
+
+ // Stats cache must be cleared, otherwise filesize() returns cached results
+ clearstatcache();
+
+ // Rollover if needed
+ if (filesize($this->file) > $this->maxFileSize) {
+ try {
+ $this->rollOver();
+ } catch (LoggerException $ex) {
+ $this->warn("Rollover failed: " . $ex->getMessage() . " Closing appender.");
+ $this->closed = true;
+ }
+ }
+
+ flock($this->fp, LOCK_UN);
+ } else {
+ $this->warn("Failed locking file for writing. Closing appender.");
+ $this->closed = true;
+ }
}
public function activateOptions() {
parent::activateOptions();
- if ($this->compress == true && !extension_loaded('zlib')) {
- $this->warn('The zlib extension is required for file-compression');
- $this->closed = true;
+ if ($this->compress && !extension_loaded('zlib')) {
+ $this->warn("The 'zlib' extension is required for file compression. Disabling compression.");
+ $this->compression = false;
}
}
/**
+ * Set the 'maxBackupIndex' parameter.
+ * @param integer $maxBackupIndex
+ */
+ public function setMaxBackupIndex($maxBackupIndex) {
+ $this->setPositiveInteger('maxBackupIndex', $maxBackupIndex);
+ }
+
+ /**
* Returns the 'maxBackupIndex' parameter.
* @return integer
*/
@@ -225,32 +240,11 @@
}
/**
- * Set the maximum size that the output file is allowed to reach
- * before being rolled over to backup files.
- * <p>In configuration files, the <b>maxFileSize</b> option takes an
- * long integer in the range 0 - 2^63. You can specify the value
- * with the suffixes "KB", "MB" or "GB" so that the integer is
- * interpreted being expressed respectively in kilobytes, megabytes
- * or gigabytes. For example, the value "10KB" will be interpreted
- * as 10240.
- *
- * @param mixed $value
- * @return the actual file size set
- */
- public function setMaxFileSize($value) {
- $this->setFileSize('maxFileSize', $value);
- }
-
- /**
- * Set the maximum size that the output file is allowed to reach
- * before being rolled over to backup files.
- *
+ * Set the 'maxFileSize' parameter.
* @param mixed $maxFileSize
- * @see setMaxFileSize()
- * @deprecated
*/
- public function setMaximumFileSize($maxFileSize) {
- return $this->setMaxFileSize($maxFileSize);
+ public function setMaxFileSize($maxFileSize) {
+ $this->setFileSize('maxFileSize', $maxFileSize);
}
/**
@@ -261,7 +255,29 @@
return $this->maxFileSize;
}
+ /**
+ * Set the 'maxFileSize' parameter (kept for backward compatibility).
+ * @param mixed $maxFileSize
+ * @deprecated Use setMaxFileSize() instead.
+ */
+ public function setMaximumFileSize($maxFileSize) {
+ $this->warn("The 'maximumFileSize' parameter is deprecated. Use 'maxFileSize' instead.");
+ return $this->setMaxFileSize($maxFileSize);
+ }
+
+ /**
+ * Sets the 'compress' parameter.
+ * @param boolean $compress
+ */
public function setCompress($compress) {
$this->setBoolean('compress', $compress);
}
+
+ /**
+ * Returns the 'compress' parameter.
+ * @param boolean
+ */
+ public function getCompress() {
+ return $this->compress;
+ }
}
diff --git a/src/test/php/appenders/LoggerAppenderFileTest.php b/src/test/php/appenders/LoggerAppenderFileTest.php
index d3c0192..5ed3eef 100644
--- a/src/test/php/appenders/LoggerAppenderFileTest.php
+++ b/src/test/php/appenders/LoggerAppenderFileTest.php
@@ -68,6 +68,21 @@
self::assertTrue($appender->requiresLayout());
}
+ public function testActivationDoesNotCreateTheFile() {
+ $path = PHPUNIT_TEMP_DIR . "/doesnotexisthopefully.log";
+ @unlink($path);
+ $appender = new LoggerAppenderFile();
+ $appender->setFile($path);
+ $appender->activateOptions();
+
+ self::assertFalse(file_exists($path));
+
+ $event = LoggerTestHelper::getInfoEvent('bla');
+ $appender->append($event);
+
+ self::assertTrue(file_exists($path));
+ }
+
public function testSimpleLogging() {
$config = $this->config1;
$config['appenders']['default']['params']['file'] = $this->testPath;
diff --git a/src/test/php/appenders/LoggerAppenderRollingFileTest.php b/src/test/php/appenders/LoggerAppenderRollingFileTest.php
index 77d5c4a..2ae1289 100644
--- a/src/test/php/appenders/LoggerAppenderRollingFileTest.php
+++ b/src/test/php/appenders/LoggerAppenderRollingFileTest.php
@@ -113,7 +113,8 @@
$file = PHPUNIT_TEMP_DIR . '/TEST-rolling.txt.2';
$this->checkFileContent($file);
- $this->assertFalse(file_exists(PHPUNIT_TEMP_DIR.'/TEST-rolling.txt.3'), 'should not roll over three times');
+ // Should not roll over three times
+ $this->assertFalse(file_exists(PHPUNIT_TEMP_DIR.'/TEST-rolling.txt.3'));
}
public function testLoggingViaLogger() {
@@ -202,5 +203,4 @@
@unlink(PHPUNIT_TEMP_DIR.'/TEST-rolling.txt.1.gz');
@unlink(PHPUNIT_TEMP_DIR.'/TEST-rolling.txt.2.gz');
}
-
}