Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=49730

Race condition in executor can lead to long delays in request processing

git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1003183 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/java/org/apache/tomcat/util/threads/TaskQueue.java b/java/org/apache/tomcat/util/threads/TaskQueue.java
index 757acc0..6471dbc 100644 (file)
--- a/java/org/apache/tomcat/util/threads/TaskQueue.java
+++ b/java/org/apache/tomcat/util/threads/TaskQueue.java
@@ -19,8 +19,8 @@ package org.apache.tomcat.util.threads;
 import java.util.Collection;
 import java.util.concurrent.LinkedBlockingQueue;
 import java.util.concurrent.RejectedExecutionException;
-import java.util.concurrent.ThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
+
 /**
  * As task queue specifically designed to run with a thread pool executor.
  * The task queue is optimised to properly utilize threads within 
@@ -65,7 +65,7 @@ public class TaskQueue extends LinkedBlockingQueue<Runnable> {
         //we are maxed out on threads, simply queue the object
         if (parent.getPoolSize() == parent.getMaximumPoolSize()) return super.offer(o);
         //we have idle threads, just add it to the queue
-        if (parent.getActiveCount()<(parent.getPoolSize())) return super.offer(o);
+        if (parent.getSubmittedCount()<(parent.getPoolSize())) return super.offer(o);
         //if we have less threads than maximum force creation of a new thread
         if (parent.getPoolSize()<parent.getMaximumPoolSize()) return false;
         //if we reached here, we need to add it to the queue

diff --git a/java/org/apache/tomcat/util/threads/ThreadPoolExecutor.java b/java/org/apache/tomcat/util/threads/ThreadPoolExecutor.java
index 271b3fb..9f02bc5 100644 (file)
--- a/java/org/apache/tomcat/util/threads/ThreadPoolExecutor.java
+++ b/java/org/apache/tomcat/util/threads/ThreadPoolExecutor.java
@@ -24,7 +24,7 @@ import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 /**
  * Same as a java.util.concurrent.ThreadPoolExecutor but implements a much more efficient
- * getActiveCount method, to be used to properly handle the work queue
+ * {@link #getSubmittedCount()} method, to be used to properly handle the work queue.
  * If a RejectedExecutionHandler is not specified a default one will be configured
  * and that one will always throw a RejectedExecutionException
  * @author fhanik
@@ -32,7 +32,13 @@ import java.util.concurrent.atomic.AtomicInteger;
  */
 public class ThreadPoolExecutor extends java.util.concurrent.ThreadPoolExecutor {
     
-    private final AtomicInteger activeCount = new AtomicInteger(0);
+    /**
+     * The number of tasks submitted but not yet finished. This includes tasks
+     * in the queue and tasks that have been handed to a worker thread but the
+     * latter did not start executing the task yet.
+     * This number is always greater or equal to {@link #getActiveCount()}.
+     */
+    private final AtomicInteger submittedCount = new AtomicInteger(0);
     
     public ThreadPoolExecutor(int corePoolSize, int maximumPoolSize, long keepAliveTime, TimeUnit unit, BlockingQueue<Runnable> workQueue, RejectedExecutionHandler handler) {
         super(corePoolSize, maximumPoolSize, keepAliveTime, unit, workQueue, handler);
@@ -53,17 +59,11 @@ public class ThreadPoolExecutor extends java.util.concurrent.ThreadPoolExecutor
 
     @Override
     protected void afterExecute(Runnable r, Throwable t) {
-        activeCount.decrementAndGet();
-    }
-
-    @Override
-    protected void beforeExecute(Thread t, Runnable r) {
-        activeCount.incrementAndGet();
+        submittedCount.decrementAndGet();
     }
 
-    @Override
-    public int getActiveCount() {
-        return activeCount.get();
+    public int getSubmittedCount() {
+        return submittedCount.get();
     }
     
     /**
@@ -88,6 +88,7 @@ public class ThreadPoolExecutor extends java.util.concurrent.ThreadPoolExecutor
      * @throws NullPointerException if command or unit is null
      */
     public void execute(Runnable command, long timeout, TimeUnit unit) {
+        submittedCount.incrementAndGet();
         try {
             super.execute(command);
         } catch (RejectedExecutionException rx) {
@@ -95,13 +96,16 @@ public class ThreadPoolExecutor extends java.util.concurrent.ThreadPoolExecutor
                 final TaskQueue queue = (TaskQueue)super.getQueue();
                 try {
                     if (!queue.force(command, timeout, unit)) {
+                        submittedCount.decrementAndGet();
                         throw new RejectedExecutionException("Queue capacity is full.");
                     }
                 } catch (InterruptedException x) {
+                    submittedCount.decrementAndGet();
                     Thread.interrupted();
                     throw new RejectedExecutionException(x);
                 }
             } else {
+                submittedCount.decrementAndGet();
                 throw rx;
             }
             

diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 0616d9f..0b6b885 100644 (file)
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -87,6 +87,11 @@
         managing the PID file and Tomcat does not have write access. (markt)
       </fix>
       <fix>
+        <bug>49730</bug>: Fix a race condition in StandardThreadExector that can
+        cause requests to experience large delays. Patch provided by Sylvain
+        Laurent. (markt)
+      </fix>
+      <fix>
         <bug>49749</bug>: Single sign on cookies should have httpOnly flag set
         using same rules as session cookies. (markt)
       </fix>