Fix smart HTTP client buffer alignment

This proved to be a pretty difficult to find bug.  If we read exactly
the number of response bytes from the UnionInputStream and didn't
try to read beyond that length, the last connection's InputStream is
still inside of the UnionInputStream, and UnionInputStream.isEmpty()
returns false.  But there is no data present, so the next read
request to our UnionInputStream returns EOF at a point where the
HTTP client code should have started a new request in order to get
more data.

Instead of wrapping the UnionInputStream, push an dummy stream onto
the end of it which when invoked always starts the next request and
then returns EOF.  The UnionInputStream will automatically pop that
dummy stream out, and then read the next request's stream.

This way we never get into the state where we don't think we need
to run another request in order to satisfy the current read request,
but we really do.

The bug was hidden for so long because BasePackConnection.init()
was always wrapping the InputStream into a BufferedInputStream
with an 8 KiB buffer.  This made the odds of us reading from the
UnionInputStream the exact number of available bytes quite low, as
the BufferedInputStream would always try to read a full buffer size.

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

1 files changed, 15 insertions, 32 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java
index 8de16c13d0..f49828bf2d 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java
@@ -632,9 +632,9 @@ public class TransportHttp extends HttpTransport implements WalkTransport,
 
 		private final String responseType;
 
-		private final UnionInputStream httpIn;
+		private final HttpExecuteStream execute;
 
-		final HttpInputStream in;
+		final UnionInputStream in;
 
 		final HttpOutputStream out;
 
@@ -645,8 +645,8 @@ public class TransportHttp extends HttpTransport implements WalkTransport,
 			this.requestType = "application/x-" + serviceName + "-request";
 			this.responseType = "application/x-" + serviceName + "-result";
 
-			this.httpIn = new UnionInputStream();
-			this.in = new HttpInputStream(httpIn);
+			this.execute = new HttpExecuteStream();
+			this.in = new UnionInputStream(execute);
 			this.out = new HttpOutputStream();
 		}
 
@@ -712,7 +712,8 @@ public class TransportHttp extends HttpTransport implements WalkTransport,
 				throw wrongContentType(responseType, contentType);
 			}
 
-			httpIn.add(openInputStream(conn));
+			in.add(openInputStream(conn));
+			in.add(execute);
 			conn = null;
 		}
 
@@ -729,43 +730,25 @@ public class TransportHttp extends HttpTransport implements WalkTransport,
 			}
 		}
 
-		class HttpInputStream extends InputStream {
-			private final UnionInputStream src;
-
-			HttpInputStream(UnionInputStream httpIn) {
-				this.src = httpIn;
-			}
-
-			private InputStream self() throws IOException {
-				if (src.isEmpty()) {
-					// If we have no InputStreams available it means we must
-					// have written data previously to the service, but have
-					// not yet finished the HTTP request in order to get the
-					// response from the service. Ensure we get it now.
-					//
-					execute();
-				}
-				return src;
-			}
-
+		class HttpExecuteStream extends InputStream {
 			public int available() throws IOException {
-				return self().available();
+				execute();
+				return 0;
 			}
 
 			public int read() throws IOException {
-				return self().read();
+				execute();
+				return -1;
 			}
 
 			public int read(byte[] b, int off, int len) throws IOException {
-				return self().read(b, off, len);
+				execute();
+				return -1;
 			}
 
 			public long skip(long n) throws IOException {
-				return self().skip(n);
-			}
-
-			public void close() throws IOException {
-				src.close();
+				execute();
+				return 0;
 			}
 		}
 	}