Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify CancelableOperation docs #264

Open
dickermoshe opened this issue Jan 23, 2024 · 4 comments
Open

Clarify CancelableOperation docs #264

dickermoshe opened this issue Jan 23, 2024 · 4 comments

Comments

@dickermoshe
Copy link

Clarify that CancelableOperation doesn't actually cancel the operation

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

@lrhn
Copy link
Member

lrhn commented Apr 5, 2024

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
Copy link

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
Copy link
Member

lrhn commented Apr 16, 2024

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
Copy link

allComputableThings commented Apr 17, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants