[JGIT PATCH 1/2] Fix deadlock in native git protocol client for upload-pack

Subsystems: the rest

DORMANTno replies

2 messages, 1 author, 2016-06-15 · open the first message on its own page

[JGIT PATCH 1/2] Fix deadlock in native git protocol client for upload-pack

From: Shawn O. Pearce <hidden>
Date: 2016-06-15 22:46:44

The upload-pack protocol states that for every flush packet
(aka "0000") sent by the client, we get exactly one NAK or
ACK line back.  However, if multi_ack is enabled, we may
get multiple "ACK %s continue" lines before the ACK/NAK is
sent by the upload-pack server.

Unfortunately, JGit was counting an "ACK %s continue" as a response
to a flush packet, causing the client side to stop reading from
the server too early.  This left a lot of "ACK %s continue" lines
in the server output buffer, eventually causing the server to stop
and wait for the output buffer to drain.  However, the client would
also get stuck after sending too many "have %s" lines, eventually
deadlocking the entire client-server pair.

Signed-off-by: Shawn O. Pearce <redacted>
---
 .../jgit/transport/BasePackFetchConnection.java    |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
index f07cc4e..eaa94bd 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
@@ -359,15 +359,15 @@ private void negotiate(final ProgressMonitor monitor) throws IOException,
 				continue;
 			}
 
-			while (resultsPending > 0) {
+			for (;;) {
 				final PacketLineIn.AckNackResult anr;
 
 				anr = pckIn.readACK(ackId);
-				resultsPending--;
 				if (anr == PacketLineIn.AckNackResult.NAK) {
 					// More have lines are necessary to compute the
 					// pack on the remote side. Keep doing that.
 					//
+					resultsPending--;
 					break;
 				}
 
-- 
1.6.3.48.g99c76

[JGIT PATCH 2/2] Decrease the fetch pack client buffer to the lower minimum

From: Shawn O. Pearce <hidden>
Date: 2016-06-15 22:46:44

This is the lowest buffer size we actually require to keep the
client and server sides from deadlocking against each other.

Also added documentation, and renamed the symbol to better match
its real purpose; naming the lower threshold we can allow for a
buffer.

Signed-off-by: Shawn O. Pearce <redacted>
---
 .../jgit/transport/BasePackFetchConnection.java    |   10 +++++++++-
 .../org/spearce/jgit/transport/TransportLocal.java |    2 +-
 2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
index eaa94bd..1d1b801 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
@@ -89,7 +89,15 @@
 	 */
 	private static final int MAX_HAVES = 256;
 
-	protected static final int MAX_CLIENT_BUFFER = MAX_HAVES * 46 + 1024;
+	/**
+	 * Amount of data the client sends before starting to read.
+	 * <p>
+	 * Any output stream given to the client must be able to buffer this many
+	 * bytes before the client will stop writing and start reading from the
+	 * input stream. If the output stream blocks before this many bytes are in
+	 * the send queue, the system will deadlock.
+	 */
+	protected static final int MIN_CLIENT_BUFFER = 2 * 32 * 46 + 4;
 
 	static final String OPTION_INCLUDE_TAG = "include-tag";
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportLocal.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportLocal.java
index cffdba1..428f73e 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportLocal.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportLocal.java
@@ -175,7 +175,7 @@ InternalLocalFetchConnection() throws TransportException {
 					// force the buffer to be big enough, otherwise it
 					// will deadlock both threads.
 					{
-						buffer = new byte[MAX_CLIENT_BUFFER];
+						buffer = new byte[MIN_CLIENT_BUFFER];
 					}
 				};
 				out_w = new PipedOutputStream(out_r);
-- 
1.6.3.48.g99c76
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help