CompletableFuture.cancel vs Future.cancel bug?

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

CompletableFuture.cancel vs Future.cancel bug?

JSR166 Concurrency mailing list
Hi,

I noticed that the CompletableFuture.cancel method breaks the contract for Future.cancel.
The spec says "This attempt will fail if the task ... has already been cancelled..." but at the same time CompletableFuture will return true even if someone else has cancelled the future.

I see that it can be workarounded by completeExceptionally(new CancellationException()) but this inconsistency is neither obvious nor even documented and IMO should be fixed.

To be clear I'm extending CompletableFuture.cancel with my own cancellation logic and obviously I want this logic to be executed only once thus using super.cancel() would be wrong. On another hand I want my future to behave consistently with the original CompletableFuture and of course I can emulate that but this all gets ugly...

Please advise.

Sergi

 

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

Re: CompletableFuture.cancel vs Future.cancel bug?

JSR166 Concurrency mailing list
Investigated further: FutureTask respects the Future.cancel contract, ForkJoinTask does not.
Also Guava AbstractFuture implements cancel correctly.

Practically this means that users can not rely on the Future.cancel() correctness.


On Thu, Aug 20, 2020 at 11:11 AM Sergi Vladykin <[hidden email]> wrote:
Hi,

I noticed that the CompletableFuture.cancel method breaks the contract for Future.cancel.
The spec says "This attempt will fail if the task ... has already been cancelled..." but at the same time CompletableFuture will return true even if someone else has cancelled the future.

I see that it can be workarounded by completeExceptionally(new CancellationException()) but this inconsistency is neither obvious nor even documented and IMO should be fixed.

To be clear I'm extending CompletableFuture.cancel with my own cancellation logic and obviously I want this logic to be executed only once thus using super.cancel() would be wrong. On another hand I want my future to behave consistently with the original CompletableFuture and of course I can emulate that but this all gets ugly...

Please advise.

Sergi

 

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

Re: CompletableFuture.cancel vs Future.cancel bug?

JSR166 Concurrency mailing list
You didn't make it completely clear - I think you're saying that the
meaning of the return value from FutureTask.cancel() and
CompletableFuture.cancel() are inconsistent.  I agree, and this
bothers me a little as well.  The spec for Future.cancel() includes
the word "typically" so arguably there's enough wiggle room.

As usual, trying to fix this now will likely cause more trouble than
living with it.

On Thu, Aug 20, 2020 at 9:28 AM Sergi Vladykin via
Concurrency-interest <[hidden email]> wrote:

>
> Investigated further: FutureTask respects the Future.cancel contract, ForkJoinTask does not.
> Also Guava AbstractFuture implements cancel correctly.
>
> Practically this means that users can not rely on the Future.cancel() correctness.
>
>
> On Thu, Aug 20, 2020 at 11:11 AM Sergi Vladykin <[hidden email]> wrote:
>>
>> Hi,
>>
>> I noticed that the CompletableFuture.cancel method breaks the contract for Future.cancel.
>> The spec says "This attempt will fail if the task ... has already been cancelled..." but at the same time CompletableFuture will return true even if someone else has cancelled the future.
>>
>> I see that it can be workarounded by completeExceptionally(new CancellationException()) but this inconsistency is neither obvious nor even documented and IMO should be fixed.
>>
>> To be clear I'm extending CompletableFuture.cancel with my own cancellation logic and obviously I want this logic to be executed only once thus using super.cancel() would be wrong. On another hand I want my future to behave consistently with the original CompletableFuture and of course I can emulate that but this all gets ugly...
>>
>> Please advise.
>>
>> Sergi
>>
>>
>
> _______________________________________________
> 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: CompletableFuture.cancel vs Future.cancel bug?

JSR166 Concurrency mailing list
On Thu, Aug 20, 2020 at 8:48 PM Martin Buchholz <[hidden email]> wrote:
You didn't make it completely clear - I think you're saying that the
meaning of the return value from FutureTask.cancel() and
CompletableFuture.cancel() are inconsistent. 

I'm saying that CompletableFuture.cancel() is inconsistent with what I see in Future.cancel() javadoc.

So I looked at other Future implementations and see that:
  - FutureTask, Guava AbstractFuture - correct implementation of the spec;
  - CompletableFuture, ForkJoinTask - incorrect implementation
 
I agree, and this
bothers me a little as well.  The spec for Future.cancel() includes
the word "typically" so arguably there's enough wiggle room.

Here is the part with "typically":

     * @return {@code false} if the task could not be cancelled,
     * typically because it has already completed normally;
     * {@code true} otherwise

I read it as "usually it is impossible to cancel the task because it has already been completed normally, but there are other reasons: for example it has already been completed exceptionally or cancelled".

And I've already cited the beginning of that javadoc which explicitly states that the attempt WILL fail if the task has been cancelled:

     * Attempts to cancel execution of this task.  This attempt will
     * fail if the task has already completed, has already been cancelled,
     * or could not be cancelled for some other reason.
 
I see no room for wiggling here.
 

As usual, trying to fix this now will likely cause more trouble than
living with it.

Not sure in what situation the current behavior of CompletableFuture would be beneficial, but I definitely agree that fixing this may be risky now. Still, if we accept this de facto state of things at the very least we should update the Future.cancel() spec to reflect that and let users know that they should not rely on "This attempt will fail if the task has already been cancelled".
 

On Thu, Aug 20, 2020 at 9:28 AM Sergi Vladykin via
Concurrency-interest <[hidden email]> wrote:
>
> Investigated further: FutureTask respects the Future.cancel contract, ForkJoinTask does not.
> Also Guava AbstractFuture implements cancel correctly.
>
> Practically this means that users can not rely on the Future.cancel() correctness.
>
>
> On Thu, Aug 20, 2020 at 11:11 AM Sergi Vladykin <[hidden email]> wrote:
>>
>> Hi,
>>
>> I noticed that the CompletableFuture.cancel method breaks the contract for Future.cancel.
>> The spec says "This attempt will fail if the task ... has already been cancelled..." but at the same time CompletableFuture will return true even if someone else has cancelled the future.
>>
>> I see that it can be workarounded by completeExceptionally(new CancellationException()) but this inconsistency is neither obvious nor even documented and IMO should be fixed.
>>
>> To be clear I'm extending CompletableFuture.cancel with my own cancellation logic and obviously I want this logic to be executed only once thus using super.cancel() would be wrong. On another hand I want my future to behave consistently with the original CompletableFuture and of course I can emulate that but this all gets ugly...
>>
>> Please advise.
>>
>> Sergi
>>
>>
>
> _______________________________________________
> 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: CompletableFuture.cancel vs Future.cancel bug?

JSR166 Concurrency mailing list

I agree that the Future.cancel javadoc should be improved to better distinguish the return value spec from the method description. It was not the intent to require the return value to be a CAS-like indication of whether the current invocation was responsible for cancellation. It is nice when it can do so (and some implementations do), but the intention for this high-level interface spec was to allow a wide range of implementations,. Perhaps it would have been better to define cancel as void, requiring a call to isCancelled to check. In other words...

On 8/20/20 3:06 PM, Sergi Vladykin via Concurrency-interest wrote:

Here is the part with "typically":

     * @return {@code false} if the task could not be cancelled,
     * typically because it has already completed normally;
     * {@code true} otherwise

I read it as "usually it is impossible to cancel the task because it has already been completed normally, but there are other reasons: for example it has already been completed exceptionally or cancelled".
The case left out here is "already cancelled", which can go either way.

And I've already cited the beginning of that javadoc which explicitly states that the attempt WILL fail if the task has been cancelled:

     * Attempts to cancel execution of this task.  This attempt will
     * fail if the task has already completed, has already been cancelled,
     * or could not be cancelled for some other reason.
 
I see no room for wiggling here.

"The attempt" is not a description of return value. Sorry for the confusion.



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

Re: CompletableFuture.cancel vs Future.cancel bug?

JSR166 Concurrency mailing list
Doug,

Thanks for the clarification!

Yes, my interpretation was exactly "successful attempt" == "return true", "failed attempt" == "return false". Would be nice to have this spec improved.

Also, could you please clarify why the decision was made not to have CAS-like semantics for CompletableFuture.cancel() while it was clearly possible? Maybe there are some hidden benefits I don't see?

 

On Sun, Aug 23, 2020 at 3:34 PM Doug Lea via Concurrency-interest <[hidden email]> wrote:

I agree that the Future.cancel javadoc should be improved to better distinguish the return value spec from the method description. It was not the intent to require the return value to be a CAS-like indication of whether the current invocation was responsible for cancellation. It is nice when it can do so (and some implementations do), but the intention for this high-level interface spec was to allow a wide range of implementations,. Perhaps it would have been better to define cancel as void, requiring a call to isCancelled to check. In other words...

On 8/20/20 3:06 PM, Sergi Vladykin via Concurrency-interest wrote:

Here is the part with "typically":

     * @return {@code false} if the task could not be cancelled,
     * typically because it has already completed normally;
     * {@code true} otherwise

I read it as "usually it is impossible to cancel the task because it has already been completed normally, but there are other reasons: for example it has already been completed exceptionally or cancelled".
The case left out here is "already cancelled", which can go either way.

And I've already cited the beginning of that javadoc which explicitly states that the attempt WILL fail if the task has been cancelled:

     * Attempts to cancel execution of this task.  This attempt will
     * fail if the task has already completed, has already been cancelled,
     * or could not be cancelled for some other reason.
 
I see no room for wiggling here.

"The attempt" is not a description of return value. Sorry for the confusion.


_______________________________________________
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: CompletableFuture.cancel vs Future.cancel bug?

JSR166 Concurrency mailing list
On 8/23/20 9:30 AM, Sergi Vladykin wrote:
>
>
> Yes, my interpretation was exactly "successful attempt" == "return
> true", "failed attempt" == "return false". Would be nice to have this
> spec improved.
See draft update below, that also tries to clarify mayInterruptIfRunning.
>
> Also, could you please clarify why the decision was made not to have
> CAS-like semantics for CompletableFuture.cancel() while it was clearly
> possible? Maybe there are some hidden benefits I don't see?
>
I don't have a clear recollection (it was during JDK5), but it may have
been a reflection of issues with (concurrent) Collection remove()
methods requiring that the return value indicate that the current call
was responsible for removal, which is sometimes arbitrary and causes
extra expense and complexity. (We considered trying to weaken this, but
didn't.)

... possible javadoc update:

     /**
      * Attempts to cancel execution of this task.  This method has no
      * effect if the task is already completed or cancelled, and may
      * fail if the task could not be cancelled for any reason. If
      * successful, and this task has not started when {@code cancel}
      * is called, this task should never run.  If the task has already
      * started, then the {@code mayInterruptIfRunning} parameter
      * determines whether the thread executing this task (if it is
      * known by the implementation) should be interrupted in an
      * attempt to stop the task.
      *
      * <p>After this method returns, subsequent calls to {@link
#isDone} will
      * always return {@code true}.  Subsequent calls to {@link
#isCancelled}
      * will always return {@code true} if this method returned {@code
true}.
      *
      * @param mayInterruptIfRunning {@code true} if the thread
      * executing this task should be interrupted (if the thread is
      * known to the implementation); otherwise, in-progress tasks are
      * allowed to complete
      * @return {@code false} if the task could not be cancelled,
      * typically because it has already completed; {@code true}
      * otherwise. If two or more threads cause a task to be cancelled,
      * then at least one of them returns {@code true}.
      */
     boolean cancel(boolean mayInterruptIfRunning);

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

Re: CompletableFuture.cancel vs Future.cancel bug?

JSR166 Concurrency mailing list
Got it, thanks!

Regarding the javadoc, I would avoid the words "success" and "failure" or we need to clarify what exactly they mean in this context.

Maybe something like this would be more clear:

     /**
      * Attempts to cancel execution of this task. This method has no
      * effect if the task is already completed or cancelled or
      * if the task could not be cancelled for any other reason. If
      * this task has been cancelled before it has started, it should never run.

Also, the description of the return value makes me think that it is allowed to return true more than once only if multiple threads have executed cancel() at the same time,
but a single thread can not see this effect if it calls cancel() multiple times sequentially. In other words the following test must pass:

var future = new CompletableFuture<>();
assertTrue(future.cancel(true)); // Causes cancellation.
assertFalse(future.cancel(true)); // Does not cause cancellation because the future has already been cancelled by the previous call.

But obviously this does not hold for CompletableFuture. I would avoid mentioning that there must be more than one thread at play.

I would rather rewrite this return value description completely, because the part "return {@code false} if the task could not be cancelled" sounds very misleading.
To me it still means that if a future has already been cancelled then it can not be cancelled twice and thus the second cancel() call must return false.

From what I see there are two main types of cancel() implementation: one is CAS-like, another returns true if already cancelled.
The most clear and comprehensive description I could come up with:

      * @return {@code true} if the cancellation is successful.
      * The exact semantics of "successful cancellation" is implementation specific:
      * it may mean that this exact call caused the future to become {@link #isCancelled cancelled}
      * or that the future is {@link #isCancelled cancelled} now regardless of what caused it,
      * other implementations are permitted as well.
      */




On Sun, Aug 23, 2020 at 11:15 PM Doug Lea <[hidden email]> wrote:
On 8/23/20 9:30 AM, Sergi Vladykin wrote:
>
>
> Yes, my interpretation was exactly "successful attempt" == "return
> true", "failed attempt" == "return false". Would be nice to have this
> spec improved.
See draft update below, that also tries to clarify mayInterruptIfRunning.
>
> Also, could you please clarify why the decision was made not to have
> CAS-like semantics for CompletableFuture.cancel() while it was clearly
> possible? Maybe there are some hidden benefits I don't see?
>
I don't have a clear recollection (it was during JDK5), but it may have
been a reflection of issues with (concurrent) Collection remove()
methods requiring that the return value indicate that the current call
was responsible for removal, which is sometimes arbitrary and causes
extra expense and complexity. (We considered trying to weaken this, but
didn't.)

... possible javadoc update:

     /**
      * Attempts to cancel execution of this task.  This method has no
      * effect if the task is already completed or cancelled, and may
      * fail if the task could not be cancelled for any reason. If
      * successful, and this task has not started when {@code cancel}
      * is called, this task should never run.  If the task has already
      * started, then the {@code mayInterruptIfRunning} parameter
      * determines whether the thread executing this task (if it is
      * known by the implementation) should be interrupted in an
      * attempt to stop the task.
      *
      * <p>After this method returns, subsequent calls to {@link
#isDone} will
      * always return {@code true}.  Subsequent calls to {@link
#isCancelled}
      * will always return {@code true} if this method returned {@code
true}.
      *
      * @param mayInterruptIfRunning {@code true} if the thread
      * executing this task should be interrupted (if the thread is
      * known to the implementation); otherwise, in-progress tasks are
      * allowed to complete
      * @return {@code false} if the task could not be cancelled,
      * typically because it has already completed; {@code true}
      * otherwise. If two or more threads cause a task to be cancelled,
      * then at least one of them returns {@code true}.
      */
     boolean cancel(boolean mayInterruptIfRunning);


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

Re: CompletableFuture.cancel vs Future.cancel bug?

JSR166 Concurrency mailing list
Anecdotally, we do have at least one place in Guava in which we rely on our own Future implementations to return true from cancel(...) only if that particular call was responsible for the cancellation. Here's the code, which could otherwise NPE (by dereferencing state/localState, which would be nulled out by the first cancellation). Glancing at the first few pages of hits in the wider Google codebase, I can see at least one more bit of code that would throw an exception, at least one that would lose track of the number of outstanding tasks, and a few that would track metrics incorrectly.

That said:
  • As I suggested above, we are very likely to encounter only our own Future implementations. (Sometimes this is outright guaranteed, as in the example I linked.)
  • If we encounter other Future implementations, the problem already exists. Changing the doc to better explain the state of the world sounds like a good thing.
  • We might be able to work around the different return value (though perhaps sometimes at a noticeable cost in complexity or performance).
So practically speaking, we are not very likely to be bitten by the current behavior. And I agree that a change to that behavior is riskier than doing nothing. So I like the idea of just better documenting how things stand today.

If anyone out there is implementing Future or a similar API from scratch, though, I wanted to at least encourage you to implement cancel(...)'s return value according to the current docs: The extra information can be useful (and it sometimes turns out to be easy enough to provide, as in CompleteableFuture, which actually has to do extra work to return true in the "already cancelled" case).

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

Re: CompletableFuture.cancel vs Future.cancel bug?

JSR166 Concurrency mailing list

If anyone out there is implementing Future or a similar API from scratch, though, I wanted to at least encourage you to implement cancel(...)'s return value according to the current docs

Looks like Google guys see this spec the same way as I do.

Probably for Guava it would make sense to have an utility method in Futures like :

public static boolean cancel(Future<?> f) {
     if (f instanceof CompletableFuture)
           return !f.isDone() && ((CompletableFuture)f).completeExceptionally(new CancellationException());
     else
           return f.cancel(true);
}

Though, I don't see any clean workarounds for ForkJoinTask except dirty magic with reflection/unsafe or by abusing compareAndSetForkJoinTaskTag() method. 



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