ThreadPoolExecutor - cancel rejected tasks with Discard*Policy?

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

ThreadPoolExecutor - cancel rejected tasks with Discard*Policy?

JSR166 Concurrency mailing list
Hello,

I'm sorry to open a new thread on something I feel must have been discussed before,
I did not find any previous mention.

Imagine this:

```java
// Note the DiscardPolicy
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.

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.

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?

Thank you,
Petr Janeček
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor - cancel rejected tasks with Discard*Policy?

JSR166 Concurrency mailing list
On 2/12/20 5:01 AM, Petr Janeček via Concurrency-interest wrote:

> // 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.

Right. None of the supplied RejectedExecutionHandlers perform
task.cancel when discarding. In retrospect, they probably should have.
At this point, it would probably suffice to add discussion to javadocs
about how to do so. But if others support adding new Policies, we should
consider it.

-Doug

>
> 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.
>
> 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?
>
> Thank you,
> Petr Janeček
> _______________________________________________
> Concurrency-interest mailing list
> [hidden email]
> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor - cancel rejected tasks with Discard*Policy?

JSR166 Concurrency mailing list
In reply to this post by JSR166 Concurrency mailing list
The Executor interface is simply 
void execute(Runnable command)
which may have been a design mistake in retrospect (but it's analogous to Thread.run).
Giving a TODO to the executor should maybe require at least an acknowledgement in the form of a returned Future?
The default rejection policy is like DiscardPolicy, with "notification".
DiscardPolicy can be regarded as a "demo" Policy that might have been better confined to javadoc.
I agree with Doug that it may be safest today to soft-deprecate DiscardPolicy in the javadoc.

On Wed, Feb 12, 2020 at 2:03 AM Petr Janeček via Concurrency-interest <[hidden email]> wrote:
Hello,

I'm sorry to open a new thread on something I feel must have been discussed before,
I did not find any previous mention.

Imagine this:

```java
// Note the DiscardPolicy
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.

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.

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?

Thank you,
Petr Janeček
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor - cancel rejected tasks with Discard*Policy?

JSR166 Concurrency mailing list
An idea I tossed around for a while was to add chaining of RejectedExecutionHandler to DiscardPolicy and DiscardOldestPolicy.  
I never really spent the time to fully flesh out all of the problems so take it for what it is worth.
The idea is that only if the executor is shutdown then call the next RejectedExecutionHandler in the chain.  The caller would construct and specify the additional handler.
It would allow you to build things like DiscardOldest and CallerRuns or Discard and Abort.  If we added a cancel policy you could do Discard and Cancel.

Maybe we wouldn't have to deprecate if added chaining?

Jason

________________________________________
From: Concurrency-interest <[hidden email]> on behalf of Martin Buchholz via Concurrency-interest <[hidden email]>
Sent: Thursday, February 13, 2020 9:09 AM
To: Petr Janeček
Cc: concurrency-interest
Subject: Re: [concurrency-interest] ThreadPoolExecutor - cancel rejected tasks with Discard*Policy?

The Executor interface is simply
void execute(Runnable command)
which may have been a design mistake in retrospect (but it's analogous to Thread.run).
Giving a TODO to the executor should maybe require at least an acknowledgement in the form of a returned Future?
The default rejection policy is like DiscardPolicy, with "notification".
DiscardPolicy can be regarded as a "demo" Policy that might have been better confined to javadoc.
I agree with Doug that it may be safest today to soft-deprecate DiscardPolicy in the javadoc.

On Wed, Feb 12, 2020 at 2:03 AM Petr Janeček via Concurrency-interest <[hidden email]<mailto:[hidden email]>> wrote:
Hello,

I'm sorry to open a new thread on something I feel must have been discussed before,
I did not find any previous mention.

Imagine this:

```java
// Note the DiscardPolicy
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.

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.

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?

Thank you,
Petr Janeček
_______________________________________________
Concurrency-interest mailing list
[hidden email]<mailto:[hidden email]>
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor - cancel rejected tasks with Discard*Policy?

JSR166 Concurrency mailing list
In reply to this post by JSR166 Concurrency mailing list
Looks like the opinions so far have been split almost evenly.

What is the next step? Should i file a bug in the JDK bug tracker, send an email to core-libs-dev, or shall we just continue here?

I'd be more than happy to help in any way -doesn't matter if that is providing some code for experimentation and regression testing, first JavaDoc suggestions (that said, I'm not a native speaker, so perhaps others would be better), waiting patiently until you resolve it, driving discussion elsewhere.

Thank you, have a nice weekend!
PJ

On Feb 12, 2020 15:26, Doug Lea via Concurrency-interest <[hidden email]> wrote:


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor - cancel rejected tasks with Discard*Policy?

JSR166 Concurrency mailing list
On 2/14/20 3:06 PM, [hidden email] wrote:
> Looks like the opinions so far have been split almost evenly.

This is what happens when we agree that we regret a small decision made
many years ago; there is rarely a great solution. Probably we'll just
add documentation. See the draft embedded example at:

http://gee.cs.oswego.edu/dl/jsr166/dist/docs/java.base/java/util/concurrent/ThreadPoolExecutor.DiscardOldestPolicy.html

Plus a few added sentences in TPE documentation
http://gee.cs.oswego.edu/dl/jsr166/dist/docs/java.base/java/util/concurrent/ThreadPoolExecutor.html

We could add @Deprecated(forRemoval=false), but I'm not sure how much
this would help. There are a few legitimate uses for it, and that code
would now encounter warnings.

Comments still welcome.

-Doug


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor - cancel rejected tasks with Discard*Policy?

JSR166 Concurrency mailing list
In reply to this post by JSR166 Concurrency mailing list
The real downside with CallerRunsPolicy is that it discards Runnables when the pool is shutting down, which means that in all cases where there might be a race condition between shutting down and submitting tasks, they may not get run at all :S

On Wed, Feb 12, 2020 at 10:03 AM Petr Janeček via Concurrency-interest <[hidden email]> wrote:
Hello,

I'm sorry to open a new thread on something I feel must have been discussed before,
I did not find any previous mention.

Imagine this:

```java
// Note the DiscardPolicy
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.

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.

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?

Thank you,
Petr Janeček
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest


--
Cheers,

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor - cancel rejected tasks with Discard*Policy?

JSR166 Concurrency mailing list
On 2/16/20 10:16 AM, Viktor Klang via Concurrency-interest wrote:
> The real downside with CallerRunsPolicy is that it discards Runnables
> when the pool is shutting down, which means that in all cases where
> there might be a race condition between shutting down and submitting
> tasks, they may not get run at all :S
>

Some people would complain about the opposite policy choice as a
downside. The main moral for JDK components is that predefining only a
few of many possibly policies for the sake of convenience is not usually
a good idea.

-Doug



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor - cancel rejected tasks with Discard*Policy?

JSR166 Concurrency mailing list


On Mon, Feb 17, 2020 at 12:03 PM Doug Lea via Concurrency-interest <[hidden email]> wrote:
On 2/16/20 10:16 AM, Viktor Klang via Concurrency-interest wrote:
> The real downside with CallerRunsPolicy is that it discards Runnables
> when the pool is shutting down, which means that in all cases where
> there might be a race condition between shutting down and submitting
> tasks, they may not get run at all :S
>

Some people would complain about the opposite policy choice as a
downside. The main moral for JDK components is that predefining only a
few of many possibly policies for the sake of convenience is not usually
a good idea.

Yeah, "There is no optimal, general, solution." - Old Klangian Proverb
 

-Doug



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest


--
Cheers,

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ThreadPoolExecutor - cancel rejected tasks with Discard*Policy?

JSR166 Concurrency mailing list
In reply to this post by JSR166 Concurrency mailing list
> From: Doug Lea <[hidden email]>
> Datum: 16. 2. 2020 15:43:13
>
> Subject: Re: [concurrency-interest] ThreadPoolExecutor - cancel rejected tasks with Discard*Policy?
>
> [...] Probably we'll just add documentation. See the draft embedded example at:
> http://gee.cs.oswego.edu/dl/jsr166/dist/docs/java.base/java/util/concurrent/ThreadPoolExecutor.DiscardOldestPolicy.html
>
> Plus a few added sentences in TPE documentation
> http://gee.cs.oswego.edu/dl/jsr166/dist/docs/java.base/java/util/concurrent/ThreadPoolExecutor.html

Thank you, that's very clear.
I would personally like a new policy, too, but I admit its usage is probably
obscure enough that it would not hold its own weight.
 
> We could add @Deprecated(forRemoval=false), but I'm not sure how much
> this would help. There are a few legitimate uses for it, and that code
> would now encounter warnings.

Agreed, I subjectively do not think @Deprecated is warranted in this case.

PJ
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest