Implement the no-done capability

Smart HTTP clients may request both multi_ack_detailed and no-done in
the same request to prevent the client from needing to send a "done"
line to the server in response to a server's "ACK %s ready".

For smart HTTP, this can save 1 full HTTP RPC in the fetch exchange,
improving overall latency when incrementally updating a client that
has not diverged very far from the remote repository.

Unfortuantely this capability cannot be enabled for the traditional
bi-directional connections.  multi_ack_detailed has the client sending
more "have" lines at the same time that the server is creating the
"ACK %s ready" and writing out the PACK stream, resulting in some race
conditions and/or deadlock, depending on how the pipe buffers are
implemented.  For very small updates, a server might actually be able
to send "ACK %s ready", then the PACK, and disconnect before the
client even finishes sending its first batch of "have" lines.  This
may cause the client to fail with a broken pipe exception.  To avoid
all of these potential problems, "no-done" is restricted only to the
smart HTTP variant of the protocol.

Change-Id: Ie0d0a39320202bc096fec2e97cb58e9efd061b2d
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>

diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java
index 1ceb0965a3860f82c8ed307a8a9e235c3b82ca43..2b9e81f1d3f3d5992eaacc21a369e07a4b98cd6d 100644 (file)
--- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java
+++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java
@@ -104,6 +104,7 @@ class UploadPackServlet extends HttpServlet {
                                ServiceNotEnabledException, ServiceNotAuthorizedException {
                        UploadPack up = (UploadPack) req.getAttribute(ATTRIBUTE_HANDLER);
                        try {
+                               up.setBiDirectionalPipe(false);
                                up.sendAdvertisedRefs(pck);
                        } finally {
                                up.getRevWalk().release();

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 c3590a44fb923daabee85a3a5be4585aac194d53..cd127cdeaf702e183e1b8d9e0219143c288a521e 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
@@ -292,8 +292,6 @@ public class SmartClientSmartServerTest extends HttpTestCase {
                                .getRequestHeader(HDR_CONTENT_LENGTH));
                assertNull("not chunked", service
                                .getRequestHeader(HDR_TRANSFER_ENCODING));
-               assertNull("no compression (too small)", service
-                               .getRequestHeader(HDR_CONTENT_ENCODING));
 
                assertEquals(200, service.getStatus());
                assertEquals("application/x-git-upload-pack-result", service
@@ -301,7 +299,70 @@ public class SmartClientSmartServerTest extends HttpTestCase {
        }
 
        @Test
-       public void testFetchUpdateExisting() throws Exception {
+       public void testFetch_FewLocalCommits() throws Exception {
+               // Bootstrap by doing the clone.
+               //
+               TestRepository dst = createTestRepository();
+               Transport t = Transport.open(dst.getRepository(), remoteURI);
+               try {
+                       t.fetch(NullProgressMonitor.INSTANCE, mirror(master));
+               } finally {
+                       t.close();
+               }
+               assertEquals(B, dst.getRepository().getRef(master).getObjectId());
+               List<AccessEvent> cloneRequests = getRequests();
+
+               // Only create a few new commits.
+               TestRepository.BranchBuilder b = dst.branch(master);
+               for (int i = 0; i < 4; i++)
+                       b.commit().tick(3600 /* 1 hour */).message("c" + i).create();
+
+               // Create a new commit on the remote.
+               //
+               b = new TestRepository(remoteRepository).branch(master);
+               RevCommit Z = b.commit().message("Z").create();
+
+               // Now incrementally update.
+               //
+               t = Transport.open(dst.getRepository(), remoteURI);
+               try {
+                       t.fetch(NullProgressMonitor.INSTANCE, mirror(master));
+               } finally {
+                       t.close();
+               }
+               assertEquals(Z, dst.getRepository().getRef(master).getObjectId());
+
+               List<AccessEvent> requests = getRequests();
+               requests.removeAll(cloneRequests);
+               assertEquals(2, requests.size());
+
+               AccessEvent info = requests.get(0);
+               assertEquals("GET", info.getMethod());
+               assertEquals(join(remoteURI, "info/refs"), info.getPath());
+               assertEquals(1, info.getParameters().size());
+               assertEquals("git-upload-pack", info.getParameter("service"));
+               assertEquals(200, info.getStatus());
+               assertEquals("application/x-git-upload-pack-advertisement",
+                               info.getResponseHeader(HDR_CONTENT_TYPE));
+
+               // We should have needed one request to perform the fetch.
+               //
+               AccessEvent service = requests.get(1);
+               assertEquals("POST", service.getMethod());
+               assertEquals(join(remoteURI, "git-upload-pack"), service.getPath());
+               assertEquals(0, service.getParameters().size());
+               assertNotNull("has content-length",
+                               service.getRequestHeader(HDR_CONTENT_LENGTH));
+               assertNull("not chunked",
+                               service.getRequestHeader(HDR_TRANSFER_ENCODING));
+
+               assertEquals(200, service.getStatus());
+               assertEquals("application/x-git-upload-pack-result",
+                               service.getResponseHeader(HDR_CONTENT_TYPE));
+       }
+
+       @Test
+       public void testFetch_TooManyLocalCommits() throws Exception {
                // Bootstrap by doing the clone.
                //
                TestRepository dst = createTestRepository();

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java
index 4ae26a3e56f739f78f9b2a06d79bced34e0296a3..67bedaca111caa47fdaf29093682a7fc2788ab81 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java
@@ -138,6 +138,8 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 
        static final String OPTION_NO_PROGRESS = "no-progress";
 
+       static final String OPTION_NO_DONE = "no-done";
+
        static enum MultiAck {
                OFF, CONTINUE, DETAILED;
        }
@@ -169,6 +171,8 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 
        private boolean allowOfsDelta;
 
+       private boolean noDone;
+
        private String lockMessage;
 
        private PackLock packLock;
@@ -408,9 +412,11 @@ public abstract class BasePackFetchConnection extends BasePackConnection
                if (allowOfsDelta)
                        wantCapability(line, OPTION_OFS_DELTA);
 
-               if (wantCapability(line, OPTION_MULTI_ACK_DETAILED))
+               if (wantCapability(line, OPTION_MULTI_ACK_DETAILED)) {
                        multiAck = MultiAck.DETAILED;
-               else if (wantCapability(line, OPTION_MULTI_ACK))
+                       if (statelessRPC)
+                               noDone = wantCapability(line, OPTION_NO_DONE);
+               } else if (wantCapability(line, OPTION_MULTI_ACK))
                        multiAck = MultiAck.CONTINUE;
                else
                        multiAck = MultiAck.OFF;
@@ -441,13 +447,13 @@ public abstract class BasePackFetchConnection extends BasePackConnection
                int havesSinceLastContinue = 0;
                boolean receivedContinue = false;
                boolean receivedAck = false;
-               boolean negotiate = true;
+               boolean receivedReady = false;
 
                if (statelessRPC)
                        state.writeTo(out, null);
 
                negotiateBegin();
-               SEND_HAVES: while (negotiate) {
+               SEND_HAVES: while (!receivedReady) {
                        final RevCommit c = walk.next();
                        if (c == null)
                                break SEND_HAVES;
@@ -514,7 +520,7 @@ public abstract class BasePackFetchConnection extends BasePackConnection
                                        receivedContinue = true;
                                        havesSinceLastContinue = 0;
                                        if (anr == AckNackResult.ACK_READY)
-                                               negotiate = false;
+                                               receivedReady = true;
                                        break;
                                }
 
@@ -540,12 +546,14 @@ public abstract class BasePackFetchConnection extends BasePackConnection
                if (monitor.isCancelled())
                        throw new CancelledException();
 
-               // When statelessRPC is true we should always leave SEND_HAVES
-               // loop above while in the middle of a request. This allows us
-               // to just write done immediately.
-               //
-               pckOut.writeString("done\n");
-               pckOut.flush();
+               if (!receivedReady || !noDone) {
+                       // When statelessRPC is true we should always leave SEND_HAVES
+                       // loop above while in the middle of a request. This allows us
+                       // to just write done immediately.
+                       //
+                       pckOut.writeString("done\n");
+                       pckOut.flush();
+               }
 
                if (!receivedAck) {
                        // Apparently if we have never received an ACK earlier

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 2802c0712e72cadc82a0eeb108c0733397241d9b..50f57130c35ec6efdb154cb6d23c7db63ef27c9c 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -100,6 +100,8 @@ public class UploadPack {
 
        static final String OPTION_NO_PROGRESS = BasePackFetchConnection.OPTION_NO_PROGRESS;
 
+       static final String OPTION_NO_DONE = BasePackFetchConnection.OPTION_NO_DONE;
+
        /** Database we read the objects from. */
        private final Repository db;
 
@@ -163,6 +165,8 @@ public class UploadPack {
        /** null if {@link #commonBase} should be examined again. */
        private Boolean okToGiveUp;
 
+       private boolean sentReady;
+
        /** Objects we sent in our advertisement list, clients can ask for these. */
        private Set<ObjectId> advertised;
 
@@ -182,6 +186,8 @@ public class UploadPack {
 
        private MultiAck multiAck = MultiAck.OFF;
 
+       private boolean noDone;
+
        private PackWriter.Statistics statistics;
 
        private UploadPackLogger logger;
@@ -401,9 +407,10 @@ public class UploadPack {
                        return;
                }
 
-               if (options.contains(OPTION_MULTI_ACK_DETAILED))
+               if (options.contains(OPTION_MULTI_ACK_DETAILED)) {
                        multiAck = MultiAck.DETAILED;
-               else if (options.contains(OPTION_MULTI_ACK))
+                       noDone = options.contains(OPTION_NO_DONE);
+               } else if (options.contains(OPTION_MULTI_ACK))
                        multiAck = MultiAck.CONTINUE;
                else
                        multiAck = MultiAck.OFF;
@@ -443,6 +450,8 @@ public class UploadPack {
                adv.advertiseCapability(OPTION_SIDE_BAND_64K);
                adv.advertiseCapability(OPTION_THIN_PACK);
                adv.advertiseCapability(OPTION_NO_PROGRESS);
+               if (!biDirectionalPipe)
+                       adv.advertiseCapability(OPTION_NO_DONE);
                adv.setDerefTags(true);
                advertised = adv.send(getAdvertisedRefs());
                adv.end();
@@ -496,6 +505,10 @@ public class UploadPack {
                                last = processHaveLines(peerHas, last);
                                if (commonBase.isEmpty() || multiAck != MultiAck.OFF)
                                        pckOut.writeString("NAK\n");
+                               if (noDone && sentReady) {
+                                       pckOut.writeString("ACK " + last.name() + "\n");
+                                       return true;
+                               }
                                if (!biDirectionalPipe)
                                        return false;
                                pckOut.flush();
@@ -538,6 +551,7 @@ public class UploadPack {
                List<ObjectId> toParse = peerHas;
                HashSet<ObjectId> peerHasSet = null;
                boolean needMissing = false;
+               sentReady = false;
 
                if (wantAll.isEmpty() && !wantIds.isEmpty()) {
                        // We have not yet parsed the want list. Parse it now.
@@ -644,7 +658,6 @@ public class UploadPack {
                // telling us about its history.
                //
                boolean didOkToGiveUp = false;
-               boolean sentReady = false;
                if (0 < missCnt) {
                        for (int i = peerHas.size() - 1; i >= 0; i--) {
                                ObjectId id = peerHas.get(i);
@@ -670,6 +683,7 @@ public class UploadPack {
 
                if (multiAck == MultiAck.DETAILED && !didOkToGiveUp && okToGiveUp()) {
                        ObjectId id = peerHas.get(peerHas.size() - 1);
+                       sentReady = true;
                        pckOut.writeString("ACK " + id.name() + " ready\n");
                        sentReady = true;
                }