From 41ce90ec360a46cb7b03f7cb8b66e01a113aa5b6 Mon Sep 17 00:00:00 2001 From: Zack Shoylev Date: Mon, 26 Oct 2015 13:51:40 -0500 Subject: [PATCH] Fixes tests failing on windows filesystems Makes windows behavior more consistent, especially for deletes --- .../FilesystemBlobStoreContextModule.java | 8 +- .../FilesystemStorageStrategyImpl.java | 48 ++++---- .../org/jclouds/filesystem/util/Utils.java | 54 +++++++-- .../filesystem/FilesystemBlobStoreTest.java | 10 +- .../FilesystemBlobIntegrationTest.java | 10 +- .../FilesystemContainerIntegrationTest.java | 3 +- .../FilesystemStorageStrategyImplTest.java | 17 ++- .../internal/BaseBlobIntegrationTest.java | 2 + .../java/org/jclouds/utils/TestUtils.java | 105 +++++++++--------- 9 files changed, 157 insertions(+), 100 deletions(-) diff --git a/apis/filesystem/src/main/java/org/jclouds/filesystem/config/FilesystemBlobStoreContextModule.java b/apis/filesystem/src/main/java/org/jclouds/filesystem/config/FilesystemBlobStoreContextModule.java index 9294450cce..cd3337b4b4 100644 --- a/apis/filesystem/src/main/java/org/jclouds/filesystem/config/FilesystemBlobStoreContextModule.java +++ b/apis/filesystem/src/main/java/org/jclouds/filesystem/config/FilesystemBlobStoreContextModule.java @@ -16,6 +16,8 @@ */ package org.jclouds.filesystem.config; +import static org.jclouds.filesystem.util.Utils.isWindows; + import org.jclouds.blobstore.BlobRequestSigner; import org.jclouds.blobstore.BlobStore; import org.jclouds.blobstore.LocalBlobRequestSigner; @@ -39,7 +41,11 @@ public class FilesystemBlobStoreContextModule extends AbstractModule { protected void configure() { bind(BlobStore.class).to(LocalBlobStore.class); install(new BlobStoreObjectModule()); - bind(ConsistencyModel.class).toInstance(ConsistencyModel.STRICT); + if (isWindows()) { + bind(ConsistencyModel.class).toInstance(ConsistencyModel.EVENTUAL); + } else { + bind(ConsistencyModel.class).toInstance(ConsistencyModel.STRICT); + } bind(LocalStorageStrategy.class).to(FilesystemStorageStrategyImpl.class); bind(BlobUtils.class).to(FileSystemBlobUtilsImpl.class); bind(FilesystemBlobKeyValidator.class).to(FilesystemBlobKeyValidatorImpl.class); diff --git a/apis/filesystem/src/main/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImpl.java b/apis/filesystem/src/main/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImpl.java index 4781ab1fcf..1f14d68b01 100644 --- a/apis/filesystem/src/main/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImpl.java +++ b/apis/filesystem/src/main/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImpl.java @@ -24,6 +24,7 @@ import static java.nio.file.Files.getPosixFilePermissions; import static java.nio.file.Files.probeContentType; import static java.nio.file.Files.readAttributes; import static java.nio.file.Files.setPosixFilePermissions; +import static org.jclouds.filesystem.util.Utils.delete; import static org.jclouds.filesystem.util.Utils.isPrivate; import static org.jclouds.filesystem.util.Utils.isWindows; import static org.jclouds.filesystem.util.Utils.setPrivate; @@ -454,7 +455,7 @@ public class FilesystemStorageStrategyImpl implements LocalStorageStrategy { try { Files.createParentDirs(outputFile); his = new HashingInputStream(Hashing.md5(), payload.openStream()); - outputFile.delete(); + delete(outputFile); Files.asByteSink(outputFile).writeFrom(his); HashCode actualHashCode = his.hash(); HashCode expectedHashCode = payload.getContentMetadata().getContentMD5AsHashCode(); @@ -477,8 +478,10 @@ public class FilesystemStorageStrategyImpl implements LocalStorageStrategy { return base16().lowerCase().encode(actualHashCode.asBytes()); } catch (IOException ex) { if (outputFile != null) { - if (!outputFile.delete()) { - logger.debug("Could not delete %s", outputFile); + try { + delete(outputFile); + } catch (IOException e) { + logger.debug("Could not delete %s: %s", outputFile, e); } } throw ex; @@ -497,23 +500,26 @@ public class FilesystemStorageStrategyImpl implements LocalStorageStrategy { String fileName = buildPathStartingFromBaseDir(container, blobKey); logger.debug("Deleting blob %s", fileName); File fileToBeDeleted = new File(fileName); - if (!fileToBeDeleted.delete()) { - if (fileToBeDeleted.isDirectory()) { - try { - UserDefinedFileAttributeView view = getUserDefinedFileAttributeView(fileToBeDeleted.toPath()); - if (view != null) { - for (String s : view.list()) { - view.delete(s); - } + + if (fileToBeDeleted.isDirectory()) { + try { + UserDefinedFileAttributeView view = getUserDefinedFileAttributeView(fileToBeDeleted.toPath()); + if (view != null) { + for (String s : view.list()) { + view.delete(s); } - } catch (IOException e) { - logger.debug("Could not delete attributes from %s", fileToBeDeleted); } - } else { - logger.debug("Could not delete %s", fileToBeDeleted); + } catch (IOException e) { + logger.debug("Could not delete attributes from %s: %s", fileToBeDeleted, e); } } + try { + delete(fileToBeDeleted); + } catch (IOException e) { + logger.debug("Could not delete %s: %s", fileToBeDeleted, e); + } + // now examine if the key of the blob is a complex key (with a directory structure) // and eventually remove empty directory removeDirectoriesTreeOfBlobKey(container, blobKey); @@ -737,10 +743,10 @@ public class FilesystemStorageStrategyImpl implements LocalStorageStrategy { } /** - * Remove leading and trailing {@link File.separator} character from the string. + * Remove leading and trailing separator character from the string. * * @param pathToBeCleaned - * @param remove + * @param onlyTrailing * only trailing separator char from path * @return */ @@ -767,7 +773,7 @@ public class FilesystemStorageStrategyImpl implements LocalStorageStrategy { * empty * * @param container - * @param normalizedKey + * @param blobKey */ private void removeDirectoriesTreeOfBlobKey(String container, String blobKey) { String normalizedBlobKey = denormalize(blobKey); @@ -786,8 +792,10 @@ public class FilesystemStorageStrategyImpl implements LocalStorageStrategy { File directory = new File(buildPathStartingFromBaseDir(container, parentPath)); String[] children = directory.list(); if (null == children || children.length == 0) { - if (!directory.delete()) { - logger.debug("Could not delete %s", directory); + try { + delete(directory); + } catch (IOException e) { + logger.debug("Could not delete %s: %s", directory, e); return; } // recursively call for removing other path diff --git a/apis/filesystem/src/main/java/org/jclouds/filesystem/util/Utils.java b/apis/filesystem/src/main/java/org/jclouds/filesystem/util/Utils.java index cc258510aa..ec1f56aeb8 100644 --- a/apis/filesystem/src/main/java/org/jclouds/filesystem/util/Utils.java +++ b/apis/filesystem/src/main/java/org/jclouds/filesystem/util/Utils.java @@ -20,7 +20,10 @@ import static java.nio.file.FileSystems.getDefault; import java.io.File; import java.io.IOException; +import java.nio.file.AccessDeniedException; +import java.nio.file.DirectoryNotEmptyException; import java.nio.file.Files; +import java.nio.file.NoSuchFileException; import java.nio.file.Path; import java.nio.file.attribute.AclEntry; import java.nio.file.attribute.AclEntryPermission; @@ -29,6 +32,9 @@ import java.nio.file.attribute.AclFileAttributeView; import java.nio.file.attribute.UserPrincipal; import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.TimeUnit; + +import com.google.common.util.concurrent.Uninterruptibles; /** * Utilities for the filesystem blobstore. @@ -39,6 +45,21 @@ public class Utils { // Do nothing } + /** + * Determine if Java is running on a Mac OS + */ + public static boolean isMacOSX() { + String osName = System.getProperty("os.name"); + return osName.contains("OS X"); + } + + /** + * Determine if Java is running on a windows OS + */ + public static boolean isWindows() { + return System.getProperty("os.name", "").toLowerCase().contains("windows"); + } + /** Delete a file or a directory recursively. */ public static void deleteRecursively(File file) throws IOException { if (file.isDirectory()) { @@ -49,14 +70,33 @@ public class Utils { } } } - Files.delete(file.toPath()); + + delete(file); } - /** - * Determine if Java is running on a windows OS - */ - public static boolean isWindows() { - return System.getProperty("os.name", "").toLowerCase().contains("windows"); + public static void delete(File file) throws IOException { + for (int n = 0; n < 10; n++) { + try { + Files.delete(file.toPath()); + if (Files.exists(file.toPath())) { + Uninterruptibles.sleepUninterruptibly(200, TimeUnit.MILLISECONDS); + continue; + } + return; + } catch (DirectoryNotEmptyException dnee) { + // A previous file delete operation did not finish before this call + Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS); + continue; + } catch (AccessDeniedException ade) { + // The file was locked by antivirus, indexing, or another operation triggered by previous file modification + Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS); + continue; + } catch (NoSuchFileException nse) { + return; // The file has been eventually deleted after a previous operation that failed. no-op + } + } + // File could not be deleted multiple times. It is very likely locked in another process + throw new IOException("Could not delete: " + file.toPath()); } /** @@ -65,7 +105,7 @@ public class Utils { */ public static boolean isPrivate(Path path) throws IOException { UserPrincipal everyone = getDefault().getUserPrincipalLookupService() - .lookupPrincipalByName("Everyone"); + .lookupPrincipalByName("Everyone"); AclFileAttributeView aclFileAttributes = java.nio.file.Files.getFileAttributeView( path, AclFileAttributeView.class); for (AclEntry aclEntry : aclFileAttributes.getAcl()) { diff --git a/apis/filesystem/src/test/java/org/jclouds/filesystem/FilesystemBlobStoreTest.java b/apis/filesystem/src/test/java/org/jclouds/filesystem/FilesystemBlobStoreTest.java index 022ab93ed9..88704e3ced 100644 --- a/apis/filesystem/src/test/java/org/jclouds/filesystem/FilesystemBlobStoreTest.java +++ b/apis/filesystem/src/test/java/org/jclouds/filesystem/FilesystemBlobStoreTest.java @@ -17,6 +17,7 @@ package org.jclouds.filesystem; import static com.google.common.io.BaseEncoding.base16; +import static org.jclouds.filesystem.util.Utils.isMacOSX; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertNotNull; @@ -31,6 +32,7 @@ import java.net.URI; import java.util.Iterator; import java.util.Properties; import java.util.Set; +import java.util.concurrent.TimeUnit; import org.jclouds.ContextBuilder; import org.jclouds.blobstore.BlobRequestSigner; @@ -64,6 +66,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.google.common.io.ByteSource; import com.google.common.io.Files; +import com.google.common.util.concurrent.Uninterruptibles; import com.google.inject.CreationException; @Test(groups = "unit", testName = "FilesystemBlobStoreTest", singleThreaded = true) @@ -417,7 +420,9 @@ public class FilesystemBlobStoreTest { assertTrue(result, "Blob " + BLOB_KEY2 + " doesn't exist"); // remove first blob + Uninterruptibles.sleepUninterruptibly(5, TimeUnit.SECONDS); blobStore.removeBlob(CONTAINER_NAME, BLOB_KEY1); + Uninterruptibles.sleepUninterruptibly(5, TimeUnit.SECONDS); result = blobStore.blobExists(CONTAINER_NAME, BLOB_KEY1); assertFalse(result, "Blob still exists"); // first file deleted, not the second @@ -491,6 +496,7 @@ public class FilesystemBlobStoreTest { assertTrue(blobStore.blobExists(CONTAINER_NAME, childKey)); blobStore.removeBlob(CONTAINER_NAME, parentKey); + Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS); assertFalse(blobStore.blobExists(CONTAINER_NAME, parentKey)); assertTrue(blobStore.blobExists(CONTAINER_NAME, childKey)); } @@ -827,7 +833,7 @@ public class FilesystemBlobStoreTest { * Creates a {@link Blob} object filled with data from a file * * @param keyName - * @param fileContent + * @param filePayload * @return */ private Blob createBlob(String keyName, File filePayload) { @@ -906,7 +912,7 @@ public class FilesystemBlobStoreTest { @DataProvider public Object[][] ignoreOnMacOSX() { - return org.jclouds.utils.TestUtils.isMacOSX() ? TestUtils.NO_INVOCATIONS + return isMacOSX() ? TestUtils.NO_INVOCATIONS : TestUtils.SINGLE_NO_ARG_INVOCATION; } } diff --git a/apis/filesystem/src/test/java/org/jclouds/filesystem/integration/FilesystemBlobIntegrationTest.java b/apis/filesystem/src/test/java/org/jclouds/filesystem/integration/FilesystemBlobIntegrationTest.java index 0452389bc7..2676fff0f4 100644 --- a/apis/filesystem/src/test/java/org/jclouds/filesystem/integration/FilesystemBlobIntegrationTest.java +++ b/apis/filesystem/src/test/java/org/jclouds/filesystem/integration/FilesystemBlobIntegrationTest.java @@ -16,6 +16,8 @@ */ package org.jclouds.filesystem.integration; +import static org.jclouds.filesystem.util.Utils.isMacOSX; + import java.io.IOException; import java.util.Map; import java.util.Properties; @@ -46,7 +48,7 @@ public class FilesystemBlobIntegrationTest extends BaseBlobIntegrationTest { // https://bugs.openjdk.java.net/browse/JDK-8030048 @Override public void checkContentMetadata(Blob blob) { - if (!org.jclouds.utils.TestUtils.isMacOSX()) { + if (!isMacOSX()) { super.checkContentMetadata(blob); } } @@ -55,7 +57,7 @@ public class FilesystemBlobIntegrationTest extends BaseBlobIntegrationTest { // https://bugs.openjdk.java.net/browse/JDK-8030048 @Override protected void checkContentDisposition(Blob blob, String contentDisposition) { - if (!org.jclouds.utils.TestUtils.isMacOSX()) { + if (!isMacOSX()) { super.checkContentDisposition(blob, contentDisposition); } } @@ -64,7 +66,7 @@ public class FilesystemBlobIntegrationTest extends BaseBlobIntegrationTest { // https://bugs.openjdk.java.net/browse/JDK-8030048 @Override protected void validateMetadata(BlobMetadata metadata) throws IOException { - if (!org.jclouds.utils.TestUtils.isMacOSX()) { + if (!isMacOSX()) { super.validateMetadata(metadata); } } @@ -81,7 +83,7 @@ public class FilesystemBlobIntegrationTest extends BaseBlobIntegrationTest { * uses to implement user metadata */ @Override protected void checkUserMetadata(Map userMetadata1, Map userMetadata2) { - if (!org.jclouds.utils.TestUtils.isMacOSX()) { + if (!isMacOSX()) { super.checkUserMetadata(userMetadata1, userMetadata2); } } diff --git a/apis/filesystem/src/test/java/org/jclouds/filesystem/integration/FilesystemContainerIntegrationTest.java b/apis/filesystem/src/test/java/org/jclouds/filesystem/integration/FilesystemContainerIntegrationTest.java index 6a89d5e429..dad14fa6c8 100644 --- a/apis/filesystem/src/test/java/org/jclouds/filesystem/integration/FilesystemContainerIntegrationTest.java +++ b/apis/filesystem/src/test/java/org/jclouds/filesystem/integration/FilesystemContainerIntegrationTest.java @@ -17,6 +17,7 @@ package org.jclouds.filesystem.integration; import static org.jclouds.blobstore.options.ListContainerOptions.Builder.maxResults; +import static org.jclouds.filesystem.util.Utils.isMacOSX; import static org.testng.Assert.assertEquals; import java.io.IOException; @@ -164,7 +165,7 @@ public class FilesystemContainerIntegrationTest extends BaseContainerIntegration @DataProvider public Object[][] ignoreOnMacOSX() { - return org.jclouds.utils.TestUtils.isMacOSX() ? TestUtils.NO_INVOCATIONS + return isMacOSX() ? TestUtils.NO_INVOCATIONS : TestUtils.SINGLE_NO_ARG_INVOCATION; } diff --git a/apis/filesystem/src/test/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImplTest.java b/apis/filesystem/src/test/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImplTest.java index af0f67ded6..c16c9b12c8 100644 --- a/apis/filesystem/src/test/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImplTest.java +++ b/apis/filesystem/src/test/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImplTest.java @@ -17,6 +17,7 @@ package org.jclouds.filesystem.strategy.internal; import static com.google.common.io.BaseEncoding.base16; +import static org.jclouds.filesystem.util.Utils.isMacOSX; import static org.jclouds.utils.TestUtils.randomByteSource; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; @@ -30,6 +31,7 @@ import java.io.IOException; import java.util.Iterator; import java.util.List; import java.util.Set; +import java.util.concurrent.TimeUnit; import javax.inject.Provider; @@ -55,6 +57,7 @@ import com.google.common.hash.HashCode; import com.google.common.hash.Hashing; import com.google.common.io.ByteSource; import com.google.common.io.Files; +import com.google.common.util.concurrent.Uninterruptibles; /** * Test class for {@link FilesystemStorageStrategyImpl } class @@ -161,14 +164,6 @@ public class FilesystemStorageStrategyImplTest { TestUtils.directoryExists(TARGET_CONTAINER_NAME, false); } - public void testDeleteDirectory_ErrorWhenNotExists() { - try { - storageStrategy.deleteDirectory(CONTAINER_NAME, null); - fail("No exception throwed"); - } catch (Exception e) { - } - } - public void testDirectoryExists() throws IOException { final String SUBDIRECTORY_NAME = "ad" + FS + "sda" + FS + "asd"; boolean result; @@ -432,6 +427,7 @@ public class FilesystemStorageStrategyImplTest { storageStrategy.putBlob(CONTAINER_NAME, storageStrategy.newBlob(childKey)); storageStrategy.removeBlob(CONTAINER_NAME, parentKey); + Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS); assertFalse(storageStrategy.blobExists(CONTAINER_NAME, parentKey)); assertTrue(storageStrategy.blobExists(CONTAINER_NAME, childKey)); } @@ -644,6 +640,7 @@ public class FilesystemStorageStrategyImplTest { .payload(randomByteSource().slice(0, 1024)) // no metadata .build(); + Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS); storageStrategy.putBlob(CONTAINER_NAME, blob); blob = storageStrategy.getBlob(CONTAINER_NAME, blobKey); @@ -687,13 +684,13 @@ public class FilesystemStorageStrategyImplTest { @DataProvider public Object[][] ignoreOnMacOSX() { - return org.jclouds.utils.TestUtils.isMacOSX() ? TestUtils.NO_INVOCATIONS + return isMacOSX() ? TestUtils.NO_INVOCATIONS : TestUtils.SINGLE_NO_ARG_INVOCATION; } @DataProvider public Object[][] onlyOnMacOSX() { - return org.jclouds.utils.TestUtils.isMacOSX() ? + return isMacOSX() ? TestUtils.SINGLE_NO_ARG_INVOCATION : TestUtils.NO_INVOCATIONS; } } diff --git a/blobstore/src/test/java/org/jclouds/blobstore/integration/internal/BaseBlobIntegrationTest.java b/blobstore/src/test/java/org/jclouds/blobstore/integration/internal/BaseBlobIntegrationTest.java index f319ac54eb..cdcdec6411 100644 --- a/blobstore/src/test/java/org/jclouds/blobstore/integration/internal/BaseBlobIntegrationTest.java +++ b/blobstore/src/test/java/org/jclouds/blobstore/integration/internal/BaseBlobIntegrationTest.java @@ -799,7 +799,9 @@ public class BaseBlobIntegrationTest extends BaseBlobStoreIntegrationTest { blob.getMetadata().getUserMetadata().put("Adrian", "wonderpuff"); blob.getMetadata().getUserMetadata().put("Adrian", "powderpuff"); + awaitConsistency(); addBlobToContainer(container, blob); + awaitConsistency(); validateMetadata(view.getBlobStore().blobMetadata(container, name)); } finally { diff --git a/core/src/test/java/org/jclouds/utils/TestUtils.java b/core/src/test/java/org/jclouds/utils/TestUtils.java index 90d7310ec3..7082346154 100644 --- a/core/src/test/java/org/jclouds/utils/TestUtils.java +++ b/core/src/test/java/org/jclouds/utils/TestUtils.java @@ -16,8 +16,8 @@ */ package org.jclouds.utils; -import java.io.InputStream; import java.io.IOException; +import java.io.InputStream; import java.util.Random; import com.google.common.io.ByteSource; @@ -27,69 +27,64 @@ import com.google.common.io.ByteSource; */ public class TestUtils { - public static boolean isMacOSX() { - String osName = System.getProperty("os.name"); - return osName.contains("OS X"); - } + public static boolean isJava6() { + return System.getProperty("java.version", "").contains("1.6."); + } - public static boolean isJava6() { - return System.getProperty("java.version", "").contains("1.6."); - } + public static ByteSource randomByteSource() { + return randomByteSource(0); + } - public static ByteSource randomByteSource() { - return randomByteSource(0); - } + public static ByteSource randomByteSource(long seed) { + return new RandomByteSource(seed); + } - public static ByteSource randomByteSource(long seed) { - return new RandomByteSource(seed); - } + private static class RandomByteSource extends ByteSource { + private final long seed; - private static class RandomByteSource extends ByteSource { - private final long seed; + RandomByteSource(long seed) { + this.seed = seed; + } - RandomByteSource(long seed) { - this.seed = seed; - } + @Override + public InputStream openStream() { + return new RandomInputStream(seed); + } + } - @Override - public InputStream openStream() { - return new RandomInputStream(seed); - } - } + private static class RandomInputStream extends InputStream { + private final Random random; + private boolean closed = false; - private static class RandomInputStream extends InputStream { - private final Random random; - private boolean closed = false; + RandomInputStream(long seed) { + this.random = new Random(seed); + } - RandomInputStream(long seed) { - this.random = new Random(seed); - } + @Override + public synchronized int read() throws IOException { + if (closed) { + throw new IOException("Stream already closed"); + } + return (byte) random.nextInt(); + } - @Override - public synchronized int read() throws IOException { - if (closed) { - throw new IOException("Stream already closed"); - } - return (byte) random.nextInt(); - } + @Override + public synchronized int read(byte[] b) throws IOException { + return read(b, 0, b.length); + } - @Override - public synchronized int read(byte[] b) throws IOException { - return read(b, 0, b.length); - } + @Override + public synchronized int read(byte[] b, int off, int len) throws IOException { + for (int i = 0; i < len; ++i) { + b[off + i] = (byte) read(); + } + return len; + } - @Override - public synchronized int read(byte[] b, int off, int len) throws IOException { - for (int i = 0; i < len; ++i) { - b[off + i] = (byte) read(); - } - return len; - } - - @Override - public void close() throws IOException { - super.close(); - closed = true; - } - } + @Override + public void close() throws IOException { + super.close(); + closed = true; + } + } }