ppkarwasz commented on code in PR #3226:
URL: https://github.com/apache/logging-log4j2/pull/3226#discussion_r1851632621
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManager.java:
##########
@@ -506,9 +506,16 @@ public synchronized void rollover() {
if (rollover(rolloverStrategy)) {
try {
- size = 0;
- initialTime = System.currentTimeMillis();
createFileAfterRollover();
+ final File file = new File(getFileName());
+ size = file.length();
+ try {
+ final FileTime creationTime = (FileTime)
Files.getAttribute(file.toPath(), "creationTime");
+ initialTime = creationTime.toMillis();
+ } catch (Exception ex) {
+ LOGGER.warn("Unable to get current file time for {}",
file);
+ initialTime = System.currentTimeMillis();
+ }
Review Comment:
- Even if the rollover fails, I would still reset the `size` field. This is
the field used by
[`SizeBasedTriggeringPolicy`](https://logging.apache.org/log4j/3.x/manual/appenders/rolling-file.html#SizeBasedTriggeringPolicy)
to determine, when a rollover is due. Otherwise the triggering policy might
attempt a rollover at **each** log event. It is safe to assume that the new
rollover might fail too.
- The `initialTime` field is not really used by any triggering policy at
runtime. It is only used at **configuration time** and never queried again. I
would leave it as is.
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManagerTest.java:
##########
@@ -188,4 +193,40 @@ public void testCreateParentDir() {
manager.close();
}
}
+
+ @Test
+ @Issue("https://github.com/apache/logging-log4j2/issues/2592";)
+ public void testRolloverOfDeletedFile() throws IOException {
+ final Configuration configuration = new NullConfiguration();
+ final File file = File.createTempFile("testRolloverOfDeletedFile",
"log");
+ file.deleteOnExit();
+ final String testContent = "Test";
+ try (final OutputStream os =
+ new ByteArrayOutputStream(); // use a dummy
OutputStream so that the real file can be deleted
+ final RollingFileManager manager = new RollingFileManager(
+ configuration.getLoggerContext(),
+ file.getAbsolutePath(),
+ "testRolloverOfDeletedFile.log.%d{yyyy-MM-dd}",
+ os,
+ true,
+ false,
+ 0,
+ System.currentTimeMillis(),
+ OnStartupTriggeringPolicy.createPolicy(1),
+ DefaultRolloverStrategy.newBuilder().build(),
+ file.getName(),
+ PatternLayout.createDefaultLayout(configuration),
Review Comment:
Both the `LoggerContext` and `Layout` parameters can actually be `null`.
This way you don't need the rather expensive `Configuration` object.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManager.java:
##########
@@ -636,7 +643,7 @@ private boolean rollover(final RolloverStrategy strategy) {
asyncExecutor.execute(new
AsyncAction(descriptor.getAsynchronous(), this));
releaseRequired = false;
}
- return success;
+ return true;
Review Comment:
There are a lot of `boolean`s in this method with different purposes. It
might be useful to rename them to make the code more clear. For example:
- `releaseRequired` actually tells us if the asynchronous action has
started. Maybe we could rename it to `asyncActionStarted` and define it only
where it is needed (just after the first `try`).
- `success` tells use if the synchronous action has succeeded. It should be
rather called something like `syncActionSuccess` and be defined just before the
synchronous action is executed.
- A third unnamed `boolean` is used in the return statements and there is no
comment stating what `rollover(RolloverStrategy)` returns. I think we should
introduce a `outputStreamClosed` variable that is defined at the beginning of
the method and used in all the return statements. The lack of documentation is
what caused #2592 in the first place.
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManagerTest.java:
##########
@@ -188,4 +193,40 @@ public void testCreateParentDir() {
manager.close();
}
}
+
+ @Test
+ @Issue("https://github.com/apache/logging-log4j2/issues/2592";)
+ public void testRolloverOfDeletedFile() throws IOException {
+ final Configuration configuration = new NullConfiguration();
+ final File file = File.createTempFile("testRolloverOfDeletedFile",
"log");
+ file.deleteOnExit();
+ final String testContent = "Test";
+ try (final OutputStream os =
+ new ByteArrayOutputStream(); // use a dummy
OutputStream so that the real file can be deleted
+ final RollingFileManager manager = new RollingFileManager(
Review Comment:
Nice test! :100:
By calling the constructor directly, you prevent the registration of
`RollingFileManager` in the registry kept by `AbstractManager`. This a very
nice approach for tests!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
