Close PrintStream properly in TestConfigurationLoader tests

[dnskr]

The PR fixes the issue in com.facebook.airlift.configuration.TestConfigurationLoader. Tests testLoadsFromFile() and testSystemOverridesFile() open PrintStream and close it only if an Exception occurred.
The tests cannot be completed successfully, because of exception in teardown() method.

The fix closes PrintStream in any way by using try-with-resources statement.

Before

$ mvn -f configuration/pom.xml test
[INFO] Scanning for projects...
[INFO] 
[INFO] -----------------< com.facebook.airlift:configuration >-----------------
...
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running TestSuite
[ERROR] Tests run: 119, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 2.435 s <<< FAILURE! - in TestSuite
[ERROR] teardown(com.facebook.airlift.configuration.TestConfigurationLoader)  Time elapsed: 0.016 s  <<< FAILURE!
java.nio.file.FileSystemException: C:\Users\dnskr\AppData\Local\Temp\1713707499451-0: failed to delete one or more files; see suppressed exceptions for details
        at com.google.common.io.MoreFiles.throwDeleteFailed(MoreFiles.java:785)
        ...
        Suppressed: java.nio.file.FileSystemException: C:\Users\dnskr\AppData\Local\Temp\1713707499451-0\config13441979576855912152.properties: The process cannot access the file because it is being used by another process.

                at java.base/sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:92)
                at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:103)
                at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:108)
                at java.base/sun.nio.fs.WindowsFileSystemProvider.implDelete(WindowsFileSystemProvider.java:270)
                at java.base/sun.nio.fs.AbstractFileSystemProvider.delete(AbstractFileSystemProvider.java:105)
                at java.base/java.nio.file.Files.delete(Files.java:1141)
                at com.google.common.io.MoreFiles.deleteRecursivelyInsecure(MoreFiles.java:672)
                at com.google.common.io.MoreFiles.deleteDirectoryContentsInsecure(MoreFiles.java:691)
                at com.google.common.io.MoreFiles.deleteRecursivelyInsecure(MoreFiles.java:665)
                at com.google.common.io.MoreFiles.deleteRecursively(MoreFiles.java:540)
                ... 30 more
[INFO] 
[INFO] Results:
[INFO]
[ERROR] Failures: 
[ERROR]   TestConfigurationLoader.teardown:50 » FileSystem C:\Users\dnskr\AppData\Local\...
[INFO]
[ERROR] Tests run: 119, Failures: 1, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  11.026 s
[INFO] Finished at: 2024-04-21T15:51:40+02:00
[INFO] ------------------------------------------------------------------------

After

$ mvn -f configuration/pom.xml test
[INFO] Scanning for projects...
[INFO] 
[INFO] -----------------< com.facebook.airlift:configuration >-----------------
...
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running TestSuite
[INFO] Tests run: 118, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.487 s - in TestSuite
[INFO] 
[INFO] Results:
[INFO]
[INFO] Tests run: 118, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  10.806 s
[INFO] Finished at: 2024-04-21T15:53:14+02:00
[INFO] ------------------------------------------------------------------------

[elharo]

Eek! PrintStream! This class really shouldn't be used for multiple reasons. Try an OutputStreamWriter instead. I'll approve since this isn' a new problem, but it should be fixed

[dnskr]

@elharo Could you please explain why it should not be used or point me to the place where I can read about it?

BTW, PrintStream is used in 2 classes:

airlift/log-manager/src/main/java/com/facebook/airlift/log/Logging.java

Lines 96 to 97 in 541e7ac

	System.setOut(new PrintStream(new LoggingOutputStream(Logger.get("stdout")), true));
	System.setErr(new PrintStream(new LoggingOutputStream(Logger.get("stderr")), true));

airlift/configuration/src/test/java/com/facebook/airlift/configuration/TestConfigurationLoader.java

Line 71 in 1016fbd

try (PrintStream out = new PrintStream(new FileOutputStream(file))) {

[elharo]

Well since you asked :-)

It's not all about why you shouldn't use PrintStream, but that is in fact a large part of the inspiration for writing that book.

tldr;

1. PrintStream doesn't handle line endings correctly, especially for network protocols.
2. PrintStream doesn't manage character sets correctly across platforms.
3. PrintStream swallows exceptions.