Re: CompletableFuture.cancel vs Future.cancel bug?

On Thu, Aug 20, 2020 at 8:48 PM Martin Buchholz <[email protected]> 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 <[email protected]> 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 <
> [email protected]> 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
> > [email protected]
> > http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>

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