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>
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 adadf81..ad98edb 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_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_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 0000000..b196c64
--- /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 c1c089d..890324f 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.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 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 3a30148..60133ef 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 @@
/* 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 f64df7d..e6933f0 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 @@
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 a47980d..d286110 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.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 @@
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$
}
}
}
