dart-lang / core

This repository is home to core Dart packages.
https://pub.dev/publishers/dart.dev
BSD 3-Clause "New" or "Revised" License
19 stars 7 forks source link

Clarify `CancelableOperation` docs #369

Open dickermoshe opened 10 months ago

dickermoshe commented 10 months ago

Clarify that CancelableOperation doesn't actually cancel the operation

https://github.com/dart-lang/sdk/issues/42855 https://pub.dev/documentation/async/latest/async/CancelableOperation-class.html

lrhn commented 8 months ago

Cancelling a CancelableOperation does cancel the operation. That's the point of it. If someone creates a cancelable operation that doesn't cancel, then they're not following protocol. The user should assume that cancelling works.

allComputableThings commented 7 months ago

What is the protocol? This use fails...

import 'package:async/async.dart';
import 'package:flutter_test/flutter_test.dart';

void main() async {
  test('async', () async {
    int ticks = 0;
    Future<void> forever() async {
      try {
        while (true) {
          print(".");
          await Future.delayed(const Duration(milliseconds: 300));
          ticks += 1;
        }
      } catch (e, s) {
        print("Forever caught ${e}");  // Never called
      } finally {
        print("Forever done");  // Never called
      }
    }

    var fut = CancelableOperation.fromFuture(forever());
    await Future.delayed(const Duration(seconds: 1));

    await fut.cancel();

    // Expecting no more ticks now
    final lastTick = ticks;

    print("CANCEL ISSUED");

    await Future.delayed(const Duration(seconds: 1));
    assert(lastTick == ticks);

    print("DONE2");
  });
}
lrhn commented 7 months ago

The issue here is:

 CancelableOperation.fromFuture(forever())

where a cancelable operations is created without an onCancel callback. That means it's a "cancelable operation" that doesn't do anything when it's cancelled. That's a choice. Presumably the author wanted to adapt a non-cancelable operation into something that can be passed in where a CancelableOperation is expected, and they are OK with not actually cancelling anything. That might be fine.

You can't cancel a future. Futures do not represent operations, they represent (eventual) results of operations. That's simply what they are designed for, and it's why they can be safely shared.

You can cancel a computation if it allows so, by accepting input that can trigger the cancel, and acting on it. The CancelableOperation class is a "standard" API for exposing a cancelable operation.

You can reasonably wrap a non-cancelable operation in CancelableOperation and not actually cancel when requested. If the code receiving that object actually expects something specific to happen when it calls cancel, then your'd not be following the protocol for operations passed to that code. If the code doens't care when it calls cancel, and considers it just a hint that it's no longer interested in the result, then not doing anything is fine.

So, the protocol is that whatever you pass a CancelableOperation to should get what it expects and requires.

Nothing can stop forever here, because it's not built to be stopped.

allComputableThings commented 7 months ago

You can't cancel a future.

If true, that's something specific to Dart (other language have async routines raise an exception when cancelled). A sad condition if Dart doesn't allow this.

I'd say the intended use is not clear for the docs.

then your'd not be following the protocol for operations passed to that code.

What protocol? Its not in the docs (also the OP's complaint).

I think you're saying CancelableOperation is pointless without an onCancel (and the future won't be stopped without some additional custom coordination between the future and the onCancel callback). Makes sense to me, but not from the docs.

Could also make onCancel required if this is the case.