pass outputstream to status code handler to improve mercurial error handling

scm-core/src/main/java/sonia/scm/web/cgi/CGIStatusCodeHandler.java

@@ -33,6 +33,9 @@ package sonia.scm.web.cgi;
 
 //~--- JDK imports ------------------------------------------------------------
 
+import java.io.IOException;
+import java.io.OutputStream;
+
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
@@ -48,12 +51,32 @@ public interface CGIStatusCodeHandler
 
   /**
    * Handles the return code of the process executed by {@link CGIExecutor}.
+   * <b>Note:</b> This method is called when the process has
+   * already written to the {@link OutputStream}.
+   *
+   *
+   * @param request the http request
+   * @param statusCode process return code
+   */
+  public void handleStatusCode(HttpServletRequest request, int statusCode);
+
+  /**
+   * Handles the return code of the process executed by {@link CGIExecutor}.
+   * <b>Note:</b> This method is only called when the process has
+   * not written to the {@link OutputStream}. Do not call
+   * {@link HttpServletResponse#getWriter()}, because there was already a call
+   * to {@link HttpServletResponse#getOutputStream()}.
    *
    *
    * @param request the http request
    * @param response the http response
+   * @param ouputStream the servlet output stream
    * @param statusCode process return code
+   *
+   * @throws IOException
    */
   public void handleStatusCode(HttpServletRequest request,
-                               HttpServletResponse response, int statusCode);
+                               HttpServletResponse response,
+                               OutputStream ouputStream, int statusCode)
+          throws IOException;
 }

scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/web/HgCGIExceptionHandler.java

@@ -33,16 +33,20 @@ package sonia.scm.web;
 
 //~--- non-JDK imports --------------------------------------------------------
 
+import com.google.common.base.Charsets;
+
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import sonia.scm.util.IOUtil;
 import sonia.scm.util.Util;
 import sonia.scm.web.cgi.CGIExceptionHandler;
+import sonia.scm.web.cgi.CGIStatusCodeHandler;
 
 //~--- JDK imports ------------------------------------------------------------
 
 import java.io.IOException;
+import java.io.OutputStream;
 import java.io.PrintWriter;
 
 import java.text.MessageFormat;
@@ -54,7 +58,8 @@ import javax.servlet.http.HttpServletResponse;
  *
  * @author Sebastian Sdorra
  */
-public class HgCGIExceptionHandler implements CGIExceptionHandler
+public class HgCGIExceptionHandler
+        implements CGIExceptionHandler, CGIStatusCodeHandler
 {
 
   /** Field description */
@@ -64,6 +69,10 @@ public class HgCGIExceptionHandler implements CGIExceptionHandler
   public static final String ERROR_NOT_CONFIGURED =
     "The mercurial installation on the scm-manager server seems to be not configured correctly. Please check the settings.";
 
+  /** Field description */
+  public static final String ERROR_STATUSCODE =
+    "Mercurial process ends with return code {0}";
+
   /** Field description */
   public static final String ERROR_UNKNOWN =
     "There is an unknown error occurred: '{0}'";
@@ -93,13 +102,65 @@ public class HgCGIExceptionHandler implements CGIExceptionHandler
       logger.error("not able to handle mercurial request", ex);
     }
 
-    try
+    sendError(response, createErrorMessage(ex));
+  }
+
+  /**
+   * Method description
+   *
+   *
+   * @param request
+   * @param response
+   * @param output
+   * @param statusCode
+   *
+   * @throws IOException
+   */
+  @Override
+  public void handleStatusCode(HttpServletRequest request,
+                               HttpServletResponse response,
+                               OutputStream output, int statusCode)
+          throws IOException
+  {
+    if (statusCode != 0)
     {
-      sendError(response, createErrorMessage(ex));
+      response.setContentType(CONTENT_TYPE_ERROR);
+
+      String msg = MessageFormat.format(ERROR_STATUSCODE, statusCode);
+
+      if (logger.isWarnEnabled())
+      {
+        logger.warn(msg);
+      }
+
+      output.write(msg.getBytes(Charsets.US_ASCII));
     }
-    catch (IOException ioEx)
+    else if (logger.isDebugEnabled())
     {
-      logger.error("could not write error message to client", ioEx);
+      logger.debug("mercurial process ends successfully");
+    }
+  }
+
+  /**
+   * Method description
+   *
+   *
+   * @param request
+   * @param statusCode
+   */
+  @Override
+  public void handleStatusCode(HttpServletRequest request, int statusCode)
+  {
+    if (statusCode != 0)
+    {
+      if (logger.isWarnEnabled())
+      {
+        logger.error("mercurial process ends with {}", statusCode);
+      }
+    }
+    else if (logger.isDebugEnabled())
+    {
+      logger.debug("mercurial process ends successfully");
     }
   }
 
@@ -110,10 +171,8 @@ public class HgCGIExceptionHandler implements CGIExceptionHandler
    * @param response
    * @param message
    *
-   * @throws IOException
    */
   public void sendError(HttpServletResponse response, String message)
-          throws IOException
   {
     response.setContentType(CONTENT_TYPE_ERROR);
 
@@ -124,6 +183,10 @@ public class HgCGIExceptionHandler implements CGIExceptionHandler
       writer = response.getWriter();
       writer.println(message);
     }
+    catch (IOException ex)
+    {
+      logger.error("could not write error message to client", ex);
+    }
     finally
     {
       IOUtil.close(writer);

scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/web/HgCGIServlet.java

@@ -271,6 +271,8 @@ public class HgCGIServlet extends HttpServlet
     CGIExecutor executor = cgiExecutorFactory.createExecutor(configuration,
                              getServletContext(), request, response);
 
+    executor.setExceptionHandler(exceptionHandler);
+    executor.setStatusCodeHandler(exceptionHandler);
     executor.setContentLengthWorkaround(true);
     executor.getEnvironment().set(ENV_REPOSITORY_NAME, name);
     executor.getEnvironment().set(ENV_REPOSITORY_PATH,

scm-webapp/src/main/java/sonia/scm/web/cgi/DefaultCGIExecutor.java

@@ -35,6 +35,8 @@ package sonia.scm.web.cgi;
 
 //~--- non-JDK imports --------------------------------------------------------
 
+import com.google.common.io.ByteStreams;
+
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -389,6 +391,7 @@ public class DefaultCGIExecutor extends AbstractCGIExecutor
   {
     InputStream processIS = null;
     InputStream processES = null;
+    ServletOutputStream servletOS = null;
 
     try
     {
@@ -396,13 +399,18 @@ public class DefaultCGIExecutor extends AbstractCGIExecutor
       processErrorStreamAsync(processES);
       processServletInput(process);
       processIS = process.getInputStream();
-      processProcessInputStream(processIS);
-      waitForFinish(process);
+      servletOS = response.getOutputStream();
+      parseHeaders(processIS);
+
+      long content = ByteStreams.copy(processIS, servletOS);
+
+      waitForFinish(process, servletOS, content);
     }
     finally
     {
       IOUtil.close(processIS);
       IOUtil.close(processES);
+      IOUtil.close(servletOS);
     }
   }
 
@@ -519,31 +527,6 @@ public class DefaultCGIExecutor extends AbstractCGIExecutor
     }).start();
   }
 
-  /**
-   * Method description
-   *
-   *
-   * @param is
-   *
-   * @throws IOException
-   */
-  private void processProcessInputStream(InputStream is) throws IOException
-  {
-    parseHeaders(is);
-
-    ServletOutputStream servletOS = null;
-
-    try
-    {
-      servletOS = response.getOutputStream();
-      IOUtil.copy(is, servletOS, bufferSize);
-    }
-    finally
-    {
-      IOUtil.close(servletOS);
-    }
-  }
-
   /**
    * Method description
    *
@@ -579,9 +562,15 @@ public class DefaultCGIExecutor extends AbstractCGIExecutor
    *
    *
    * @param process
+   * @param output
+   * @param content
    *
+   *
+   * @throws IOException
    */
-  private void waitForFinish(Process process)
+  private void waitForFinish(Process process, ServletOutputStream output,
+                             long content)
+          throws IOException
   {
     try
     {
@@ -591,11 +580,20 @@ public class DefaultCGIExecutor extends AbstractCGIExecutor
       {
         if (logger.isTraceEnabled())
         {
-          logger.trace("handle status code {} with statusCodeHandler",
-                       exitCode);
+          logger.trace(
+              "handle status code {} with statusCodeHandler, there are {} bytes written to outputstream",
+              exitCode, content);
         }
 
-        getStatusCodeHandler().handleStatusCode(request, response, exitCode);
+        if (content == 0)
+        {
+          getStatusCodeHandler().handleStatusCode(request, response, output,
+                  exitCode);
+        }
+        else
+        {
+          getStatusCodeHandler().handleStatusCode(request, exitCode);
+        }
       }
       else if (logger.isDebugEnabled())
       {