rhbz988202 - ensure when exception occur semaphore is released

functional-test/src/main/java/org/zanata/rest/SampleProjectResource.java

@@ -36,4 +36,17 @@ Response userJoinsLanguageTeams(@PathParam("username") String username,
 
     @DELETE
     Response deleteExceptEssentialData();
+
+    /**
+     * This dummy service can be used to simulate long running operation or throws exception.
+     *
+     * @param timeInMillis time used running this service
+     * @param qualifiedExceptionClass exception to be thrown if not null
+     * @return ok otherwise
+     * @throws Throwable represented by qualifiedExceptionClass
+     */
+    @GET
+    @Path("/dummy")
+    Response dummyService(@QueryParam("timeUsedInMillis") long timeInMillis,
+            @QueryParam("exception") String qualifiedExceptionClass) throws Throwable;
 }

functional-test/src/main/java/org/zanata/rest/SampleProjectResourceImpl.java

@@ -2,6 +2,7 @@
 
 import java.util.List;
 import java.util.Set;
+import java.util.concurrent.TimeUnit;
 import javax.persistence.EntityManager;
 import javax.ws.rs.Path;
 import javax.ws.rs.PathParam;
@@ -15,13 +16,19 @@
 import org.zanata.model.HLocale;
 import org.zanata.model.HPerson;
 import org.zanata.util.SampleProjectProfile;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import com.google.common.base.Throwables;
+import com.google.common.util.concurrent.Uninterruptibles;
+import lombok.extern.slf4j.Slf4j;
 
 /**
  * @author Patrick Huang <a
  *         href="mailto:pahuang@redhat.com">pahuang@redhat.com</a>
  */
 @Path("/test/data/sample")
 @Name("sampleProjectResourceImpl")
+@Slf4j
 public class SampleProjectResourceImpl implements SampleProjectResource {
 
     @In(create = true)
@@ -91,4 +98,34 @@ public void execute() {
         }.addRole("admin").run();
         return Response.ok().build();
     }
+
+    @Override
+    public Response dummyService(long timeInMillis,
+            String qualifiedExceptionClass) throws Throwable {
+        if (timeInMillis > 0) {
+            log.info("I am going to take a nap for {} ms", timeInMillis);
+            Uninterruptibles.sleepUninterruptibly(timeInMillis,
+                    TimeUnit.MILLISECONDS);
+        }
+        if (Strings.isNullOrEmpty(qualifiedExceptionClass)) {
+            return Response.ok().build();
+        }
+
+        try {
+            Class<?> exceptionClass = Class.forName(qualifiedExceptionClass);
+            boolean isThrowable =
+                    Throwable.class.isAssignableFrom(exceptionClass);
+            if (!isThrowable) {
+                return Response
+                        .status(Response.Status.BAD_REQUEST)
+                        .entity(qualifiedExceptionClass + " is not a Throwable")
+                        .build();
+            }
+            log.info("about to throw exception: {}", exceptionClass);
+            throw (Throwable) exceptionClass.newInstance();
+        } catch (Exception e) {
+            return Response.status(Response.Status.BAD_REQUEST)
+                    .entity(e.getMessage()).build();
+        }
+    }
 }

functional-test/src/test/java/org/zanata/feature/misc/RateLimitRestAndUITest.java

@@ -11,7 +11,6 @@
 
 import org.hamcrest.Matchers;
 import org.jboss.resteasy.client.ClientRequest;
-import org.jboss.resteasy.client.ClientResponse;
 import org.jboss.resteasy.client.core.BaseClientResponse;
 import org.jboss.resteasy.util.GenericType;
 import org.junit.Rule;
@@ -36,6 +35,7 @@
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.zanata.model.HApplicationConfiguration.KEY_ADMIN_EMAIL;
+import static org.zanata.model.HApplicationConfiguration.KEY_MAX_ACTIVE_REQ_PER_API_KEY;
 import static org.zanata.model.HApplicationConfiguration.KEY_MAX_CONCURRENT_REQ_PER_API_KEY;
 import static org.zanata.model.HApplicationConfiguration.KEY_RATE_LIMIT_PER_SECOND;
 import static org.zanata.util.ZanataRestCaller.checkStatusAndReleaseConnection;
@@ -53,13 +53,14 @@ public class RateLimitRestAndUITest {
 
     // because of the time based nature, tests may fail occasionally
 //    @Rule
-    public RetryRule retryRule = new RetryRule(3);
+    public RetryRule retryRule = new RetryRule(2);
 
     private static final String TRANSLATOR = "translator";
     private static final String TRANSLATOR_API =
             "d83882201764f7d339e97c4b087f0806";
     private String rateLimitingPathParam = "c/" + KEY_RATE_LIMIT_PER_SECOND;
     private String maxConcurrentPathParam = "c/" + KEY_MAX_CONCURRENT_REQ_PER_API_KEY;
+    private String maxActivePathParam = "c/" + KEY_MAX_ACTIVE_REQ_PER_API_KEY;
 
     @Test
     public void canConfigureRateLimitByWebUI() {
@@ -238,6 +239,46 @@ ImmutableList.<Callable<Integer>> builder()
                 Matchers.containsInAnyOrder(201, 201, 201, 201, 403));
     }
 
+    @Test(timeout = 5000)
+    public void exceptionWillReleaseSemaphore() throws Exception {
+        // Given: max active is set to 1
+        ClientRequest configRequest = clientRequestAsAdmin(
+                "rest/configurations/" + maxActivePathParam);
+        configRequest.body("text/plain", "1");
+        checkStatusAndReleaseConnection(configRequest.put());
+
+        // When: multiple requests that will result in a mapped exception
+        ClientRequest clientRequest = clientRequestAsAdmin(
+                "rest/test/data/sample/dummy?exception=org.zanata.rest.NoSuchEntityException");
+        getStatusAndReleaseConnection(clientRequest.get());
+        getStatusAndReleaseConnection(clientRequest.get());
+        getStatusAndReleaseConnection(clientRequest.get());
+        getStatusAndReleaseConnection(clientRequest.get());
+
+        // Then: request that result in exception should still release semaphore. i.e. no permit leak
+        assertThat(1, Matchers.is(1));
+    }
+
+    @Test(timeout = 5000)
+    public void unmappedExceptionWillAlsoReleaseSemaphore() throws Exception {
+        // Given: max active is set to 1
+        ClientRequest configRequest = clientRequestAsAdmin(
+                "rest/configurations/" + maxActivePathParam);
+        configRequest.body("text/plain", "1");
+        checkStatusAndReleaseConnection(configRequest.put());
+
+        // When: multiple requests that will result in an unmapped exception
+        ClientRequest clientRequest = clientRequestAsAdmin(
+                "rest/test/data/sample/dummy?exception=java.lang.RuntimeException");
+        getStatusAndReleaseConnection(clientRequest.get());
+        getStatusAndReleaseConnection(clientRequest.get());
+        getStatusAndReleaseConnection(clientRequest.get());
+        getStatusAndReleaseConnection(clientRequest.get());
+
+        // Then: request that result in exception should still release semaphore. i.e. no permit leak
+        assertThat(1, Matchers.is(1));
+    }
+
     private static Integer invokeRestService(ZanataRestCaller restCaller,
             String projectSlug, String iterationSlug,
             AtomicInteger atomicInteger) {

zanata-war/src/main/java/org/zanata/rest/AuthorizationExceptionMapper.java

@@ -8,11 +8,12 @@
 import org.jboss.seam.security.AuthorizationException;
 
 @Provider
-public class AuthorizationExceptionMapper implements
-        ExceptionMapper<AuthorizationException> {
+public class AuthorizationExceptionMapper extends
+        RateLimitingAwareExceptionMapper implements ExceptionMapper<AuthorizationException> {
 
     @Override
     public Response toResponse(AuthorizationException exception) {
+        releaseSemaphoreBeforeReturnResponse();
         return Response.status(Status.UNAUTHORIZED).build();
     }
 

zanata-war/src/main/java/org/zanata/rest/ConstraintViolationExceptionMapper.java

@@ -13,10 +13,12 @@
 
 @Provider
 @Slf4j
-public class ConstraintViolationExceptionMapper implements
+public class ConstraintViolationExceptionMapper extends
+        RateLimitingAwareExceptionMapper implements
         ExceptionMapper<ConstraintViolationException> {
     @Override
     public Response toResponse(ConstraintViolationException e) {
+        releaseSemaphoreBeforeReturnResponse();
         Set<ConstraintViolation<?>> invalidValues = e.getConstraintViolations();
         for (ConstraintViolation<?> invalidValue : invalidValues) {
             log.error("Invalid state for leaf bean {}: {}", e,

zanata-war/src/main/java/org/zanata/rest/HibernateExceptionMapper.java

@@ -8,14 +8,17 @@
 import lombok.extern.slf4j.Slf4j;
 
 import org.hibernate.HibernateException;
+import org.zanata.webtrans.server.HibernateIntegrator;
 
 @Provider
 @Slf4j
-public class HibernateExceptionMapper implements
+public class HibernateExceptionMapper extends
+        RateLimitingAwareExceptionMapper implements
         ExceptionMapper<HibernateException> {
 
     @Override
     public Response toResponse(HibernateException exception) {
+        releaseSemaphoreBeforeReturnResponse();
         log.error("Hibernate Exception in REST request", exception);
         return Response.status(Status.INTERNAL_SERVER_ERROR).build();
     }

zanata-war/src/main/java/org/zanata/rest/NoSuchEntityExceptionMapper.java

@@ -6,11 +6,13 @@
 import javax.ws.rs.ext.Provider;
 
 @Provider
-public class NoSuchEntityExceptionMapper implements
+public class NoSuchEntityExceptionMapper extends
+        RateLimitingAwareExceptionMapper implements
         ExceptionMapper<NoSuchEntityException> {
 
     @Override
     public Response toResponse(NoSuchEntityException exception) {
+        releaseSemaphoreBeforeReturnResponse();
         return Response.status(Status.NOT_FOUND).entity(exception.getMessage())
                 .build();
     }

zanata-war/src/main/java/org/zanata/rest/NotLoggedInExceptionMapper.java

@@ -8,11 +8,13 @@
 import org.jboss.seam.security.NotLoggedInException;
 
 @Provider
-public class NotLoggedInExceptionMapper implements
+public class NotLoggedInExceptionMapper extends
+        RateLimitingAwareExceptionMapper implements
         ExceptionMapper<NotLoggedInException> {
 
     @Override
     public Response toResponse(NotLoggedInException exception) {
+        releaseSemaphoreBeforeReturnResponse();
         return Response.status(Status.UNAUTHORIZED).build();
     }
 

zanata-war/src/main/java/org/zanata/rest/RateLimitingAwareExceptionMapper.java

@@ -0,0 +1,35 @@
+package org.zanata.rest;
+
+
+/**
+ * This should be the base of ALL REST exception mapper once we introduce rate
+ * limiting.
+ *
+ * Note: I can't use template method pattern here to enforce all subclass has
+ * called releaseSemaphore method in toResponse method. RESTeasy looks at
+ * generic interface and grab the type out of there. Child class has to provide
+ * a toResponse method with exact exception as argument. Therefore this class
+ * mainly serve as an marker mechanism where there is a test to scan all
+ * ExceptionMapper implementation and ensure they are all subclass of this type
+ * (so that when developers implement new mapper they won't forget to call
+ * release semaphore)
+ *
+ * @see <a href=
+ *      "http://stackoverflow.com/questions/13937998/resteasy-post-process-interceptor-chain-not-traversed-when-response-created-by-e">RestEasy
+ *      Post Process Interceptor chain not traversed when response created by
+ *      ExceptionMapper</a>
+ *
+ * @see org.jboss.resteasy.spi.ResteasyProviderFactory#addExceptionMapper(javax.ws.rs.ext.ExceptionMapper, java.lang.Class)
+ *
+ * @author Patrick Huang <a
+ *         href="mailto:pahuang@redhat.com">pahuang@redhat.com</a>
+ */
+public abstract class RateLimitingAwareExceptionMapper {
+
+    /**
+     * Just remember to call this method before returning your Response.
+     */
+    protected void releaseSemaphoreBeforeReturnResponse() {
+        RateLimiterHolder.getInstance().releaseSemaphoreForCurrentThread();
+    }
+}

zanata-war/src/main/java/org/zanata/rest/ReadOnlyEntityExceptionMapper.java

@@ -6,13 +6,14 @@
 import javax.ws.rs.ext.Provider;
 
 @Provider
-public class ReadOnlyEntityExceptionMapper implements
+public class ReadOnlyEntityExceptionMapper extends
+        RateLimitingAwareExceptionMapper implements
         ExceptionMapper<ReadOnlyEntityException> {
 
     @Override
     public Response toResponse(ReadOnlyEntityException exception) {
+        releaseSemaphoreBeforeReturnResponse();
         return Response.status(Status.FORBIDDEN).entity(exception.getMessage())
                 .build();
     }
-
 }

zanata-war/src/main/java/org/zanata/rest/TMXParseExceptionMapper.java

@@ -32,10 +32,12 @@
  *         href="mailto:camunoz@redhat.com">camunoz@redhat.com</a>
  */
 @Provider
-public class TMXParseExceptionMapper implements
+public class TMXParseExceptionMapper extends
+        RateLimitingAwareExceptionMapper implements
         ExceptionMapper<TMXParseException> {
     @Override
     public Response toResponse(TMXParseException exception) {
+        releaseSemaphoreBeforeReturnResponse();
         return Response.status(Status.BAD_REQUEST)
                 .entity(exception.getMessage()).build();
     }

zanata-war/src/main/java/org/zanata/rest/ZanataResteasyBootstrap.java

@@ -106,6 +106,9 @@ public void invoke(HttpRequest request, HttpResponse response) {
                         log.error(
                                 "Failed to send error on failed REST request",
                                 ioe);
+                    } finally {
+                        RateLimiterHolder.getInstance()
+                                .releaseSemaphoreForCurrentThread();
                     }
                 }
             }

zanata-war/src/main/java/org/zanata/rest/ZanataServiceExceptionMapper.java

@@ -31,10 +31,12 @@
  *         href="mailto:camunoz@redhat.com">camunoz@redhat.com</a>
  */
 @Provider
-public class ZanataServiceExceptionMapper implements
+public class ZanataServiceExceptionMapper extends
+        RateLimitingAwareExceptionMapper implements
         ExceptionMapper<ZanataServiceException> {
     @Override
     public Response toResponse(ZanataServiceException exception) {
+        releaseSemaphoreBeforeReturnResponse();
         return Response.status(exception.getHttpStatus())
                 .entity(exception.getMessage()).build();
     }