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 1ceb096..2b9e81f 100644
--- 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 @@
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 c3590a4..cd127cd 100644
--- 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 @@
.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 @@
}
@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 4ae26a3..67bedac 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 @@
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 @@
private boolean allowOfsDelta;
+ private boolean noDone;
+
private String lockMessage;
private PackLock packLock;
@@ -408,9 +412,11 @@
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 @@
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 @@
receivedContinue = true;
havesSinceLastContinue = 0;
if (anr == AckNackResult.ACK_READY)
- negotiate = false;
+ receivedReady = true;
break;
}
@@ -540,12 +546,14 @@
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 2802c07..50f5713 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 @@
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 @@
/** 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 @@
private MultiAck multiAck = MultiAck.OFF;
+ private boolean noDone;
+
private PackWriter.Statistics statistics;
private UploadPackLogger logger;
@@ -401,9 +407,10 @@
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 @@
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 @@
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 @@
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 @@
// 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 @@
if (multiAck == MultiAck.DETAILED && !didOkToGiveUp && okToGiveUp()) {
ObjectId id = peerHas.get(peerHas.size() - 1);
+ sentReady = true;
pckOut.writeString("ACK " + id.name() + " ready\n");
sentReady = true;
}
