Push errors back over sideband when possible

If an internal exception occurs while packing and the request
needs to abort, the HTTP response might already be committed due
to progress message having already been delivered to the client.
This prevents UploadPackServlet from resetting the response and
sending back an HTTP 500 response.

Try to catch all exceptions and report internal errors over the
sideband stream or as an ERR command during the initial ACK/NAK
negotiation phase. This allows JGit to transmit an error message
that the user will receive on their console without needing to
worry about resetting the (already gone) HTTP response.

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

2 files changed, 24 insertions, 4 deletions

diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ReceivePackServlet.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ReceivePackServlet.java
index 192050a17c..0c856d4aa5 100644
--- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ReceivePackServlet.java
+++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ReceivePackServlet.java
@@ -64,6 +64,7 @@ import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
+import org.eclipse.jgit.errors.UnpackException;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.transport.ReceivePack;
 import org.eclipse.jgit.transport.RefAdvertiser.PacketLineOutRefAdvertiser;
@@ -170,9 +171,18 @@ class ReceivePackServlet extends HttpServlet {
 			};
 			rp.receive(getInputStream(req), out, null);
 			out.close();
+		} catch (UnpackException e) {
+			// This should be already reported to the client.
+			getServletContext().log(
+					HttpServerText.get().internalErrorDuringReceivePack,
+					e.getCause());
+
 		} catch (IOException e) {
 			getServletContext().log(HttpServerText.get().internalErrorDuringReceivePack, e);
-			rsp.sendError(SC_INTERNAL_SERVER_ERROR);
+			if (!rsp.isCommitted()) {
+				rsp.reset();
+				rsp.sendError(SC_INTERNAL_SERVER_ERROR);
+			}
 			return;
 		}
 	}
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 2b9e81f1d3..178473c401 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
@@ -68,6 +68,7 @@ import javax.servlet.http.HttpServletResponse;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.transport.RefAdvertiser.PacketLineOutRefAdvertiser;
 import org.eclipse.jgit.transport.UploadPack;
+import org.eclipse.jgit.transport.UploadPackInternalServerErrorException;
 import org.eclipse.jgit.transport.UploadPackMayNotContinueException;
 import org.eclipse.jgit.transport.resolver.ServiceNotAuthorizedException;
 import org.eclipse.jgit.transport.resolver.ServiceNotEnabledException;
@@ -175,14 +176,23 @@ class UploadPackServlet extends HttpServlet {
 			out.close();
 
 		} catch (UploadPackMayNotContinueException e) {
-			if (!e.isOutput())
+			if (!e.isOutput() && !rsp.isCommitted()) {
+				rsp.reset();
 				rsp.sendError(SC_SERVICE_UNAVAILABLE);
+			}
 			return;
 
+		} catch (UploadPackInternalServerErrorException e) {
+			getServletContext().log(
+					HttpServerText.get().internalErrorDuringUploadPack,
+					e.getCause());
+
 		} catch (IOException e) {
 			getServletContext().log(HttpServerText.get().internalErrorDuringUploadPack, e);
-			rsp.reset();
-			rsp.sendError(SC_INTERNAL_SERVER_ERROR);
+			if (!rsp.isCommitted()) {
+				rsp.reset();
+				rsp.sendError(SC_INTERNAL_SERVER_ERROR);
+			}
 			return;
 		}
 	}