-
Bug
-
Resolution: Fixed
-
P4
-
8, 11, 16
-
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
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