Bug 577432 - Speed up and simplify file processing in StorageI20211206-1800I20211205-1800I20211204-1800I20211204-0500I20211203-1800I20211203-1010I20211202-0900I20211202-0440I20211201-1950I20211201-1800I20211130-1800

Change-Id: I69dc7e2ebcda363fddcde7da56f183a637e83459
Signed-off-by: Hannes Wellmann <wellmann.hannes1@gmx.net>
Reviewed-on: https://git.eclipse.org/r/c/equinox/rt.equinox.framework/+/188090
Tested-by: Equinox Bot <equinox-bot@eclipse.org>

7 files changed, 91 insertions, 150 deletions

diff --git a/bundles/org.eclipse.osgi.tests/META-INF/MANIFEST.MF b/bundles/org.eclipse.osgi.tests/META-INF/MANIFEST.MF
index e6c6868a7..7c2281c8a 100644
--- a/bundles/org.eclipse.osgi.tests/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.osgi.tests/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: Core OSGi Tests
 Bundle-SymbolicName: org.eclipse.osgi.tests;singleton:=true
-Bundle-Version: 3.16.500.qualifier
+Bundle-Version: 3.16.600.qualifier
 Bundle-Vendor: Eclipse.org
 Require-Bundle: 
  org.eclipse.core.runtime,
diff --git a/bundles/org.eclipse.osgi.tests/pom.xml b/bundles/org.eclipse.osgi.tests/pom.xml
index 6e9591109..137e447a4 100644
--- a/bundles/org.eclipse.osgi.tests/pom.xml
+++ b/bundles/org.eclipse.osgi.tests/pom.xml
@@ -19,7 +19,7 @@
   </parent>
   <groupId>org.eclipse.osgi</groupId>
   <artifactId>org.eclipse.osgi.tests</artifactId>
-  <version>3.16.500-SNAPSHOT</version>
+  <version>3.16.600-SNAPSHOT</version>
   <packaging>eclipse-test-plugin</packaging>
 
   <properties>
diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/MultiReleaseJarTests.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/MultiReleaseJarTests.java
index 3a80a4814..d5301a43f 100644
--- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/MultiReleaseJarTests.java
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/MultiReleaseJarTests.java
@@ -817,7 +817,7 @@ public class MultiReleaseJarTests extends AbstractBundleTests {
 
 	public void testMultiReleaseBundleDeletedRestart() throws Exception {
 		File copyMrJarBundle = OSGiTestsActivator.getContext().getDataFile("copy-" + mrJarBundle.getName());
-		StorageUtil.readFile(new FileInputStream(mrJarBundle), copyMrJarBundle);
+		StorageUtil.copy(mrJarBundle, copyMrJarBundle);
 
 		System.setProperty("java.specification.version", "9");
 
diff --git a/bundles/org.eclipse.osgi/META-INF/MANIFEST.MF b/bundles/org.eclipse.osgi/META-INF/MANIFEST.MF
index e69682fc2..4da893f7f 100644
--- a/bundles/org.eclipse.osgi/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.osgi/META-INF/MANIFEST.MF
@@ -104,7 +104,7 @@ Bundle-Activator: org.eclipse.osgi.internal.framework.SystemBundleActivator
 Bundle-Description: %systemBundle
 Bundle-Copyright: %copyright
 Bundle-Vendor: %eclipse.org
-Bundle-Version: 3.17.100.qualifier
+Bundle-Version: 3.17.200.qualifier
 Bundle-Localization: systembundle
 Bundle-DocUrl: http://www.eclipse.org
 Eclipse-ExtensibleAPI: true
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/Storage.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/Storage.java
index 7af943e77..ff7e2499d 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/Storage.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/Storage.java
@@ -10,7 +10,8 @@
  *
  * Contributors:
  *     IBM Corporation - initial API and implementation
- *      Christoph Laeubrich - Bug 527175 - Storage#getSystemContent() should first make the file absolute
+ *     Christoph Laeubrich - Bug 527175 - Storage#getSystemContent() should first make the file absolute
+ *     Hannes Wellmann - Bug 577432 - Speed up and improve file processing in Storage
  *******************************************************************************/
 package org.eclipse.osgi.storage;
 
@@ -22,7 +23,6 @@ import java.io.DataInputStream;
 import java.io.DataOutputStream;
 import java.io.File;
 import java.io.FileInputStream;
-import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.lang.reflect.Method;
@@ -30,6 +30,7 @@ import java.net.MalformedURLException;
 import java.net.URL;
 import java.net.URLConnection;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
 import java.nio.file.Path;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
@@ -1122,10 +1123,8 @@ public class Storage {
 				if (inFile.isDirectory()) {
 					// need to delete the outFile because it is not a directory
 					outFile.delete();
-					StorageUtil.copyDir(inFile, outFile);
-				} else {
-					StorageUtil.readFile(in, outFile);
 				}
+				StorageUtil.copy(inFile, outFile);
 			} else {
 				StorageUtil.readFile(in, outFile);
 			}
@@ -1238,15 +1237,11 @@ public class Storage {
 			File delete = new File(target, DELETE_FLAG);
 			// and the directory is marked for delete
 			if (delete.exists()) {
-				// if rm fails to delete the directory and .delete was removed
-				if (!StorageUtil.rm(target, getConfiguration().getDebug().DEBUG_STORAGE) && !delete.exists()) {
-					try {
-						// recreate .delete
-						FileOutputStream out = new FileOutputStream(delete);
-						out.close();
-					} catch (IOException e) {
-						if (getConfiguration().getDebug().DEBUG_STORAGE)
-							Debug.println("Unable to write " + delete.getPath() + ": " + e.getMessage()); //$NON-NLS-1$ //$NON-NLS-2$
+				try {
+					deleteFlaggedDirectory(target);
+				} catch (IOException e) {
+					if (getConfiguration().getDebug().DEBUG_STORAGE) {
+						Debug.println("Unable to write " + delete.getPath() + ": " + e.getMessage()); //$NON-NLS-1$ //$NON-NLS-2$
 					}
 				}
 			} else {
@@ -1257,11 +1252,11 @@ public class Storage {
 
 	void delete(final File delete) throws IOException {
 		if (System.getSecurityManager() == null) {
-			delete0(delete);
+			deleteFlaggedDirectory(delete);
 		} else {
 			try {
 				AccessController.doPrivileged((PrivilegedExceptionAction<Void>) () -> {
-					delete0(delete);
+					deleteFlaggedDirectory(delete);
 					return null;
 				});
 			} catch (PrivilegedActionException e) {
@@ -1272,11 +1267,9 @@ public class Storage {
 		}
 	}
 
-	void delete0(File delete) throws IOException {
+	private void deleteFlaggedDirectory(File delete) throws IOException {
 		if (!StorageUtil.rm(delete, getConfiguration().getDebug().DEBUG_STORAGE)) {
-			/* create .delete */
-			FileOutputStream out = new FileOutputStream(new File(delete, DELETE_FLAG));
-			out.close();
+			ensureDeleteFlagFileExists(delete.toPath());
 		}
 	}
 
@@ -2163,21 +2156,15 @@ public class Storage {
 			bundleTempDir.deleteOnExit();
 			// This is just a safeguard incase the VM is terminated unexpectantly, it also looks like deleteOnExit cannot really work because
 			// the VM likely will still have a lock on the lib file at the time of VM exit.
-			File deleteFlag = new File(libTempDir, DELETE_FLAG);
-			if (!deleteFlag.exists()) {
-				// need to create a delete flag to force removal the temp libraries
-				try {
-					FileOutputStream out = new FileOutputStream(deleteFlag);
-					out.close();
-				} catch (IOException e) {
-					// do nothing; that would mean we did not make the temp dir successfully
-				}
+			try { // need to create a delete flag to force removal the temp libraries
+				ensureDeleteFlagFileExists(libTempDir.toPath());
+			} catch (IOException e) {
+				// do nothing; that would mean we did not make the temp dir successfully
 			}
 		}
 		// copy the library file
 		try {
-			InputStream in = new FileInputStream(realLib);
-			StorageUtil.readFile(in, libTempFile);
+			StorageUtil.copy(realLib, libTempFile);
 			// set permissions if needed
 			setPermissions(libTempFile);
 			libTempFile.deleteOnExit(); // this probably will not work because the VM will probably have the lib locked at exit
@@ -2189,6 +2176,13 @@ public class Storage {
 		}
 	}
 
+	private static void ensureDeleteFlagFileExists(Path directory) throws IOException {
+		Path deleteFlag = directory.resolve(DELETE_FLAG);
+		if (!Files.exists(deleteFlag) ) {
+			Files.createFile(deleteFlag);
+		}
+	}
+
 	public SecurityAdmin getSecurityAdmin() {
 		return securityAdmin;
 	}
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/StorageUtil.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/StorageUtil.java
index 4f4ebc37e..2c9eddf8b 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/StorageUtil.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/StorageUtil.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2005, 2020 IBM Corporation and others.
+ * Copyright (c) 2005, 2021 IBM Corporation and others.
  *
  * This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License 2.0
@@ -10,25 +10,28 @@
  *
  * Contributors:
  *     IBM Corporation - initial API and implementation
+ *     Hannes Wellmann - Bug 577432 - Speed up and improve file processing in Storage
  *******************************************************************************/
 
 package org.eclipse.osgi.storage;
 
-import java.io.DataInputStream;
-import java.io.DataOutputStream;
 import java.io.File;
-import java.io.FileInputStream;
-import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.MalformedURLException;
 import java.net.URL;
+import java.nio.file.FileVisitResult;
 import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.SimpleFileVisitor;
 import java.nio.file.StandardCopyOption;
+import java.nio.file.attribute.BasicFileAttributes;
 import java.util.Arrays;
 import java.util.Dictionary;
 import java.util.HashSet;
 import java.util.Hashtable;
+import java.util.Set;
+import java.util.stream.Stream;
 import org.eclipse.osgi.internal.debug.Debug;
 import org.osgi.framework.BundleContext;
 import org.osgi.framework.Constants;
@@ -38,29 +41,24 @@ import org.osgi.framework.ServiceRegistration;
  * A utility class with some generally useful static methods for adaptor hook implementations
  */
 public class StorageUtil {
-	/** The NULL tag used in bundle storage */
-	public static final byte NULL = 0;
-	/** The OBJECT tag used in bundle storage */
-	public static final byte OBJECT = 1;
 
 	/**
-	 * Does a recursive copy of one directory to another.
-	 * @param inDir input directory to copy.
-	 * @param outDir output directory to copy to.
+	 * Copies the content of the given path (file or directory) to the specified
+	 * target. If the source is a directory all contained elements are copied
+	 * recursively.
+	 * @param inFile  input directory to copy.
+	 * @param outFile output directory to copy to.
 	 * @throws IOException if any error occurs during the copy.
 	 */
-	public static void copyDir(File inDir, File outDir) throws IOException {
-		String[] files = inDir.list();
-		if (files != null && files.length > 0) {
-			outDir.mkdir();
-			for (String file : files) {
-				File inFile = new File(inDir, file);
-				File outFile = new File(outDir, file);
-				if (inFile.isDirectory()) {
-					copyDir(inFile, outFile);
-				} else {
-					InputStream in = new FileInputStream(inFile);
-					readFile(in, outFile);
+	public static void copy(File inFile, File outFile) throws IOException {
+		Path source = inFile.toPath();
+		Path target = outFile.toPath();
+		if (Files.exists(source)) {
+			Files.createDirectories(target.getParent());
+			try (Stream<Path> walk = Files.walk(source)) {
+				for (Path s : (Iterable<Path>) walk::iterator) {
+					Path t = target.resolve(source.relativize(s));
+					Files.copy(s, t, StandardCopyOption.REPLACE_EXISTING);
 				}
 			}
 		}
@@ -74,32 +72,7 @@ public class StorageUtil {
 	 * @exception IOException
 	 */
 	public static void readFile(InputStream in, File file) throws IOException {
-		FileOutputStream fos = null;
-		try {
-			fos = new FileOutputStream(file);
-
-			byte buffer[] = new byte[1024];
-			int count;
-			while ((count = in.read(buffer, 0, buffer.length)) > 0) {
-				fos.write(buffer, 0, count);
-			}
-		} finally {
-			if (in != null) {
-				try {
-					in.close();
-				} catch (IOException ee) {
-					// nothing to do here
-				}
-			}
-
-			if (fos != null) {
-				try {
-					fos.close();
-				} catch (IOException ee) {
-					// nothing to do here
-				}
-			}
-		}
+		Files.copy(in, file.toPath(), StandardCopyOption.REPLACE_EXISTING);
 	}
 
 	/**
@@ -109,51 +82,39 @@ public class StorageUtil {
 	 * @return false is the specified files still exists, true otherwise.
 	 */
 	public static boolean rm(File file, boolean DEBUG) {
-		if (file.exists()) {
-			if (file.isDirectory()) {
-				String list[] = file.list();
-				if (list != null) {
-					int len = list.length;
-					for (int i = 0; i < len; i++) {
-						// we are doing a lot of garbage collecting here
-						rm(new File(file, list[i]), DEBUG);
-					}
-				}
-			}
-			if (DEBUG) {
-				if (file.isDirectory()) {
-					Debug.println("rmdir " + file.getPath()); //$NON-NLS-1$
-				} else {
-					Debug.println("rm " + file.getPath()); //$NON-NLS-1$
+		Path path = file.toPath();
+		if (!Files.exists(path)) {
+			return true;
+		}
+		try {
+			Files.walkFileTree(path, new SimpleFileVisitor<Path>() {
+				@Override
+				public FileVisitResult visitFile(Path f, BasicFileAttributes attrs) {
+					return delete(f, DEBUG);
 				}
-			}
-
-			boolean success = file.delete();
 
-			if (DEBUG) {
-				if (!success) {
-					Debug.println("  rm failed!"); //$NON-NLS-1$
+				@Override
+				public FileVisitResult postVisitDirectory(Path dir, IOException exc) {
+					return delete(dir, DEBUG);
 				}
-			}
 
-			return (success);
-		}
-		return (true);
-	}
-
-	public static String readString(DataInputStream in, boolean intern) throws IOException {
-		byte type = in.readByte();
-		if (type == NULL)
-			return null;
-		return intern ? in.readUTF().intern() : in.readUTF();
-	}
-
-	public static void writeStringOrNull(DataOutputStream out, String string) throws IOException {
-		if (string == null)
-			out.writeByte(NULL);
-		else {
-			out.writeByte(OBJECT);
-			out.writeUTF(string);
+				private FileVisitResult delete(Path pathToDelete, boolean debug) {
+					try {
+						if (debug) {
+							Debug.println("rm " + pathToDelete); //$NON-NLS-1$
+						}
+						Files.delete(pathToDelete);
+					} catch (IOException e) {
+						if (debug) {
+							Debug.println("  rm failed:" + e.getMessage()); //$NON-NLS-1$
+						}
+					}
+					return FileVisitResult.CONTINUE;
+				}
+			});
+			return !Files.exists(path);
+		} catch (IOException e) {
+			return false;
 		}
 	}
 
@@ -259,42 +220,28 @@ public class StorageUtil {
 		}
 	}
 
-	private static final boolean IS_WINDOWS = "\\\\.\\NUL" //$NON-NLS-1$
-			.equals(getDeviceName(new File("NUL"))); //$NON-NLS-1$
-
-	private static String getDeviceName(File file) {
-		try {
-			return file.getCanonicalPath();
-		} catch (IOException e) {
-			return null;
-		}
-	}
+	private static final boolean IS_WINDOWS = File.separatorChar == '\\';
 
 	// reserved names according to
 	// https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file
-	private static HashSet<String> RESERVED_NAMES = new HashSet<>(
-			Arrays.asList(new String[] { "aux", "com1", "com2", "com3", "com4", //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$
-					"com5", "com6", "com7", "com8", "com9", "con", "lpt1", "lpt2", //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$ //$NON-NLS-6$ //$NON-NLS-7$ //$NON-NLS-8$
-					"lpt3", "lpt4", "lpt5", "lpt6", "lpt7", "lpt8", "lpt9", "nul", "prn" })); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$ //$NON-NLS-6$ //$NON-NLS-7$ //$NON-NLS-8$ //$NON-NLS-9$
+	private static final Set<String> RESERVED_NAMES = new HashSet<>(Arrays.asList("aux", "com1", "com2", "com3", "com4", //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$
+			"com5", "com6", "com7", "com8", "com9", "con", "lpt1", "lpt2", //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$ //$NON-NLS-6$ //$NON-NLS-7$ //$NON-NLS-8$
+			"lpt3", "lpt4", "lpt5", "lpt6", "lpt7", "lpt8", "lpt9", "nul", "prn")); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$ //$NON-NLS-6$ //$NON-NLS-7$ //$NON-NLS-8$ //$NON-NLS-9$
 
 	/** Tests whether the filename can escape path into special device **/
 	public static boolean isReservedFileName(File file) {
 		// Directory names are not checked here because illegal directory names will be
 		// handled by OS.
+		if (!IS_WINDOWS) { // only windows has special file names which can escape any path
+			return false;
+		}
 		String fileName = file.getName();
-		return isReservedFileName(fileName);
-	}
-
-	private static boolean isReservedFileName(String name) {
 		// Illegal characters are not checked here because they are check by both JDK
 		// and OS. This is only a check against technical allowed but unwanted device
 		// names.
-		if (!IS_WINDOWS) { // only windows has special file names which can escape any path
-			return false;
-		}
-		int dot = name.indexOf('.');
+		int dot = fileName.indexOf('.');
 		// on windows, filename suffixes are not relevant to name validity
-		String basename = dot == -1 ? name : name.substring(0, dot);
+		String basename = dot == -1 ? fileName : fileName.substring(0, dot);
 		return RESERVED_NAMES.contains(basename.toLowerCase());
 	}
 
diff --git a/bundles/org.eclipse.osgi/pom.xml b/bundles/org.eclipse.osgi/pom.xml
index 5069cb59f..3ee2061e9 100644
--- a/bundles/org.eclipse.osgi/pom.xml
+++ b/bundles/org.eclipse.osgi/pom.xml
@@ -19,7 +19,7 @@
   </parent>
   <groupId>org.eclipse.osgi</groupId>
   <artifactId>org.eclipse.osgi</artifactId>
-  <version>3.17.100-SNAPSHOT</version>
+  <version>3.17.200-SNAPSHOT</version>
   <packaging>eclipse-plugin</packaging>
 
   <build>