Make UploadPack observe exceptions reading refs

Now if refs are unreadable when serving an upload pack the handler
will fail due to the actual underlying failure. Previously all wants
would be rejected as invalid because Repository.getAllRefs() returned
an empty map.

Testing this required a new subclass of InMemoryRepository so that
an IOException could be injected at the correct time.

Signed-off-by: Michael Edgar <adgar@google.com>
Change-Id: Iac708b1db9d0ccce08c4ef5ace599ea0b57afdc0

diff --git a/org.eclipse.jgit.http.test/META-INF/MANIFEST.MF b/org.eclipse.jgit.http.test/META-INF/MANIFEST.MF
index 08924e72ce319007d09d8ce4403c688b03327d6e..253679b2c98425fa829ec64f8898167f90de9822 100644 (file)
--- a/org.eclipse.jgit.http.test/META-INF/MANIFEST.MF
+++ b/org.eclipse.jgit.http.test/META-INF/MANIFEST.MF
@@ -27,6 +27,7 @@ Import-Package: javax.servlet;version="[2.5.0,3.2.0)",
  org.eclipse.jgit.http.server.glue;version="[4.4.0,4.5.0)",
  org.eclipse.jgit.http.server.resolver;version="[4.4.0,4.5.0)",
  org.eclipse.jgit.internal;version="[4.4.0,4.5.0)",
+ org.eclipse.jgit.internal.storage.dfs;version="[4.4.0,4.5.0)",
  org.eclipse.jgit.internal.storage.file;version="[4.4.0,4.5.0)",
  org.eclipse.jgit.junit;version="[4.4.0,4.5.0)",
  org.eclipse.jgit.junit.http;version="[4.4.0,4.5.0)",

diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/RefsUnreadableInMemoryRepository.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/RefsUnreadableInMemoryRepository.java
new file mode 100644 (file)
index 0000000..485cced
--- /dev/null
+++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/RefsUnreadableInMemoryRepository.java
@@ -0,0 +1,52 @@
+package org.eclipse.jgit.http.test;
+
+import java.io.IOException;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.lib.RefDatabase;
+
+/**
+ * An {@link InMemoryRepository} whose refs can be made unreadable for testing
+ * purposes.
+ */
+class RefsUnreadableInMemoryRepository extends InMemoryRepository {
+
+       private final RefsUnreadableRefDatabase refs;
+
+       private volatile boolean failing;
+
+       RefsUnreadableInMemoryRepository(DfsRepositoryDescription repoDesc) {
+               super(repoDesc);
+               refs = new RefsUnreadableRefDatabase();
+               failing = false;
+       }
+
+       @Override
+       public RefDatabase getRefDatabase() {
+               return refs;
+       }
+
+       /**
+        * Make the ref database unable to scan its refs.
+        * <p>
+        * It may be useful to follow a call to startFailing with a call to
+        * {@link RefDatabase#refresh()}, ensuring the next ref read fails.
+        */
+       void startFailing() {
+               failing = true;
+       }
+
+       private class RefsUnreadableRefDatabase extends MemRefDatabase {
+
+               @Override
+               protected RefCache scanAllRefs() throws IOException {
+                       if (failing) {
+                               throw new IOException("disk failed, no refs found");
+                       } else {
+                               return super.scanAllRefs();
+                       }
+               }
+       }
+}

diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
index 0f3d3c6cf5c8820bf57a8ed50f129e549f8222cf..073c751286c03beafea395d02cb84820747dc073 100644 (file)
--- a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
+++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
@@ -82,9 +82,11 @@ import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.errors.TransportException;
 import org.eclipse.jgit.http.server.GitServlet;
 import org.eclipse.jgit.internal.JGitText;
+import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.junit.TestRng;
 import org.eclipse.jgit.junit.http.AccessEvent;
+import org.eclipse.jgit.junit.http.AppServer;
 import org.eclipse.jgit.junit.http.HttpTestCase;
 import org.eclipse.jgit.lib.ConfigConstants;
 import org.eclipse.jgit.lib.Constants;
@@ -155,18 +157,7 @@ public class SmartClientSmartServerTest extends HttpTestCase {
 
                ServletContextHandler app = server.addContext("/git");
                GitServlet gs = new GitServlet();
-               gs.setRepositoryResolver(new RepositoryResolver<HttpServletRequest>() {
-                       public Repository open(HttpServletRequest req, String name)
-                                       throws RepositoryNotFoundException,
-                                       ServiceNotEnabledException {
-                               if (!name.equals(srcName))
-                                       throw new RepositoryNotFoundException(name);
-
-                               final Repository db = src.getRepository();
-                               db.incrementOpen();
-                               return db;
-                       }
-               });
+               gs.setRepositoryResolver(new TestRepoResolver(src, srcName));
                app.addServlet(new ServletHolder(gs), "/*");
 
                ServletContextHandler broken = server.addContext("/bad");
@@ -508,6 +499,51 @@ public class SmartClientSmartServerTest extends HttpTestCase {
                }
        }
 
+       @Test
+       public void testFetch_RefsUnreadableOnUpload() throws Exception {
+               AppServer noRefServer = new AppServer();
+               try {
+                       final String repoName = "refs-unreadable";
+                       RefsUnreadableInMemoryRepository badRefsRepo = new RefsUnreadableInMemoryRepository(
+                                       new DfsRepositoryDescription(repoName));
+                       final TestRepository<Repository> repo = new TestRepository<Repository>(
+                                       badRefsRepo);
+
+                       ServletContextHandler app = noRefServer.addContext("/git");
+                       GitServlet gs = new GitServlet();
+                       gs.setRepositoryResolver(new TestRepoResolver(repo, repoName));
+                       app.addServlet(new ServletHolder(gs), "/*");
+                       noRefServer.setUp();
+
+                       RevBlob A2_txt = repo.blob("A2");
+                       RevCommit A2 = repo.commit().add("A2_txt", A2_txt).create();
+                       RevCommit B2 = repo.commit().parent(A2).add("A2_txt", "C2")
+                                       .add("B2", "B2").create();
+                       repo.update(master, B2);
+
+                       URIish badRefsURI = new URIish(noRefServer.getURI()
+                                       .resolve(app.getContextPath() + "/" + repoName).toString());
+
+                       Repository dst = createBareRepository();
+                       try (Transport t = Transport.open(dst, badRefsURI);
+                                       FetchConnection c = t.openFetch()) {
+                               // We start failing here to exercise the post-advertisement
+                               // upload pack handler.
+                               badRefsRepo.startFailing();
+                               // Need to flush caches because ref advertisement populated them.
+                               badRefsRepo.getRefDatabase().refresh();
+                               c.fetch(NullProgressMonitor.INSTANCE,
+                                               Collections.singleton(c.getRef(master)),
+                                               Collections.<ObjectId> emptySet());
+                               fail("Successfully served ref with value " + c.getRef(master));
+                       } catch (TransportException err) {
+                               assertEquals("internal server error", err.getMessage());
+                       }
+               } finally {
+                       noRefServer.tearDown();
+               }
+       }
+
        @Test
        public void testPush_NotAuthorized() throws Exception {
                final TestRepository src = createTestRepository();
@@ -677,4 +713,28 @@ public class SmartClientSmartServerTest extends HttpTestCase {
                cfg.setBoolean("http", null, "receivepack", true);
                cfg.save();
        }
+
+       private final class TestRepoResolver
+                       implements RepositoryResolver<HttpServletRequest> {
+
+               private final TestRepository<Repository> repo;
+
+               private final String repoName;
+
+               private TestRepoResolver(TestRepository<Repository> repo,
+                               String repoName) {
+                       this.repo = repo;
+                       this.repoName = repoName;
+               }
+
+               public Repository open(HttpServletRequest req, String name)
+                               throws RepositoryNotFoundException, ServiceNotEnabledException {
+                       if (!name.equals(repoName))
+                               throw new RepositoryNotFoundException(name);
+
+                       Repository db = repo.getRepository();
+                       db.incrementOpen();
+                       return db;
+               }
+       }
 }

diff --git a/org.eclipse.jgit/META-INF/MANIFEST.MF b/org.eclipse.jgit/META-INF/MANIFEST.MF
index 51bee1afff6872b5d0cfdd281f907b10ff485db7..a3d1a92f514b15821eb5cd75abf8a16d08b99c0c 100644 (file)
--- a/org.eclipse.jgit/META-INF/MANIFEST.MF
+++ b/org.eclipse.jgit/META-INF/MANIFEST.MF
@@ -60,7 +60,10 @@ Export-Package: org.eclipse.jgit.annotations;version="4.4.0",
  org.eclipse.jgit.ignore.internal;version="4.4.0";x-friends:="org.eclipse.jgit.test",
  org.eclipse.jgit.internal;version="4.4.0";x-friends:="org.eclipse.jgit.test,org.eclipse.jgit.http.test",
  org.eclipse.jgit.internal.ketch;version="4.4.0";x-friends:="org.eclipse.jgit.junit,org.eclipse.jgit.test,org.eclipse.jgit.pgm",
- org.eclipse.jgit.internal.storage.dfs;version="4.4.0";x-friends:="org.eclipse.jgit.test,org.eclipse.jgit.http.server",
+ org.eclipse.jgit.internal.storage.dfs;version="4.4.0";
+  x-friends:="org.eclipse.jgit.test,
+   org.eclipse.jgit.http.server,
+   org.eclipse.jgit.http.test",
  org.eclipse.jgit.internal.storage.file;version="4.4.0";
   x-friends:="org.eclipse.jgit.test,
    org.eclipse.jgit.junit,

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java
index b312835bb34af857d98fd23803d7522d934215da..ccf1b42b3d37e42af8dd730540b6651316e6aa32 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java
@@ -252,11 +252,20 @@ public class InMemoryRepository extends DfsRepository {
                }
        }
 
-       private class MemRefDatabase extends DfsRefDatabase {
+       /**
+        * A ref database storing all refs in-memory.
+        * <p>
+        * This class is protected (and not private) to facilitate testing using
+        * subclasses of InMemoryRepository.
+        */
+    protected class MemRefDatabase extends DfsRefDatabase {
                private final ConcurrentMap<String, Ref> refs = new ConcurrentHashMap<String, Ref>();
                private final ReadWriteLock lock = new ReentrantReadWriteLock(true /* fair */);
 
-               MemRefDatabase() {
+               /**
+                * Initialize a new in-memory ref database.
+                */
+               protected MemRefDatabase() {
                        super(InMemoryRepository.this);
                }
 

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
index 8b642bb980ed6d0001d7a2fd8ec77eda6b3f890e..e49ee87b7346c394945c34f5039788461ce461f8 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -82,6 +82,7 @@ import org.eclipse.jgit.lib.NullProgressMonitor;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ProgressMonitor;
 import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.RefDatabase;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.AsyncRevObjectQueue;
 import org.eclipse.jgit.revwalk.DepthWalk;
@@ -704,22 +705,22 @@ public class UploadPack {
                return statistics;
        }
 
-       private Map<String, Ref> getAdvertisedOrDefaultRefs() {
+       private Map<String, Ref> getAdvertisedOrDefaultRefs() throws IOException {
                if (refs == null)
-                       setAdvertisedRefs(null);
+                       setAdvertisedRefs(db.getRefDatabase().getRefs(RefDatabase.ALL));
                return refs;
        }
 
        private void service() throws IOException {
-               if (biDirectionalPipe)
-                       sendAdvertisedRefs(new PacketLineOutRefAdvertiser(pckOut));
-               else if (requestValidator instanceof AnyRequestValidator)
-                       advertised = Collections.emptySet();
-               else
-                       advertised = refIdSet(getAdvertisedOrDefaultRefs().values());
-
                boolean sendPack;
                try {
+                       if (biDirectionalPipe)
+                               sendAdvertisedRefs(new PacketLineOutRefAdvertiser(pckOut));
+                       else if (requestValidator instanceof AnyRequestValidator)
+                               advertised = Collections.emptySet();
+                       else
+                               advertised = refIdSet(getAdvertisedOrDefaultRefs().values());
+
                        recvWants();
                        if (wantIds.isEmpty()) {
                                preUploadHook.onBeginNegotiateRound(this, wantIds, 0);