Merge pull request #12 from levelops/bugs/SEI-2499

SEI-2499 - Jenkins Plugin - Causing "Too many open files" on Jenkins Server

pom.xml

@@ -82,7 +82,7 @@
     </profiles>
 
     <properties>
-        <revision>1.0.28</revision>
+        <revision>1.0.29</revision>
         <!-- <changelist>999999-SNAPSHOT</changelist> -->
         <changelist></changelist>
         <product.build.sourceEncoding>UTF-8</product.build.sourceEncoding>

src/main/java/io/jenkins/plugins/propelo/commons/service/JobLogsService.java

@@ -25,7 +25,21 @@ public UUID getLogFailedString(Run run, File jobRunCompleteDataDirectory) {
         File failedJobLogFile = getPathToLogFile(run.getParent().getRootDir().toString(), run.getNumber()).toFile();
         StringBuilder logsFromFile = new StringBuilder();
         String lineFromFile;
-        try (BufferedReader bufferedReader = new BufferedReader(new FileReader(failedJobLogFile))) {
+        /*
+        https://stackoverflow.com/questions/884007/correct-way-to-close-nested-streams-and-writers-in-java
+        Good:
+        try (ObjectInputStream ois = new ObjectInputStream(new FileInputStream(f))) {
+          // do something with ois
+        }
+        Better:
+        try (FileInputStream fis = new FileInputStream(f); ObjectInputStream ois = new ObjectInputStream(fis)) {
+          // do something with ois
+        }
+        Reason: The try-with-resources is not aware of the inner FileInputStream, so if the ObjectInputStream constructor throws an exception,
+        the FileInputStream is never closed (until the garbage collector gets to it).
+         */
+        try ( FileReader fr = new FileReader(failedJobLogFile);
+              BufferedReader bufferedReader = new BufferedReader(fr)) {
             while ((lineFromFile = bufferedReader.readLine()) != null) {
                 logsFromFile.append(lineFromFile);
                 logsFromFile.append(separator);

src/main/java/io/jenkins/plugins/propelo/commons/service/JobRunPerforceChangesParserService.java

@@ -50,7 +50,22 @@ public List<String> parsePerforceChangeCommitIds(ObjectMapper xmlMapper, File jo
 
     private String getInputStreamAsString(InputStream inputStream) throws IOException {
         StringBuilder stringBuilder = new StringBuilder();
-        try (BufferedReader br = new BufferedReader(new InputStreamReader(inputStream))) {
+
+        /*
+        https://stackoverflow.com/questions/884007/correct-way-to-close-nested-streams-and-writers-in-java
+        Good:
+        try (ObjectInputStream ois = new ObjectInputStream(new FileInputStream(f))) {
+          // do something with ois
+        }
+        Better:
+        try (FileInputStream fis = new FileInputStream(f); ObjectInputStream ois = new ObjectInputStream(fis)) {
+          // do something with ois
+        }
+        Reason: The try-with-resources is not aware of the inner FileInputStream, so if the ObjectInputStream constructor throws an exception,
+        the FileInputStream is never closed (until the garbage collector gets to it).
+         */
+        try ( InputStreamReader isr = new InputStreamReader(inputStream);
+              BufferedReader br = new BufferedReader(isr) ) {
             String line;
             while ((line = br.readLine()) != null) {
                 stringBuilder.append(line);

src/main/java/io/jenkins/plugins/propelo/commons/service/ZipService.java

@@ -57,22 +57,40 @@ public void zipDirectory(File sourceDirectory, File outputZipFile) throws IOExce
         }
     }
 
+    /*
+    Unzip is not used in the plugin. Fixing this proactively.
+     */
     public void unZip(File sourceZipFile, File unzipDestinationDir) throws IOException {
         byte[] buffer = new byte[1024];
-        ZipInputStream zis = new ZipInputStream(new FileInputStream(sourceZipFile));
-        ZipEntry zipEntry = zis.getNextEntry();
-        while (zipEntry != null) {
-            File newFile = newFile(unzipDestinationDir, zipEntry);
-            try (FileOutputStream fos = new FileOutputStream(newFile)){
-                int len;
-                while ((len = zis.read(buffer)) > 0) {
-                    fos.write(buffer, 0, len);
+
+        /*
+        https://stackoverflow.com/questions/884007/correct-way-to-close-nested-streams-and-writers-in-java
+        Good:
+        try (ObjectInputStream ois = new ObjectInputStream(new FileInputStream(f))) {
+          // do something with ois
+        }
+        Better:
+        try (FileInputStream fis = new FileInputStream(f); ObjectInputStream ois = new ObjectInputStream(fis)) {
+          // do something with ois
+        }
+        Reason: The try-with-resources is not aware of the inner FileInputStream, so if the ObjectInputStream constructor throws an exception,
+        the FileInputStream is never closed (until the garbage collector gets to it).
+         */
+        try (FileInputStream fis = new FileInputStream(sourceZipFile);
+             ZipInputStream zis = new ZipInputStream(fis) ) {
+            ZipEntry zipEntry = zis.getNextEntry();
+            while (zipEntry != null) {
+                File newFile = newFile(unzipDestinationDir, zipEntry);
+                try (FileOutputStream fos = new FileOutputStream(newFile)){
+                    int len;
+                    while ((len = zis.read(buffer)) > 0) {
+                        fos.write(buffer, 0, len);
+                    }
                 }
+                zipEntry = zis.getNextEntry();
             }
-            zipEntry = zis.getNextEntry();
+            zis.closeEntry();
         }
-        zis.closeEntry();
-        zis.close();
     }
 
     public static File newFile(File destinationDir, ZipEntry zipEntry) throws IOException {

src/main/java/io/jenkins/plugins/propelo/job_reporter/extensions/LevelOpsRunListener.java

@@ -36,7 +36,9 @@
 import java.io.File;
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
+import java.nio.file.DirectoryStream;
 import java.nio.file.Files;
+import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.List;
@@ -199,20 +201,22 @@ private void deleteJobRunDataCompleteDirectoryContents() {
             return;
         }
         String todayDirName = DateUtils.getDateFormattedDirName();
-        try {
-            Files.newDirectoryStream(
+
+        try (DirectoryStream<Path> ds = Files.newDirectoryStream(
                 dataDirectoryWithVersion.toPath(),
                 (entry) -> {
-                    boolean use = entry.getFileName().toString().equalsIgnoreCase(todayDirName);
+                    boolean use = !entry.getFileName().toString().equalsIgnoreCase(todayDirName);
                     LOGGER.log(Level.FINER, "Filtering files... accept {0}? {1}", new Object[]{todayDirName, use});
                     return use;
-                }).forEach(item -> {
-                    LOGGER.log(Level.FINE, "Deleting old historic report: {0}", item.toString());
-                    FileUtils.deleteQuietly(item.toFile());
-                });
+                })) {
+            ds.forEach(item -> {
+                LOGGER.log(Level.FINE, "Deleting old historic report: {0}", item.toString());
+                FileUtils.deleteQuietly(item.toFile());
+            });
+
         } catch (SecurityException | IOException e) {
             LOGGER.log(Level.SEVERE, e, () -> {
-                return String.format("Unable to delete all files from the historic reports in the directory '%s' other than %s", dataDirectoryWithVersion.toPath().toString(), todayDirName);
+                return String.format("Unable to delete all files from the historic reports in the directory '%s' other than %s", dataDirectoryWithVersion.toPath(), todayDirName);
             });
         }
     }

src/main/java/io/jenkins/plugins/propelo/job_reporter/plugins/PropeloPluginImpl.java

@@ -34,7 +34,9 @@
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
 import java.net.UnknownHostException;
+import java.nio.file.DirectoryStream;
 import java.nio.file.Files;
+import java.nio.file.Path;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
@@ -259,12 +261,15 @@ private void deleteOlderDirectories() {
             LOGGER.log(Level.FINE, "Skipping old directories deletion: plugin_dir={0}, todays_data_directory_name={1}", new Object[]{expandedLevelOpsPluginDir, currentDataDirectoryWithVersion});
             return;
         }
-        try {
-            Files.newDirectoryStream(expandedLevelOpsPluginDir.toPath(), (path) -> {
-                boolean use = OLDER_DIRECTORIES_PATTERN.matcher(path.getFileName().toString()).find() && !currentDataDirectoryWithVersion.getName().equalsIgnoreCase(path.getFileName().toString());
-                LOGGER.log(Level.FINEST, "Filering files... accept {0}? {1}", new Object[]{path.toString(), use});
-                return use;
-            }).forEach(path -> {
+
+        try (DirectoryStream<Path> ds = Files.newDirectoryStream(
+                expandedLevelOpsPluginDir.toPath(),
+                (path) -> {
+                    boolean use = OLDER_DIRECTORIES_PATTERN.matcher(path.getFileName().toString()).find() && !currentDataDirectoryWithVersion.getName().equalsIgnoreCase(path.getFileName().toString());
+                    LOGGER.log(Level.FINEST, "Filering files... accept {0}? {1}", new Object[]{path.toString(), use});
+                    return use;
+                })) {
+            ds.forEach(path -> {
                 LOGGER.log(Level.FINER, "Deleting file: {0}", path);
                 FileUtils.deleteQuietly(path.toFile());
             });