ScheduledThreadPoolExecutor not closed when using a TimeBasedRotationPolicy

[pitschr]

Hi.

It's me again :-) . I now realized that the plugin (which is using your library) prevents the main application to be shutdown gracefully.

I digged the code and figured out that the issue happens when using the DailyRotationPolicy.getInstance() .

        final var config = RotationConfig
                .builder()
                .file(baseFile)
                .filePattern(rolloverFile)
                .policy(DailyRotationPolicy.getInstance()) // <-- PROBLEM!
                .append(false);

The reason is DailyRotationPolicy extends TimeBasedRotationPolicy which invokes the following line in method TimeBasedRotationPolicy#start(Rotatable)

        config.getExecutorService().schedule(task, triggerDelayMillis, TimeUnit.MILLISECONDS);

However, this executor service is never subject to be closed. To let my application to shutdown correctly, I need to call the following line in my code:

        fos.getConfig().getExecutorService().shutdownNow();

This has been proven as working. But I think the executor service should be closed by your library as it is owned by you. Therefore, I propose to add the line into your RotatingFileOutputStream#close(); unless you see a better way how to close the executor service properly:

    @Override
    public synchronized void close() throws IOException {
        config.getCallback().onClose(null, config.getClock().now(), stream);
        config.getExecutorService().shutdownNow();  // <-- NEW LINE
        stream.close();
        stream = null;
    }

Be aware, this code has not been tested by myself and I do not know if there are not side-effects. I just explained the root cause what is keeping the application running and running and how the potential fix could look like.

Let me know if you need further details. If you prefer a Pull Request just let me know it.

KR, Christoph

[vy]

[Darn! I have thought of this while writing the code, but forgot about it along the way.]

Closing the ScheduledExecutorService (SES) at close() is not a desirable solution. SES might have been provided by the user to RotationConfig and it is users responsibility to close it. A viable option is to mark Threads created by the default SES as daemon. I've checked this and it works. Right now I am busy with writing a unit test to verify the behavior. Once I am done, I will put it in a branch for you to test.

I will keep you posted.

[vy]

Purposing JUnit 4 to test whether something gets closed at JVM exit or not turned out be more difficult than I was anticipating. Nevertheless, I've pushed my changes issues/12 branch. Would you mind checking it out and giving it a try, please? If you confirm the fix, I will cut a new release.

[pitschr]

Hello.

Thanks for the quick fix. Yes, I can confirm the issue as fixed as the application is shutdown gracefully. Tested on Windows. Well done!

To check if the application is shutdown gracefully in most safe way I think the approach would to execute the sample application as part of CI/CD pipeline with timeout command. If the application is killed because of timeout the exit code will be 124, otherwise the exit code is 0. This check can be added to your CI/CD pipeline. The sample application should just print a e.g. "hello world" text to file and then shut down immediately. Just an idea - of course through JUnit would be elegant, but I am afraid that it would be too fragile and limited to a specific system.

Example:

# If it takes longer than 5 seconds then kill application and build should fail
timeout 5 java -jar ...

Thanks for fixing it!

[vy]

It took quite a while for me to properly test this. But finally pulled it out. I have just released v0.9.2, should momentarily appear in Maven Central. Thanks so much again.