CancelableOperation.isCompleted docs are incorrect

[natebosch]

The doc comment for CancelableOperation.isCompleted says it is tied to whether the operation can be canceled.

https://github.com/dart-lang/async/blob/5737ad764bfce1ac14918944e7fb538cd674359e/lib/src/cancelable_operation.dart#L134-L139

This isn't true. The operation may still be canceled if the completer was completed with a Future that has not yet completed. We want definitely that behavior, otherwise the CancelableOperation.fromFuture API makes no sense - you'd never be able to cancel such an operation at all because isCompleted is true immediately in that case. I don't think we want to change the behavior of CancelableCompleter._cancel().

https://github.com/dart-lang/async/blob/5737ad764bfce1ac14918944e7fb538cd674359e/lib/src/cancelable_operation.dart#L34-L36

We need to decide if we want CancelableOparation.isCompleted to match the first or the second detail in this comment, and update the doc comment, and potentially implementation, to match.

We should either update the doc to reflect current behavior:

  /// Whether the [CancelableCompleter] backing this operation has been
  /// completed with a value or a Future.
  ///
  /// This value being true does not imply that the [value] future has
  /// completed. If the completer backing this operation was completed
  /// with a Future it may still be possible to cancel this operation.

Or, update the doc and change behavior. This would be a breaking change. I think it makes the API more useful, if potentially more confusing.

  /// Whether the [value] future has completed.
  ///
  /// Listeners on the future may not have been invoked yet,
  /// even if this returns true.
  ///
  /// The [CancelableCompleter] backing this operation may
  /// have been completed, even if this returns false.
  /// That is, `completer.isCompleted` may be true before
  /// before `completer.operation.isCompleted` is true.
  ///
  /// When this returns true, it is no longer possible to
  /// cancel this operation.

cc @lrhn

[lrhn]

It's a highly complicated state-based API.
It appears to have at least the following states:

• Initial
• Cancelled (cancel has been successfully called prior to the result being available).
• Completed (the future of the cancellable operation has completed with a value or error, cancel is impossible).
• Pre-completed (the future has been "completed" with a future, but that future hasn't completed yet. Cancel ... is not currently possible?)

I'd try to avoid making the last state user visible. It's true that at that point, cancel has no effect on stopping the computation, but it can still change the result. So, I'd go with allowing cancel until the final value/error is ready.

The CancelableOperation should not refer to CancelableCompleter anywhere in the documentation (other than as a hint on how to create a CancelableOperation). Using a completer is just one implementation approach for creating cancelable operations, there could be others.

So the isCompleted should mean what it says, whether the CancelableOperation itself has completed. It should not refer to the backing CancelableCompleter at all, so whether that one has been completed with a future is a moot point.

So, I'd change isCompleted documentation to something like:

  /// Whether the result of this operation is ready.
  ///
  /// When ready, the [value] future is completed with the result value
  /// or error, and this operation can no longer be cancelled.

and cancel to

  /// Cancel this operation.
  ///
  /// Has no effect if called after the result has become available 
  /// (when [isCompleted] becomes true).
  /// If called before that, the result of the computation will be discarded
  /// and the computation is ended earlier if possible.
  ///
  /// The cancellation may take time, or even have exceptions, 
  /// in which case the returned future will be completed when
  /// the cancellation is complete. It's still guaranteed that the
  /// [value] future is not completed with a result after this
  /// method has been called.

(Not great, something like that).
The cancel should be able to cancel up to the moment where isCompleted changes to true.

That does mean that the completer can't just fill a Future into the future-completer and say that it's done.
It has to wait for that future to complete and then (if not cancelled in the meantime) stuff the result into the future-completer.

[natebosch]

The CancelableOperation should not refer to CancelableCompleter anywhere in the documentation (other than as a hint on how to create a CancelableOperation). Using a completer is just one implementation approach for creating cancelable operations, there could be others.

There aren't others today unless we want to leave out these details just in case there is some class that implements CancelableOperation. I do agree that we can probably target these docs more towards the consumers of the operation than the producers, and keep producer focused docs on CancelableCompleter.

So the isCompleted should mean what it says, whether the CancelableOperation itself has completed. It should not refer to the backing CancelableCompleter at all, so whether that one has been completed with a future is a moot point.

I agree that would be the most useful. Getting there will be breaking and it's not clear to me yet how difficult it will be.