title: Java Concurrency in Practice: race condition in BoundedExecutor?
body: <p>There's something odd about the implementation of the <code>BoundedExecutor</code> in the book Java Concurrency in Practice.</p>
  
  <p>It's supposed to throttle task submission to the Executor by blocking the submitting thread when there are enough threads either queued or running in the Executor.</p>
  
  <p>This is the implementation (after adding the missing rethrow in the catch clause):</p>
  
  <pre><code>public class BoundedExecutor {
      private final Executor exec;
      private final Semaphore semaphore;
  
      public BoundedExecutor(Executor exec, int bound) {
          this.exec = exec;
          this.semaphore = new Semaphore(bound);
      }
  
      public void submitTask(final Runnable command) throws InterruptedException, RejectedExecutionException {
          semaphore.acquire();
  
          try {
              exec.execute(new Runnable() {
                  @Override public void run() {
                      try {
                          command.run();
                      } finally {
                          semaphore.release();
                      }
                  }
              });
          } catch (RejectedExecutionException e) {
              semaphore.release();
              throw e;
          }
      }
  </code></pre>
  
  <p>When I instantiate the <code>BoundedExecutor</code> with an <code>Executors.newCachedThreadPool()</code> and a bound of 4, I would expect the number of threads instantiated by the cached thread pool to never exceed 4. In practice, however, it does. I've gotten this little test program to create as much as 11 threads:</p>
  
  <pre><code>public static void main(String[] args) throws Exception {
      class CountingThreadFactory implements ThreadFactory {
          int count;
  
          @Override public Thread newThread(Runnable r) {
              ++count;
              return new Thread(r);
          }           
      }
  
      List&lt;Integer&gt; counts = new ArrayList&lt;Integer&gt;();
  
      for (int n = 0; n &lt; 100; ++n) {
          CountingThreadFactory countingThreadFactory = new CountingThreadFactory();
          ExecutorService exec = Executors.newCachedThreadPool(countingThreadFactory);
  
          try {
              BoundedExecutor be = new BoundedExecutor(exec, 4);
  
              for (int i = 0; i &lt; 20000; ++i) {
                  be.submitTask(new Runnable() {
                      @Override public void run() {}
                  });
              }
          } finally {
              exec.shutdown();
          }
  
          counts.add(countingThreadFactory.count);
      }
  
      System.out.println(Collections.max(counts));
  }
  </code></pre>
  
  <p>I think there's a tiny little time frame between the release of the semaphore and the task ending, where another thread can aquire a permit and submit a task while the releasing thread hasn't finished yet. In other words, it has a race condition.</p>
  
  <p>Can someone confirm this?</p>