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

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)",

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();
+			}
+		}
+	}
+}

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;
+		}
+	}
 }

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,

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);
 		}
 

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);