SONAR-8073 no Throwable must be handled by Tomcat

diff --git a/server/sonar-server/src/main/java/org/sonar/server/platform/web/RootFilter.java b/server/sonar-server/src/main/java/org/sonar/server/platform/web/RootFilter.java
index dec5a8d76c17b86c80496b4e925faa338612f9ab..7d11e1782a0bbdaad4640493bef272d07ba16406 100644 (file)
--- a/server/sonar-server/src/main/java/org/sonar/server/platform/web/RootFilter.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/platform/web/RootFilter.java
@@ -31,11 +31,14 @@ import javax.servlet.ServletRequest;
 import javax.servlet.ServletResponse;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpSession;
 import org.apache.commons.lang.StringUtils;
 import org.sonar.api.utils.log.Loggers;
 import org.sonar.api.utils.log.Profiler;
 
+import static java.lang.String.format;
+
 /**
  * <p>Profile HTTP requests using platform profiling utility.</p>
  * <p>To avoid profiling of requests for static resources, the <code>staticDirs</code>
@@ -72,23 +75,14 @@ public class RootFilter implements Filter {
   @Override
   public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
     if (request instanceof HttpServletRequest) {
-      HttpServletRequest httpRequest = new ServletRequestWrapper((HttpServletRequest) request);
-      String requestUri = httpRequest.getRequestURI();
-      String rootDir = getRootDir(requestUri);
-
-      if (staticResourceDirs.contains(rootDir)) {
-        // Static resource, not profiled
-        chain.doFilter(httpRequest, response);
-      } else {
-        Profiler profiler = Profiler.createIfDebug(Logger).start();
-        try {
-          chain.doFilter(httpRequest, response);
-        } finally {
-          if (profiler.isDebugEnabled()) {
-            String queryString = httpRequest.getQueryString();
-            String message = String.format(queryString == null ? MESSAGE_WITHOUT_QUERY : MESSAGE_WITH_QUERY, httpRequest.getMethod(), requestUri, queryString);
-            profiler.stopDebug(message);
-          }
+      HttpServletRequest httpRequest = (HttpServletRequest) request;
+      HttpServletResponse httpResponse = (HttpServletResponse) response;
+      try {
+        doFilter(new ServletRequestWrapper(httpRequest), httpResponse, chain);
+      } catch (Throwable e) {
+        Loggers.get(RootFilter.class).error(format("Processing of request %s failed", toUrl(httpRequest)), e);
+        if (!response.isCommitted()) {
+          httpResponse.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
         }
       }
     } else {
@@ -97,6 +91,36 @@ public class RootFilter implements Filter {
     }
   }
 
+  private static String toUrl(HttpServletRequest request) {
+    String requestURI = request.getRequestURI();
+    String queryString = request.getQueryString();
+    if (queryString == null) {
+      return requestURI;
+    }
+    return requestURI + '?' + queryString;
+  }
+
+  private void doFilter(HttpServletRequest httpRequest, HttpServletResponse response, FilterChain chain) throws IOException, ServletException {
+    String requestUri = httpRequest.getRequestURI();
+    String rootDir = getRootDir(requestUri);
+
+    if (staticResourceDirs.contains(rootDir)) {
+      // Static resource, not profiled
+      chain.doFilter(httpRequest, response);
+    } else {
+      Profiler profiler = Profiler.createIfDebug(Logger).start();
+      try {
+        chain.doFilter(httpRequest, response);
+      } finally {
+        if (profiler.isDebugEnabled()) {
+          String queryString = httpRequest.getQueryString();
+          String message = format(queryString == null ? MESSAGE_WITHOUT_QUERY : MESSAGE_WITH_QUERY, httpRequest.getMethod(), requestUri, queryString);
+          profiler.stopDebug(message);
+        }
+      }
+    }
+  }
+
   private String getRootDir(String requestUri) {
     String rootPath = "";
     String localPath = StringUtils.substringAfter(requestUri, contextRoot);

diff --git a/server/sonar-server/src/test/java/org/sonar/server/platform/web/RootFilterTest.java b/server/sonar-server/src/test/java/org/sonar/server/platform/web/RootFilterTest.java
index aff2ff1b64223a6900d9de4e97db9bb99412a0b7..038787742128f6cdb4996798f52248d793b3a944 100644 (file)
--- a/server/sonar-server/src/test/java/org/sonar/server/platform/web/RootFilterTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/platform/web/RootFilterTest.java
@@ -22,20 +22,21 @@ package org.sonar.server.platform.web;
 import javax.servlet.FilterChain;
 import javax.servlet.FilterConfig;
 import javax.servlet.ServletContext;
-import javax.servlet.ServletException;
 import javax.servlet.ServletRequest;
 import javax.servlet.ServletResponse;
 import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import org.mockito.ArgumentCaptor;
-import org.mockito.Mockito;
 
 import static org.assertj.core.api.Java6Assertions.assertThat;
 import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
@@ -75,10 +76,22 @@ public class RootFilterTest {
     filter.doFilter(request("POST", "/context", null), null, chain);
   }
 
-  @Test(expected = ServletException.class)
-  public void should_profile_even_when_exception() throws Exception {
-    Mockito.doThrow(new ServletException()).when(chain).doFilter(any(ServletRequest.class), any(ServletResponse.class));
-    filter.doFilter(request("POST", "/context/service/call", "param=value"), null, chain);
+  @Test
+  public void throwable_in_dofilter_is_caught_and_500_error_returned_if_response_is_not_committed() throws Exception {
+    doThrow(new RuntimeException()).when(chain).doFilter(any(ServletRequest.class), any(ServletResponse.class));
+    HttpServletResponse response = mockHttpResponse(false);
+    filter.doFilter(request("POST", "/context/service/call", "param=value"), response, chain);
+
+    verify(response).sendError(500);
+  }
+
+  @Test
+  public void throwable_in_dofilter_is_caught_but_no_500_response_is_sent_if_response_already_committed() throws Exception {
+    doThrow(new RuntimeException()).when(chain).doFilter(any(ServletRequest.class), any(ServletResponse.class));
+    HttpServletResponse response = mockHttpResponse(true);
+    filter.doFilter(request("POST", "/context/service/call", "param=value"), response, chain);
+
+    verify(response, never()).sendError(500);
   }
 
   @Test
@@ -149,4 +162,10 @@ public class RootFilterTest {
     when(request.getQueryString()).thenReturn(query);
     return request;
   }
+
+  private HttpServletResponse mockHttpResponse(boolean commited) {
+    HttpServletResponse response = mock(HttpServletResponse.class);
+    when(response.isCommitted()).thenReturn(commited);
+    return response;
+  }
 }