From 7d2b3b9e25a430241a2d277e0cb222ad40cd0c2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Fri, 18 Sep 2015 14:05:23 -0400 Subject: [PATCH] Remove repository from cache when it's closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RepositoryCache has 2 methods to remove a repository from the cache but they are never called when a repository is closed. Users of the cache were expected to call one of those 2 methods but how could they have called them at proper time without having visibility of the repository usage count. Ideally, I would have reworked the RepositoryCache to wrap any repository it opens in a class that would be responsible to unregister them from the cache when it's really closed, i.e. when usage counter reaches 0. The problem preventing the wrapping solution is the RepositoryCache.register method that allows to register an already opened repository in the cache. Such repositories cannot be wrapped because callers are still holding a reference on the unwrapped repository. Document that RepositoryCache.close method is removing the repository from the cache as well as closing it and rework RepositoryCache.unregister method to only remove the repository from the cache. Use the latter to unregister repository when Repository.doClose is getting executed. Change-Id: Ia364816e4da8d7b6cfa72f10758ca31aa8a1f9db Signed-off-by: Hugo Arès Signed-off-by: Matthias Sohn --- .../eclipse/jgit/lib/RepositoryCacheTest.java | 14 ++++++ .../src/org/eclipse/jgit/lib/Repository.java | 1 + .../org/eclipse/jgit/lib/RepositoryCache.java | 43 +++++++++++++++---- 3 files changed, 49 insertions(+), 9 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryCacheTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryCacheTest.java index 8f30fd082..a1cec2d91 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryCacheTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryCacheTest.java @@ -194,4 +194,18 @@ public class RepositoryCacheTest extends RepositoryTestCase { db.close(); assertEquals(0, ((Repository) db).useCnt.get()); } + + public void testRepositoryUnregisteringWhenClosing() throws Exception { + FileKey loc = FileKey.exact(db.getDirectory(), db.getFS()); + Repository d2 = RepositoryCache.open(loc); + assertEquals(1, d2.useCnt.get()); + assertThat(RepositoryCache.getRegisteredKeys(), + hasItem(FileKey.exact(db.getDirectory(), db.getFS()))); + assertEquals(1, RepositoryCache.getRegisteredKeys().size()); + + d2.close(); + + assertEquals(0, d2.useCnt.get()); + assertEquals(0, RepositoryCache.getRegisteredKeys().size()); + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java index 5546b790e..ba0dea39f 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java @@ -865,6 +865,7 @@ public abstract class Repository implements AutoCloseable { public void close() { if (useCnt.decrementAndGet() == 0) { doClose(); + RepositoryCache.unregister(this); } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryCache.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryCache.java index 58771857b..22b5fcd11 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryCache.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryCache.java @@ -130,10 +130,10 @@ public class RepositoryCache { } /** - * Remove a repository from the cache. + * Close and remove a repository from the cache. *

- * Removes a repository from the cache, if it is still registered here, - * permitting it to close. + * Removes a repository from the cache, if it is still registered here, and + * close it. * * @param db * repository to unregister. @@ -141,15 +141,35 @@ public class RepositoryCache { public static void close(final Repository db) { if (db.getDirectory() != null) { FileKey key = FileKey.exact(db.getDirectory(), db.getFS()); - cache.unregisterRepository(key); + cache.unregisterAndCloseRepository(key); } } /** * Remove a repository from the cache. *

- * Removes a repository from the cache, if it is still registered here, - * permitting it to close. + * Removes a repository from the cache, if it is still registered here. This + * method will not close the repository, only remove it from the cache. See + * {@link RepositoryCache#close(Repository)} to remove and close the + * repository. + * + * @param db + * repository to unregister. + * @since 4.3 + */ + public static void unregister(final Repository db) { + if (db.getDirectory() != null) { + unregister(FileKey.exact(db.getDirectory(), db.getFS())); + } + } + + /** + * Remove a repository from the cache. + *

+ * Removes a repository from the cache, if it is still registered here. This + * method will not close the repository, only remove it from the cache. See + * {@link RepositoryCache#close(Repository)} to remove and close the + * repository. * * @param location * location of the repository to remove. @@ -214,11 +234,16 @@ public class RepositoryCache { oldDb.close(); } - private void unregisterRepository(final Key location) { + private Repository unregisterRepository(final Key location) { Reference oldRef = cacheMap.remove(location); - Repository oldDb = oldRef != null ? oldRef.get() : null; - if (oldDb != null) + return oldRef != null ? oldRef.get() : null; + } + + private void unregisterAndCloseRepository(final Key location) { + Repository oldDb = unregisterRepository(location); + if (oldDb != null) { oldDb.close(); + } } private Collection getKeys() {