Bug 561152 - Make move operation more robust

For staging files into the storage area use Files.move to try atomic
moves.  Fall back to non-atomic on failure.  Also use replace option in
case the target exists.

Change-Id: Icd645470b25979dc76a7a1d9ea12b3e4837d634d
Signed-off-by: Thomas Watson <tjwatson@us.ibm.com>

6 files changed, 100 insertions, 59 deletions

diff --git a/bundles/org.eclipse.osgi.tests/.settings/org.eclipse.jdt.ui.prefs b/bundles/org.eclipse.osgi.tests/.settings/org.eclipse.jdt.ui.prefs
index adadf81b4..ad98edb62 100644
--- a/bundles/org.eclipse.osgi.tests/.settings/org.eclipse.jdt.ui.prefs
+++ b/bundles/org.eclipse.osgi.tests/.settings/org.eclipse.jdt.ui.prefs
@@ -10,33 +10,45 @@ sp_cleanup.add_default_serial_version_id=true
 sp_cleanup.add_generated_serial_version_id=false
 sp_cleanup.add_missing_annotations=true
 sp_cleanup.add_missing_deprecated_annotations=true
+sp_cleanup.add_missing_methods=false
 sp_cleanup.add_missing_nls_tags=false
 sp_cleanup.add_missing_override_annotations=true
+sp_cleanup.add_missing_override_annotations_interface_methods=false
 sp_cleanup.add_serial_version_id=false
 sp_cleanup.always_use_blocks=true
 sp_cleanup.always_use_parentheses_in_expressions=false
 sp_cleanup.always_use_this_for_non_static_field_access=false
 sp_cleanup.always_use_this_for_non_static_method_access=false
+sp_cleanup.convert_functional_interfaces=false
 sp_cleanup.convert_to_enhanced_for_loop=false
 sp_cleanup.correct_indentation=false
 sp_cleanup.format_source_code=true
+sp_cleanup.format_source_code_changes_only=true
+sp_cleanup.insert_inferred_type_arguments=false
 sp_cleanup.make_local_variable_final=false
 sp_cleanup.make_parameters_final=false
 sp_cleanup.make_private_fields_final=true
+sp_cleanup.make_type_abstract_if_missing_method=false
 sp_cleanup.make_variable_declarations_final=true
 sp_cleanup.never_use_blocks=false
 sp_cleanup.never_use_parentheses_in_expressions=true
+sp_cleanup.number_suffix=false
 sp_cleanup.on_save_use_additional_actions=false
 sp_cleanup.organize_imports=true
+sp_cleanup.push_down_negation=false
 sp_cleanup.qualify_static_field_accesses_with_declaring_class=false
 sp_cleanup.qualify_static_member_accesses_through_instances_with_declaring_class=true
 sp_cleanup.qualify_static_member_accesses_through_subtypes_with_declaring_class=true
 sp_cleanup.qualify_static_member_accesses_with_declaring_class=false
 sp_cleanup.qualify_static_method_accesses_with_declaring_class=false
 sp_cleanup.remove_private_constructors=true
+sp_cleanup.remove_redundant_modifiers=false
+sp_cleanup.remove_redundant_semicolons=false
+sp_cleanup.remove_redundant_type_arguments=false
 sp_cleanup.remove_trailing_whitespaces=false
 sp_cleanup.remove_trailing_whitespaces_all=true
 sp_cleanup.remove_trailing_whitespaces_ignore_empty=false
+sp_cleanup.remove_unnecessary_array_creation=false
 sp_cleanup.remove_unnecessary_casts=true
 sp_cleanup.remove_unnecessary_nls_tags=false
 sp_cleanup.remove_unused_imports=false
@@ -45,12 +57,18 @@ sp_cleanup.remove_unused_private_fields=true
 sp_cleanup.remove_unused_private_members=false
 sp_cleanup.remove_unused_private_methods=true
 sp_cleanup.remove_unused_private_types=true
+sp_cleanup.simplify_lambda_expression_and_method_ref=false
 sp_cleanup.sort_members=false
 sp_cleanup.sort_members_all=false
+sp_cleanup.use_anonymous_class_creation=false
+sp_cleanup.use_autoboxing=false
 sp_cleanup.use_blocks=false
 sp_cleanup.use_blocks_only_for_return_and_throw=false
+sp_cleanup.use_directly_map_method=false
+sp_cleanup.use_lambda=false
 sp_cleanup.use_parentheses_in_expressions=false
 sp_cleanup.use_this_for_non_static_field_access=false
 sp_cleanup.use_this_for_non_static_field_access_only_if_necessary=true
 sp_cleanup.use_this_for_non_static_method_access=false
 sp_cleanup.use_this_for_non_static_method_access_only_if_necessary=true
+sp_cleanup.use_unboxing=false
diff --git a/bundles/org.eclipse.osgi.tests/.settings/org.eclipse.ltk.core.refactoring.prefs b/bundles/org.eclipse.osgi.tests/.settings/org.eclipse.ltk.core.refactoring.prefs
new file mode 100644
index 000000000..b196c64a3
--- /dev/null
+++ b/bundles/org.eclipse.osgi.tests/.settings/org.eclipse.ltk.core.refactoring.prefs
@@ -0,0 +1,2 @@
+eclipse.preferences.version=1
+org.eclipse.ltk.core.refactoring.enable.project.refactoring.history=false
diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/SystemBundleTests.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/SystemBundleTests.java
index c1c089deb..890324f4f 100755
--- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/SystemBundleTests.java
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/SystemBundleTests.java
@@ -13,6 +13,10 @@
  *******************************************************************************/
 package org.eclipse.osgi.tests.bundles;
 
+import static java.nio.file.Files.createDirectories;
+import static java.nio.file.Files.createFile;
+import static java.nio.file.Files.write;
+
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileNotFoundException;
@@ -28,6 +32,7 @@ import java.net.URISyntaxException;
 import java.net.URL;
 import java.net.URLClassLoader;
 import java.net.URLConnection;
+import java.nio.file.Path;
 import java.security.Permission;
 import java.security.PrivilegedAction;
 import java.util.ArrayList;
@@ -4203,6 +4208,59 @@ public class SystemBundleTests extends AbstractBundleTests {
 		}
 	}
 
+	public void testCorruptStageInstallUpdate() throws IOException, BundleException {
+		File config = OSGiTestsActivator.getContext().getDataFile(getName()); // $NON-NLS-1$
+		final Equinox equinox = new Equinox(
+				Collections.singletonMap(Constants.FRAMEWORK_STORAGE, config.getAbsolutePath()));
+		try {
+			equinox.init();
+		} catch (BundleException e) {
+			fail("Unexpected exception in init()", e); //$NON-NLS-1$
+		}
+
+		File dirBundleFile = createBundle(config, "dir.bundle", false, true);
+		File jarBundleFile = createBundle(config, "jar.bundle", false, false);
+
+		// install dir bundle to get the path to storage
+		BundleContext bc = equinox.getBundleContext();
+		Bundle dirBundle = bc.installBundle(dirBundleFile.toURI().toASCIIString());
+		assertEquals("Wrong BSN", "bundledir.bundle", dirBundle.getSymbolicName());
+
+		URLConverter converter = bc.getService(bc.getServiceReference(URLConverter.class));
+		URL dirFileURL = converter.resolve(dirBundle.getEntry("/"));
+		File dirFile = new File(dirFileURL.getPath());
+		File rootStore = dirFile.getParentFile().getParentFile().getParentFile();
+		dirBundle.uninstall();
+
+		long next = dirBundle.getBundleId() + 1;
+		next = doTestExistingBundleFile(bc, next, rootStore, jarBundleFile, "bundlejar.bundle", true);
+		next = doTestExistingBundleFile(bc, next, rootStore, jarBundleFile, "bundlejar.bundle", false);
+		next = doTestExistingBundleFile(bc, next, rootStore, dirBundleFile, "bundledir.bundle", true);
+		next = doTestExistingBundleFile(bc, next, rootStore, dirBundleFile, "bundledir.bundle", false);
+
+	}
+
+	private long doTestExistingBundleFile(BundleContext bc, long next, File rootStore, File content, String bsn,
+			boolean d) throws IOException, BundleException {
+		createGenerationContent(next, rootStore, d);
+		Bundle b = bc.installBundle(content.toURI().toASCIIString());
+		assertEquals("Wrong BSN", bsn, b.getSymbolicName());
+		assertEquals("Wrong Bundle ID", next, b.getBundleId());
+		b.uninstall();
+		return b.getBundleId() + 1;
+	}
+
+	private Path createGenerationContent(long nextBundleID, File rootStore, boolean directory) throws IOException {
+		Path nextBundleFile = new File(rootStore, nextBundleID + "/0/bundleFile").toPath();
+		if (directory) {
+			createDirectories(nextBundleFile);
+			return write(createFile(new File(nextBundleFile.toFile(), "testContent.txt").toPath()),
+					"Some Content".getBytes());
+		}
+		createDirectories(nextBundleFile.getParent());
+		return write(createFile(nextBundleFile), "Some Content".getBytes());
+	}
+
 	// Note this is more of a performance test.  It has a timeout that will cause it to
 	// fail if it takes too long.
 	public void testMassiveParallelInstallStart() {
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/BundleInfo.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/BundleInfo.java
index 3a3014837..60133ef6c 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/BundleInfo.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/BundleInfo.java
@@ -354,16 +354,13 @@ public final class BundleInfo {
 			/* copy the entry to the cache */
 			File tempDest = File.createTempFile("staged", ".tmp", dir); //$NON-NLS-1$ //$NON-NLS-2$
 			StorageUtil.readFile(in, tempDest);
-			if (destination.exists() || !StorageUtil.move(tempDest, destination, getStorage().getConfiguration().getDebug().DEBUG_STORAGE)) {
+			if (destination.exists()) {
 				// maybe because some other thread already beat us there.
-				if (destination.exists()) {
-					// just delete our copy that could not get renamed
-					tempDest.delete();
-				} else {
-					throw new IOException("Failed to store the extracted content: " + destination); //$NON-NLS-1$
-				}
+				// just delete our staged copy
+				tempDest.delete();
+			} else {
+				StorageUtil.move(tempDest, destination, getStorage().getConfiguration().getDebug().DEBUG_STORAGE);
 			}
-
 			if (nativeCode) {
 				getBundleInfo().getStorage().setPermissions(destination);
 			}
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 f64df7d72..e6933f079 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
@@ -972,8 +972,11 @@ public class Storage {
 				throw new BundleException("Could not create generation directory: " + generationRoot.getAbsolutePath()); //$NON-NLS-1$
 			}
 			contentFile = new File(generationRoot, BUNDLE_FILE_NAME);
-			if (!StorageUtil.move(staged, contentFile, getConfiguration().getDebug().DEBUG_STORAGE)) {
-				throw new BundleException("Error while renaming bundle file to final location: " + contentFile); //$NON-NLS-1$
+			try {
+				StorageUtil.move(staged, contentFile, getConfiguration().getDebug().DEBUG_STORAGE);
+			} catch (IOException e) {
+				throw new BundleException("Error while renaming bundle file to final location: " + contentFile, //$NON-NLS-1$
+						BundleException.READ_ERROR, e);
 			}
 		} else {
 			contentFile = staged;
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 a47980df7..d286110d5 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
@@ -24,9 +24,9 @@ import java.io.InputStream;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.nio.file.Files;
+import java.nio.file.StandardCopyOption;
 import java.util.Dictionary;
 import java.util.Hashtable;
-import java.util.concurrent.TimeUnit;
 import org.eclipse.osgi.internal.debug.Debug;
 import org.osgi.framework.BundleContext;
 import org.osgi.framework.Constants;
@@ -219,58 +219,21 @@ public class StorageUtil {
 		return classbytes;
 	}
 
-	/**
-	 * To remain Java 6 compatible work around the unreliable renameTo() via
-	 * retries: http://bugs.java.com/view_bug.do?bug_id=6213298
-	 *
-	 * @param from
-	 * @param to
-	 */
-	public static boolean move(File from, File to, boolean DEBUG) {
-		// Try several attempts with incremental sleep
-		final int maxTries = 10;
-		final int sleepStep = 200;
-		for (int tryCount = 0, sleep = sleepStep;; sleep += sleepStep, tryCount++) {
-			if (from.renameTo(to)) {
-				return true;
-			}
-
-			if (DEBUG) {
-				Debug.println("move: failed to rename " + from + " to " + to + " (" + (maxTries - tryCount) + " attempts remaining)"); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$
-			}
-
-			if (tryCount >= maxTries) {
-				break;
-			}
-
-			try {
-				TimeUnit.MILLISECONDS.sleep(sleep);
-			} catch (InterruptedException e) {
-				// Ignore
-			}
-		}
-
-		// Try a copy
+	public static void move(File from, File to, boolean DEBUG) throws IOException {
 		try {
-			if (from.isDirectory()) {
-				copyDir(from, to);
-			} else {
-				readFile(new FileInputStream(from), to);
-			}
-
-			if (!rm(from, DEBUG)) {
-				Debug.println("move: failed to delete " + from + " after copy to " + to + ". Scheduling for delete on JVM exit."); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
-				from.deleteOnExit();
-			}
-			return true;
+			Files.move(from.toPath(), to.toPath(), StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE);
 		} catch (IOException e) {
 			if (DEBUG) {
-				Debug.println("move: failed to copy " + from + " to " + to); //$NON-NLS-1$ //$NON-NLS-2$
-				Debug.printStackTrace(e);
+				Debug.println("Failed to move atomically: " + from + " to " + to); //$NON-NLS-1$ //$NON-NLS-2$
 			}
-
-			// Give up
-			return false;
+			// remove in case it failed because the target to non-empty directory or
+			// the target type does not match the from
+			rm(to, DEBUG);
+			// also, try without atomic operation
+			Files.move(from.toPath(), to.toPath(), StandardCopyOption.REPLACE_EXISTING);
+		}
+		if (DEBUG) {
+			Debug.println("Successfully moved file: " + from + " to " + to); //$NON-NLS-1$ //$NON-NLS-2$
 		}
 	}
 }