You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@brooklyn.apache.org by he...@apache.org on 2014/07/14 17:36:00 UTC
[04/11] git commit: Fix occasionally failing temporary file and
folder creation.
Fix occasionally failing temporary file and folder creation.
The previous implementation wasn't deterministic, instead counting
on the statistical improbability of generating an existing file/folder name.
This coupled with the fact that the random generator is weak (repeats
id within 2000 attempts) made it likely to generate an existing name,
leading to unexpected exception.
The solution is to use the Java provided functionality where possible
and retry until success otherwise.
Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/cec9243a
Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/cec9243a
Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/cec9243a
Branch: refs/heads/master
Commit: cec9243ade58f7e2d247aa16e5d425be8422da84
Parents: b4d1df9
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Authored: Thu Jul 10 16:57:52 2014 +0300
Committer: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Committed: Thu Jul 10 16:57:52 2014 +0300
----------------------------------------------------------------------
.../src/main/java/brooklyn/util/os/Os.java | 41 +++++++++++------
.../java/brooklyn/util/text/Identifiers.java | 3 ++
.../src/test/java/brooklyn/util/os/OsTest.java | 48 ++++++++++++++++++++
3 files changed, 77 insertions(+), 15 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/cec9243a/utils/common/src/main/java/brooklyn/util/os/Os.java
----------------------------------------------------------------------
diff --git a/utils/common/src/main/java/brooklyn/util/os/Os.java b/utils/common/src/main/java/brooklyn/util/os/Os.java
index f0ef9dd..eab6be0 100644
--- a/utils/common/src/main/java/brooklyn/util/os/Os.java
+++ b/utils/common/src/main/java/brooklyn/util/os/Os.java
@@ -56,6 +56,8 @@ public class Os {
private static final Logger log = LoggerFactory.getLogger(Os.class);
+ private static final int TEMP_DIR_ATTEMPTS = 1000;
+
private static final char SEPARATOR_UNIX = '/';
private static final char SEPARATOR_WIN = '\\';
@@ -523,17 +525,16 @@ public class Os {
* either prefix or ext may be null;
* if ext is non-empty and not > 4 chars and not starting with a ., then a dot will be inserted */
public static File newTempFile(String prefix, String ext) {
- String baseName = (prefix!=null ? prefix + "-" : "") + Identifiers.makeRandomId(4) +
- (ext!=null ? ext.startsWith(".") || ext.length()>4 ? ext : "."+ext : "");
- File tempFile = new File(tmp(), baseName);
+ String sanitizedPrefix = (prefix!=null ? prefix + "-" : "");
+ String sanitizedExt = (ext!=null ? ext.startsWith(".") || ext.length()>4 ? ext : "."+ext : "");
try {
- if (tempFile.createNewFile()) {
- tempFile.deleteOnExit();
- return tempFile;
- }
- throw new IllegalStateException("cannot create temp file "+tempFile+", call returned false");
+ File tempFile = File.createTempFile(sanitizedPrefix, sanitizedExt, new File(tmp()));
+ tempFile.deleteOnExit();
+ return tempFile;
} catch (IOException e) {
- throw new IllegalStateException("cannot create temp file "+tempFile+", error: "+e, e);
+ Exceptions.propagate(e);
+ //non-reachable, prevent uninitialized warning in following code
+ return null;
}
}
@@ -544,13 +545,23 @@ public class Os {
/** creates a temp dir which will be deleted on exit */
public static File newTempDir(String prefix) {
- String baseName = (prefix==null ? "" : prefix + "-") + Identifiers.makeRandomId(4);
- File tempDir = new File(tmp(), baseName);
- if (tempDir.mkdir()) {
- Os.deleteOnExitRecursively(tempDir);
- return tempDir;
+ String sanitizedPrefix = (prefix==null ? "" : prefix + "-");
+ String tmpParent = tmp();
+ for (int i = 0; i < TEMP_DIR_ATTEMPTS; i++) {
+ String baseName = sanitizedPrefix + Identifiers.makeRandomId(4);
+ File tempDir = new File(tmpParent, baseName);
+ if (!tempDir.exists()) {
+ if (tempDir.mkdir()) {
+ Os.deleteOnExitRecursively(tempDir);
+ return tempDir;
+ } else {
+ log.warn("Attempt to create temp dir failed " + tempDir);
+ }
+ } else {
+ log.debug("Attemp to create temp dir failed, already exists " + tempDir);
+ }
}
- throw new IllegalStateException("cannot write to temp dir, making directory "+tempDir);
+ throw new IllegalStateException("cannot create temporary folders in parent " + tmpParent + " after " + TEMP_DIR_ATTEMPTS + " attempts.");
}
/** as {@link #newTempDir(String)}, using the class as the basis for a prefix */
http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/cec9243a/utils/common/src/main/java/brooklyn/util/text/Identifiers.java
----------------------------------------------------------------------
diff --git a/utils/common/src/main/java/brooklyn/util/text/Identifiers.java b/utils/common/src/main/java/brooklyn/util/text/Identifiers.java
index 0ef34f9..11bda41 100644
--- a/utils/common/src/main/java/brooklyn/util/text/Identifiers.java
+++ b/utils/common/src/main/java/brooklyn/util/text/Identifiers.java
@@ -51,6 +51,9 @@ public class Identifiers {
* in general this is preferable to base64 as is more portable,
* can be used throughout javascript (as ID's which don't allow +)
* or as java identifiers (which don't allow numbers in the first char)
+ *
+ * WARNING: The method is not as random as advertised!
+ * Highly probable to return the same id in the first 2000 attempts.
**/
public static String makeRandomId(int l) {
//this version is 30-50% faster than the old double-based one,
http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/cec9243a/utils/common/src/test/java/brooklyn/util/os/OsTest.java
----------------------------------------------------------------------
diff --git a/utils/common/src/test/java/brooklyn/util/os/OsTest.java b/utils/common/src/test/java/brooklyn/util/os/OsTest.java
index 841b7ba..54f39af 100644
--- a/utils/common/src/test/java/brooklyn/util/os/OsTest.java
+++ b/utils/common/src/test/java/brooklyn/util/os/OsTest.java
@@ -21,12 +21,16 @@ package brooklyn.util.os;
import static org.testng.Assert.assertEquals;
import java.io.File;
+import java.util.ArrayList;
+import java.util.Collection;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testng.Assert;
import org.testng.annotations.Test;
+import brooklyn.util.exceptions.Exceptions;
+
import com.google.common.collect.ImmutableSet;
@Test
@@ -84,5 +88,49 @@ public class OsTest {
assertEquals(Os.mergePaths("a", "b/"), "a/b/");
assertEquals(Os.mergePaths("/a", "b"), "/a/b");
}
+
+ @Test
+ public void testNewTempFile() {
+ int CREATE_CNT = 1500;
+ Collection<File> folders = new ArrayList<File>(CREATE_CNT);
+
+ try {
+ for (int i = 0; i < CREATE_CNT; i++) {
+ try {
+ folders.add(Os.newTempFile(OsTest.class, "test"));
+ } catch (IllegalStateException e) {
+ log.warn("testNewTempFile failed at " + i + " iteration.");
+ Exceptions.propagate(e);
+ }
+ }
+ } finally {
+ //cleanup
+ for (File folder : folders) {
+ folder.delete();
+ }
+ }
+ }
+
+ @Test
+ public void testNewTempDir() {
+ int CREATE_CNT = 2000;
+ Collection<File> folders = new ArrayList<File>(CREATE_CNT);
+
+ try {
+ for (int i = 0; i < CREATE_CNT; i++) {
+ try {
+ folders.add(Os.newTempDir(OsTest.class));
+ } catch (IllegalStateException e) {
+ log.warn("testNewTempDir failed at " + i + " iteration.");
+ Exceptions.propagate(e);
+ }
+ }
+ } finally {
+ //cleanup
+ for (File folder : folders) {
+ folder.delete();
+ }
+ }
+ }
}