Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8257671

ThreadPoolExecutor.Discard*Policy: rejected tasks are not cancelled

XMLWordPrintable

    • b05
    • generic
    • generic
    • Verified

      A DESCRIPTION OF THE PROBLEM :
      Imagine this:

      ```java
      // Note the DiscardPolicy, and a capacity of 1
      ExecutorService executor = new ThreadPoolExecutor(
          1, 1,
          1, TimeUnit.MINUTES,
          new ArrayBlockingQueue<>(1),
          new ThreadPoolExecutor.DiscardPolicy()
      );

      Runnable task = () -> Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
      executor.submit(task);
      executor.submit(task);

      // The following task will get rejected and discarded by the pool:
      executor.submit(task).get();
      ```

      The code above will block forever, the `get()` call never returns. The task
      had been rejected, but the returned Future is not cancelled, will never run,
      and therefore will never be completed in any way. There's no way I'm aware
      of to know the returned Future had been rejected, the future is broken and
      not useful in any way.

      This was very confusing and unexpected to me, I definitely expected the policy
      to cancel the task. Note that this only happens with the Discard* policies
      because the AbortPolicy throws and never returns a broken Future to the user,
      while the CallerRunsPolicy returns a runnable Future.

      This issue has been previously discussed in a thread on concurrency-interest:
      http://cs.oswego.edu/pipermail/concurrency-interest/2020-February/017068.html
      and in short, the consensus is that we probably do not want to change any of the
      Discard*Policies not introduce a new policy, but rather document the current state
      of affairs in a JavaDoc and via a soft deprecation.

      For completeness, here are my original proposals:

      >>>
      I'd like to open a discussion around a few possible naive approaches to ease
      the pain of future developers who get similarly caught by surprise:

      1. Change the Discard*Policy to cancel the task. I did not find any place
      where this would break any existing contracts. That said, obviously such
      a change changes the behaviour in a significant way, and so it might not be
      the way to go.

      2. Introduce a DiscardAndCancelPolicy. Yes, writing such a policy is trivial
      and anybody can do it if he needs it. The problem is that this is so obscure
      that I cannot imagine many people do this right away. Discoverability is very
      important here, and having an extra policy would tip people off.

      3. Change the Discard*Policie's JavaDoc in a way it is clear that it does not
      play with ExecutorService.submit() very well.

      4. All of the above, or some more complex solution?
      >>>

      STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
      // Note the DiscardPolicy, and a capacity of 1
      ExecutorService executor = new ThreadPoolExecutor(
          1, 1,
          1, TimeUnit.MINUTES,
          new ArrayBlockingQueue<>(1),
          new ThreadPoolExecutor.DiscardPolicy()
      );

      Runnable task = () -> Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
      executor.submit(task);
      executor.submit(task);

      // The following task will get rejected and discarded by the pool:
      executor.submit(task).get();

      EXPECTED VERSUS ACTUAL BEHAVIOR :
      EXPECTED -
      I believe the Future::get call should immediately throw a CancellationException as the task had been rejected.
      ACTUAL -
      The Future::get call on a rejected future never finishes.

      ---------- BEGIN SOURCE ----------
      package com.gitlab.janecekpetr.playground;

      import com.google.common.util.concurrent.MoreExecutors;
      import com.google.common.util.concurrent.Uninterruptibles;

      import org.junit.jupiter.api.AfterEach;
      import org.junit.jupiter.api.Test;
      import org.junit.jupiter.api.Timeout;

      import java.time.Duration;
      import java.util.concurrent.ArrayBlockingQueue;
      import java.util.concurrent.ExecutorService;
      import java.util.concurrent.Future;
      import java.util.concurrent.ThreadPoolExecutor;
      import java.util.concurrent.TimeUnit;

      public class TpeDiscardPolicyRejectionTest {

          private final ExecutorService executor = new ThreadPoolExecutor(
                  1, 1,
                  1, TimeUnit.MINUTES,
                  new ArrayBlockingQueue<>(1),
                  new ThreadPoolExecutor.DiscardPolicy()
              );

          @Test
          @Timeout(value = 5, unit = TimeUnit.SECONDS)
          public void testRejectedTask() throws Exception {
              // Note the DiscardPolicy, and a capacity of 1
              ExecutorService executor = new ThreadPoolExecutor(
                  1, 1,
                  1, TimeUnit.MINUTES,
                  new ArrayBlockingQueue<>(1),
                  new ThreadPoolExecutor.DiscardPolicy()
              );

              Runnable task = () -> Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
              executor.submit(task);
              executor.submit(task);

              Future<?> future = executor.submit(task); // Gets rejected and discarded by the pool.
              future.get(); // Blocks forever.
          }

          @AfterEach
          void cleanup() {
              MoreExecutors.shutdownAndAwaitTermination(executor, Duration.ofSeconds(5));
          }

      }
      ---------- END SOURCE ----------

      CUSTOMER SUBMITTED WORKAROUND :
      Avoid DiscardPolicy, use a custom DiscardAndCancelPolicy.

      FREQUENCY : always


            martin Martin Buchholz
            webbuggrp Webbug Group
            Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

              Created:
              Updated:
              Resolved: