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

Proposal: remove (or at least deprecate) all ReactiveCommand.CreateFromTask overloads that work with CancellationToken #1245

Closed
kentcb opened this issue Jan 26, 2017 · 12 comments

Comments

@kentcb
Copy link
Contributor

kentcb commented Jan 26, 2017

ReactiveCommand defines a number of CreateFromTask overloads that have funcs accepting a CancellationToken. I posit that these overloads are entirely useless. A full discussion on why this is so is included below.

Part of the impetus for this proposal is gathering feedback from community members who do use these overloads and, if so, how they are using them successfully.

Ideally, I would like to just remove these overloads. Yes, it's a breaking change, but given my statement above, no one should be using these overloads anyway, so we'd in theory be breaking nobody. At the very least, I want to deprecate these APIs and remove them in a later major revision (probably v8). Doing so would lead to a simpler, less confusing API. It would no longer mislead people into thinking these overloads are useful for cancellable TPL-based commands because they're not.

Below is an excerpt from some literature I'm writing. The context is a discussion on cancelling reactive commands.

CreateFromTask with CancellationToken is useless

If, on the other hand, you choose (or are forced) to used TPL for modelling command execution, things get a little trickier. In such cases, you should first be sure to accept a CancellationToken in your command’s execution logic:

private async Task<bool?> LoginAsync(CancellationToken ct)
{
    var result = this.Password == "Mr. Goodbytes";
    await Task.Delay(TimeSpan.FromSeconds(1.5),  ct);
    return result;
}

Secondly, be sure to use it! Notice that we’re passing the CancellationToken into Task.Delay. This will ensure that we bail as early as possible once the token indicates it has been cancelled (we’ll see how that happens shortly). If the API you’re consuming does not support cancellation, you will have to find some other recourse. Perhaps by submitting a pull request or switching to an alternative library. Failing that, you can always simulate cancellation by kicking off the task and periodically checking whether cancellation has been requested, bailing and ignoring the result if it has.

Finally – and this is where it gets less intuitive – it’s easiest to forgo CreateFromTask and instead use CreateFromObservable:

this.loginCommand = ReactiveCommand.CreateFromObservable(
    ()  =>
    Observable
        .StartAsync(this.LoginAsync)
        .TakeUntil(this.cancelCommand));

To convert our Task to an observable, we simply call Observable.StartAsync. There are overloads of StartAsync that work with both Task and Task<T>, with and without CancellationToken support. Since our LoginAsync method accepts a CancellationToken, we will receive a CancellationToken that is cancelled when the subscription to the observable is disposed. This makes it much easier to support cancellation of the command because we simply use the same TakeUntil trick as discussed above.

You may wonder why it’s better to use CreateFromObservable. You may look at the overloads for CreateFromTask and see that there are several that do use the CancellationToken type. If you try it out, you’ll see that you can do something like this:

this.loginCommand = ReactiveCommand.CreateFromTask(
    ct  => this.LoginAsync(ct),
    canLogin);

For clarity, I’ve made the ct parameter explicit here, but you could of course write this as:

this.loginCommand = ReactiveCommand.CreateFromTask(
    this.LoginAsync,
    canLogin);

Either way, this will cause ReactiveUI to perform the Observable.StartAsync call on our behalf, and our LoginAsync method still receives an appropriate CancellationToken. The problem is, of course, how do we actually cancel the command execution? We no longer have the ability to cancel based on a signal from our cancelCommand because we never get a say in the pipeline representing our command’s execution. We are only left with messy, far more onerous options.

The first thing we might try is maintaining our own CancellationTokenSource, creating a new one with each execution of loginCommand. We would then need to cancel that token manually when our cancelCommand executes. This results in extra state we need to manage and coordinate, and that state compounds for each cancellable command we create.

We might also consider the fact that ReactiveUI is calling Observable.StartAsync on our behalf. This means that the subscription to the command’s execution could be disposed to cancel the execution. However, we don’t normally create or manage that subscription ourselves. Usually, we’d use bindings so that all the peripheral functionality the accompanies commands – setting the target control’s enabled state, for example – just works. If we forgo binding and attempt to manage the subscription ourselves, we’ll quickly discover that this too leads to messy, convoluted code.

You could argue, of course, that this is all a design deficiency in ReactiveUI and that it should more readily support cancellation for TPL-based asynchrony scenarios. But if ReactiveUI attempted to do that, it would wind up in much the same dilemma as we did. Scattered and difficult to understand state would arise, and the API would become more complicated.

@escamoteur
Copy link

So how do you cancel running commands?
I would love having them removed as they led to quite some frustration yesterday.

But if my Page Disposes it commands bindings, would this not automatically end execution?

@kentcb
Copy link
Contributor Author

kentcb commented Jan 26, 2017

@escamoteur: the way to cancel TPL-based commands is discussed in the excerpt above.

@escamoteur
Copy link

Perhaps i just didn't understand it fully :-) Does this apply also for async Methods?

@escamoteur
Copy link

escamoteur commented Jan 26, 2017

However, we don’t normally create or manage that subscription ourselves. Usually, we’d use bindings so that all the peripheral functionality the accompanies commands – setting the target control’s enabled state, for example – just works.

I don't fully agree with this, If you follow the WhenActivated Pattern with CompundDisposable you take care of the Subscriptions or not?
But actuall this just supports your request to remove the overloads

@Hinidu
Copy link
Contributor

Hinidu commented Jan 26, 2017

We don't use these overloads but maybe it would be better to extend them with a cancellation argument (maybe of type IObservable<Unit>) than entirely remove them? It looks quite boring and hard to remember to always write code like this:

this.loginCommand = ReactiveCommand.CreateFromObservable(
    ()  =>
    Observable
        .StartAsync(this.LoginAsync)
        .TakeUntil(this.cancelCommand));

It looks like this implementation can be hidden in a library function looking something like that:

return ReactiveCommand.CreateFromObservable(
    ()  =>
    Observable
        .StartAsync(execute)
        .TakeUntil(cancel));

@kentcb
Copy link
Contributor Author

kentcb commented Jan 26, 2017

@Hinidu yep, certainly an option worth considering.

@VistianOpenSource
Copy link

At one level, i'm not bothered whether these CreateFromTasks with CancellationTokens get binned off or not. I've used the framework for quite a few years, I'm very comfortable with observables rather than tasks.

However, and perhaps a little off topic, I am concerned however that another barrier is potentially being put up for those coming from the Task/async world. I'm sure if you did a poll I'm sure the vast majority of C# developers would say they are familiar with tasks & async, Reactive Extensions i'd hazard a guess would come a somewhat distant second.

Now you don't have to be a Reactive Extensions guru to get started with ReactiveUI, but to really gain benefit from the framework one really does need to get well embedded in the Reactive Extensions world.

As I said, its not directly related to your initial question, but I do think that when the roadmap is considered for ReactiveUI (and how to onboard people to the framework) then dealing with the potential lack of reactive extensions experience is imho one that needs serious consideration.

@stale
Copy link

stale bot commented Aug 25, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this because it helps reduce the workload of our extremely busy maintainers. If you want this issue progressed faster please start talks with a maintainer with how you can help out.
There are so many ways to contribute to open-source and the most valued are often not code related. We are always looking for more help and look after those who stick around. Say howdy to one of the maintainers or browse https://reactiveui.net/contribute/ for ideas on where to start your journey.

@WorldMaker
Copy link

Isn't this just asking to add an optional IObservable<Unit> cancelOn = null parameter to CreateFromTask with CancellationToken to optionally add that TakeWhile operator? CancellationToken overloads are still useful in cases of command observable disposal even without a "cancelOn" trigger to force cancellation of specific tasks.

@ChrisPulman
Copy link
Member

We are trying to come up with a resolve for this but the root cause is in the System.Reactive code
dotnet/reactive#1256
This issue has been outstanding for quite some time.

ChrisPulman added a commit that referenced this issue Jan 1, 2024
…Task (#3704)

<!-- Please be sure to read the
[Contribute](https://github.com/reactiveui/reactiveui#contribute)
section of the README -->

**What kind of change does this PR introduce?**
<!-- Bug fix, feature, docs update, ... -->

Fix for #1245
Fix for #2153
Fix for #3450

**What is the current behavior?**
<!-- You can also link to an open issue here. -->

ReactiveCommand does not properly support Cancellation tokens properly
for CreateFromTask due to an underlying issue in System.Reactive

**What is the new behavior?**
<!-- If this is a feature change -->

Fix the issues with the base functions within ReactiveCommand due to an
issue with Observable.FromAsync from System.Reactive by using a new
ObservableMixins.FromAsyncWithAllNotifications as the new function, this
extends Observable.FromAsync handling the error bubbling as required.

ObservableMixins.FromAsyncWithAllNotifications can be used to transform
a Cancellation Task into an Observable producing the expected
cancellation, errors and results.

**What might this PR break?**

ReactiveCommand.CreateFromTask will now handle exceptions as expected,
any existing workarounds could be removed once tested with actual
implementation in end users code.

**Please check if the PR fulfills these requirements**
- [x] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)

**Other information**:

Co-authored-by: @idg10 - created the base code in #3556
@ChrisPulman
Copy link
Member

This should now be resolved, please raise a new issue with any new findings, many thanks

Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants