Remove repository from cache when it's closed

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 <hugo.ares@ericsson.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

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 8f30fd0821e64ff377d2881d51e3a4c9e731dd87..a1cec2d9142ea5f64637eb5d46bba7fd0ce943f4 100644 (file)
--- 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 5546b790e8cac193ab9c3f81cc2e897b3b121187..ba0dea39f504e637d237d9cd2683e07f80646da9 100644 (file)
--- 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 58771857b6e2404fa5a8293552ad8e33a78a6ac5..22b5fcd1128af517bab5bd3276cb91bc32b87143 100644 (file)
--- 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.
         * <p>
-        * 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.
         * <p>
-        * 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.
+        * <p>
+        * 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<Repository> 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<Key> getKeys() {