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>

2 files changed, 36 insertions, 14 deletions

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 4ae26a3e56..67bedaca11 100644
--- 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 2802c0712e..50f57130c3 100644
--- 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;
 		}