Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

fixes FileLog doesn't delete on rotation if count(files) is greater 'rotate' #1705

Merged
merged 2 commits into from

3 participants

@Schlaefer

If for whatever reason there were more log files than should according
to the rotate parameter the log files weren't deleted at all.

@markstory markstory commented on the diff
lib/Cake/Log/Engine/FileLog.php
((8 lines not shown))
}
- if ($this->_config['rotate']) {
@markstory Owner

Why was this condition removed? It looks like rotation will always occur now.

Because if log files exist and rotate is decreased from a higher value to 0 the existing log files are never cleared out otherwise.

Now at first the .log is processed in 203-207. After that it's checked if there are any backlog files and if so they are removed according to the rotate. If rotate is zero and there are backlog files all of them are deleted (so yes, rotation will always occur).

The overhead is a glob($filepath . '.*');, but filesize will exit early, so I don't think this is a performance issue for a reasonable size.

@ADmad Collaborator
ADmad added a note

I dislike this. Even if the overhead is minor there's no point penalizing everyone to support an edge case. Extra log files lying around don't hurt anyone. If you decrease the rotate value you should clean up the log files manually.

So I am a :-1: on this.

Even if the overhead is minor there's no point penalizing everyone to support an edge case.

But that's the default behavior at the moment with rotate set to 10. With this as default there's no penalty to anyone. If there's is a strong opinion about not penalizing everyone rotate should be 0 right now instead of "wasting" everyones I/O and disk space.

Anyhow I don't care much about rotate = 0. More important was to make sure that the backlog doesn't grow endlessly with count(files) > rotate and rotate > 0 by accident.

If you decrease the rotate value you should clean up the log files manually.

One should. I thought it was a bug when the logs weren't deleted after I reduced rotate on my dev machine.

@ADmad Collaborator
ADmad added a note

Since this check and deletions are needed only once after log filesize increases over the specified size, I guess there's no point being too fussy about it. I concede, :+1:.

Though a micro optimization possible would be, for rotate > 0 do the file renaming after older files have been purged so that one less file needs to be globed :smile:.

Though a micro optimization possible would be, for rotate > 0 do the file renaming after older files have been purged so that one less file needs to be globed :smile:.

I'm not really sure what you mean with "globed". My non native english is failing me here. :grimacing:. [e] Oh … I see … sometimes I'm just stupid … :wink:

Having the renaming after the rotate was the original version. I decided against it for three reasons:

  • Manipulating the .log file was handled in two places – before and after the rotate, now it is a single place before the rotate.
  • Having the renaming occurring later means you have a "delete files plus one because somewhere later something happens that's not clear right now" which leads to an awkward/non-obvious +1 testing in the rotate part.
  • The glob lists the directory with one more file but this file is never touched otherwise.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markstory
Owner

Given the discussion above I think this looks good.

@ADmad ADmad merged commit 105f032 into cakephp:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
14 lib/Cake/Log/Engine/FileLog.php
@@ -201,17 +201,21 @@ protected function _rotateFile($filename) {
}
if ($this->_config['rotate'] === 0) {
- return unlink($filepath);
+ $result = unlink($filepath);
+ } else {
+ $result = rename($filepath, $filepath . '.' . time());
}
- if ($this->_config['rotate']) {
@markstory Owner

Why was this condition removed? It looks like rotation will always occur now.

Because if log files exist and rotate is decreased from a higher value to 0 the existing log files are never cleared out otherwise.

Now at first the .log is processed in 203-207. After that it's checked if there are any backlog files and if so they are removed according to the rotate. If rotate is zero and there are backlog files all of them are deleted (so yes, rotation will always occur).

The overhead is a glob($filepath . '.*');, but filesize will exit early, so I don't think this is a performance issue for a reasonable size.

@ADmad Collaborator
ADmad added a note

I dislike this. Even if the overhead is minor there's no point penalizing everyone to support an edge case. Extra log files lying around don't hurt anyone. If you decrease the rotate value you should clean up the log files manually.

So I am a :-1: on this.

Even if the overhead is minor there's no point penalizing everyone to support an edge case.

But that's the default behavior at the moment with rotate set to 10. With this as default there's no penalty to anyone. If there's is a strong opinion about not penalizing everyone rotate should be 0 right now instead of "wasting" everyones I/O and disk space.

Anyhow I don't care much about rotate = 0. More important was to make sure that the backlog doesn't grow endlessly with count(files) > rotate and rotate > 0 by accident.

If you decrease the rotate value you should clean up the log files manually.

One should. I thought it was a bug when the logs weren't deleted after I reduced rotate on my dev machine.

@ADmad Collaborator
ADmad added a note

Since this check and deletions are needed only once after log filesize increases over the specified size, I guess there's no point being too fussy about it. I concede, :+1:.

Though a micro optimization possible would be, for rotate > 0 do the file renaming after older files have been purged so that one less file needs to be globed :smile:.

Though a micro optimization possible would be, for rotate > 0 do the file renaming after older files have been purged so that one less file needs to be globed :smile:.

I'm not really sure what you mean with "globed". My non native english is failing me here. :grimacing:. [e] Oh … I see … sometimes I'm just stupid … :wink:

Having the renaming after the rotate was the original version. I decided against it for three reasons:

  • Manipulating the .log file was handled in two places – before and after the rotate, now it is a single place before the rotate.
  • Having the renaming occurring later means you have a "delete files plus one because somewhere later something happens that's not clear right now" which leads to an awkward/non-obvious +1 testing in the rotate part.
  • The glob lists the directory with one more file but this file is never touched otherwise.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- $files = glob($filepath . '.*');
- if (count($files) === $this->_config['rotate']) {
+ $files = glob($filepath . '.*');
+ if ($files) {
+ $filesToDelete = count($files) - $this->_config['rotate'];
+ while ($filesToDelete > 0) {
unlink(array_shift($files));
+ $filesToDelete--;
}
}
- return rename($filepath, $filepath . '.' . time());
+ return $result;
}
}
View
3  lib/Cake/Test/Case/Log/Engine/FileLogTest.php
@@ -117,6 +117,8 @@ public function testRotation() {
$result = file_get_contents($files[1]);
$this->assertRegExp('/Warning: Test warning second/', $result);
+ file_put_contents($path . 'error.log.0000000000', "The oldest log file with over 35 bytes.\n");
+
sleep(1);
clearstatcache();
$log->write('warning', 'Test warning fourth');
@@ -140,6 +142,7 @@ public function testRotation() {
'size' => 35,
'rotate' => 0
));
+ file_put_contents($path . 'debug.log.0000000000', "The oldest log file with over 35 bytes.\n");
$log->write('debug', 'Test debug');
$this->assertTrue(file_exists($path . 'debug.log'));
Something went wrong with that request. Please try again.