Skip to contentSkip to conversation

Rate limit warnings spewed by RepositoryTestCase.recursiveDelete

RFC PATCH In review Jonas Fonseca 2 participants 4 messages Jun 15, 2016

lore
Share & tools
lore
#0 lore raw

RFC PATCH 3/3 Rate limit warnings spewed by RepositoryTestCase.recursiveDelete

On Windows XP / NTFS / NetBeans 6.1 / Java 5 a lot of warnings are printed. In most cases the path is in fact deleted and it seems to just be a timing bug or something Windows or NTFS specific. Also, many of the warnings are for the same paths and therefore a bit redundant. Furthermore, when a lot of warnings are printed it gives the feeling of slowing down the tests. By rate limiting the warnings the problem is still made visible for developers to notice.

---
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

This is mostly an annoyance fix, but might be acceptable.

org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java +3 -1
index 9d7d133..44e24d7 100644
@@ -51,6 +51,8 @@
 
 public abstract class RepositoryTestCase extends TestCase {
 
+	private static long deleteFailures;
+
 	protected final File trashParent = new File("trash");
 
 	protected File trash;
@@ -79,7 +81,7 @@ protected static void recursiveDelete(final File dir) {
 			}
 		}
 		dir.delete();
-		if (dir.exists()) {
+		if (dir.exists() && deleteFailures++ < 42) {
 			System.out.println("Warning: Failed to delete " + dir);
 		}
 	}
Signature · 4 lines
1.6.0.2.1166.g8d97a.dirty

-- 
Jonas Fonseca
#1 lore raw

onsdagen den 22 oktober 2008 10.34.20 skrev Jonas Fonseca:

Quoted 7 lines

On Windows XP / NTFS / NetBeans 6.1 / Java 5 a lot of warnings are printed. In most cases the path is in fact deleted and it seems to just be a timing bug or something Windows or NTFS specific. Also, many of the warnings are for the same paths and therefore a bit redundant. Furthermore, when a lot of warnings are printed it gives the feeling of slowing down the tests. By rate limiting the warnings the problem is still made visible for developers to notice.

The problem is actually flaws in the unit tests and in the supporting RepositoryTestCase. I think I'll fix that instead.

-- robin

#2 lore raw

Robin Rosenberg <[email protected]> wrote Fri, Oct 24, 2008:

Quoted 7 lines

onsdagen den 22 oktober 2008 10.34.20 skrev Jonas Fonseca:

Quoted 3 lines

On Windows XP / NTFS / NetBeans 6.1 / Java 5 a lot of warnings are printed. In most cases the path is in fact deleted and it seems to just be a timing bug or something Windows or NTFS specific.

The problem is actually flaws in the unit tests and in the supporting RepositoryTestCase. I think I'll fix that instead.

Thanks. Any progress on this? Can I help in any way?

Signature · 1 line
Jonas Fonseca
#3 lore raw

torsdagen den 30 oktober 2008 11.46.20 skrev Jonas Fonseca:

Quoted 10 lines

Robin Rosenberg <[email protected]> wrote Fri, Oct 24, 2008:

Quoted 7 lines

onsdagen den 22 oktober 2008 10.34.20 skrev Jonas Fonseca:

Quoted 3 lines

On Windows XP / NTFS / NetBeans 6.1 / Java 5 a lot of warnings are printed. In most cases the path is in fact deleted and it seems to just be a timing bug or something Windows or NTFS specific.

The problem is actually flaws in the unit tests and in the supporting RepositoryTestCase. I think I'll fix that instead.

Thanks. Any progress on this? Can I help in any way?

Ok here is a W-I-P. One test case is not cleaning up properly. You may note that I still give some room for gc not to unmap immediately, but at the end of the program we hope that will be the case and I think the missing case is just a bug and not GC being delayed. The counter testcount is only here for debugging this problem.

-- robin

Quoted 1 line

From b3907a11bec0158a99bc77a7655379725203da9c Mon Sep 17 00:00:00 2001

From: Robin Rosenberg <[email protected]>
Date: Thu, 23 Oct 2008 01:27:50 +0200
Subject: [EGIT PATCH] Make the cleanup less verbose when it fails to delete temporary stuff.

---
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |   57 +++++++++++++++-----
 1 files changed, 44 insertions(+), 13 deletions(-)
org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java +44 -13
index 9d7d133..8015a18 100644
@@ -47,6 +47,7 @@
 import java.io.Reader;
 
 import junit.framework.TestCase;
+
 import org.spearce.jgit.util.JGitTestUtil;
 
 public abstract class RepositoryTestCase extends TestCase {
@@ -66,22 +67,34 @@
 		jcommitter = new PersonIdent("J. Committer", "[email protected]");
 	}
 
-	protected static void recursiveDelete(final File dir) {
+	protected static boolean recursiveDelete(final File dir) {
+		return recursiveDelete(dir, false);
+	}
+
+	protected static boolean recursiveDelete(final File dir, boolean silent) {
+		if (!dir.exists())
+			return silent;
 		final File[] ls = dir.listFiles();
 		if (ls != null) {
 			for (int k = 0; k < ls.length; k++) {
 				final File e = ls[k];
 				if (e.isDirectory()) {
-					recursiveDelete(e);
+					silent = recursiveDelete(e, silent);
 				} else {
-					e.delete();
+					if (!e.delete()) {
+						if (!silent)
+							System.out.println("Warning: Failed to delete " + e);
+						silent = true;
+					}
 				}
 			}
 		}
-		dir.delete();
-		if (dir.exists()) {
-			System.out.println("Warning: Failed to delete " + dir);
+		if (!dir.delete()) {
+			if (!silent)
+				System.out.println("Warning: Failed to delete " + dir);
+			silent = true;
 		}
+		return silent;
 	}
 
 	protected static void copyFile(final File src, final File dst)
@@ -121,19 +134,31 @@ protected static void checkFile(File f, final String checkData)
 
 	protected Repository db;
 
+	private static int testcount;
+	private static Thread shutdownhook;
+
 	public void setUp() throws Exception {
 		super.setUp();
-		recursiveDelete(trashParent);
-		trash = new File(trashParent,"trash"+System.currentTimeMillis());
+		System.gc();
+		trash = new File(trashParent,"xtrash"+System.currentTimeMillis()+"."+(testcount++));
+		recursiveDelete(trashParent, true);
 		trash_git = new File(trash, ".git");
-
+		if (shutdownhook == null) {
+			shutdownhook = new Thread() {
+				@Override
+				public void run() {
+					System.gc();
+					recursiveDelete(trashParent, false);
+				}
+			};
+			Runtime.getRuntime().addShutdownHook(shutdownhook);
+		}
 		Runtime.getRuntime().addShutdownHook(new Thread() {
 			@Override
 			public void run() {
-				recursiveDelete(trashParent);
+				recursiveDelete(trash);
 			}
 		});
-
 		db = new Repository(trash_git);
 		db.create();
 
@@ -170,12 +195,18 @@ protected void tearDown() throws Exception {
 	 * @throws IOException
 	 */
 	protected Repository createNewEmptyRepo() throws IOException {
-		File newTestRepo = new File(trashParent, "new"+System.currentTimeMillis()+"/.git");
+		final File newTestRepo = new File(trashParent, "new"+System.currentTimeMillis()+"."+(testcount++)+"/.git");
 		assertFalse(newTestRepo.exists());
-		File unusedDir = new File(trashParent, "tmp"+System.currentTimeMillis());
+		File unusedDir = new File(trashParent, "tmp"+System.currentTimeMillis()+"."+(testcount++));
 		assertTrue(unusedDir.mkdirs());
 		final Repository newRepo = new Repository(newTestRepo);
 		newRepo.create();
+		Runtime.getRuntime().addShutdownHook(new Thread() {
+			@Override
+			public void run() {
+				recursiveDelete(newTestRepo);
+			}
+		});
 		return newRepo;
 	}
 
Signature · 1 line
1.6.0.2.308.gef4a