diff options
author | Thomas Watson | 2020-03-30 15:08:13 +0000 |
---|---|---|
committer | Thomas Watson | 2020-04-01 13:32:34 +0000 |
commit | 057efc46638dc24e2ed6cf5d5bddf2ae7f05d740 (patch) | |
tree | a650f3c31900bca6a92dcfbf1a3dd4e020cff74c | |
parent | d762ec7fd7e762d0885566db330986381997dfeb (diff) | |
download | rt.equinox.framework-057efc46638dc24e2ed6cf5d5bddf2ae7f05d740.tar.gz rt.equinox.framework-057efc46638dc24e2ed6cf5d5bddf2ae7f05d740.tar.xz rt.equinox.framework-057efc46638dc24e2ed6cf5d5bddf2ae7f05d740.zip |
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$ } } } |