-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[BUG] IsExecuting becomes False when cancellation is requested instead of task actually finishes #2153
Comments
Hey @dtoppani-twist 👋, Thank you for opening an issue. We will get back to you as soon as we can. Also, check out our Open Collective and consider contributing financially. https://opencollective.com/reactiveui
|
Can you please add details that were removed from the issue template such as platform, which UI platform you are developing for etc. Thanks |
Looking at ReactiveCommand source code I am trying to reduce the problem - I hope it can helps isolate the bug faster. This creates the same observable from ReactiveCommand Execute but using Simple Debug outputs
And this is the code to try it:
The cancellation is done by running:
The debug output is:
AFAICT IsExecuting is calculated based on the number of "begin" and "end" markers. The "end" marker is generated on cancellation instead of when the task is actually completed. Observable.Finally is supposed to happen when the observable completes or throws an exception, so theoretically the "end" marker should happen after Concat(execute) completes. So far the code looks right but it still does not behave as expected... |
I think
In a way, this makes sense because if the observable is disposed, Finally has to run at this point since there won't be another chance to do so. |
I can go around the limitation with a special version of ReactiveCommand implementation for tasks - the task is wrapped with a try/finally and the "end" marker is moved there instead of being on the observable
If the task is transformed to an observable, there is no obvious way to fix IsExecuting behavior |
We are trying to come up with a resolve for this but the root cause is in the |
A basic tenet of Rx is that once you have called This means that there is a problem with trying to use
The fundamental problem here is that 1 works by calling Furthermore, the problem being reported in dotnet/reactive#1256 is really about something slightly different. The problem you are describing here is that you aren't getting enough information about the post-cancellation outcome of the task, whereas Rx issue 1256 is actually the opposite: it's a complaint that Rx produces too much information, and the request is for Rx to ignore failures that occur after cancellation. More specifically, failures of that kind are currently reported globally as unobserved exceptions, but if you look in the main text of the issue itself you'll find this:
In other words, the person who submitted that issue is saying that we need to be more comprehensive in our application of the fundamental tenet that when you unsubscribe from a source, you are making a statement that you no longer want to hear anything from that source. @bartdesmet proposed a solution to that Rx issue which entailed:
That is entirely in the spirit of what the original poster asked for. And it doesn't help you in the slightest. I don't believe the problems you've described here can be fixed with any adjustments to the behaviour of the existing I think what you actually need is a completely different mechanism. I think what you need is something with signatures more like this: public static IObservable<(IObservable<Unit> Result, Action Cancel)> FromAsyncWithCancel(
Func<CancellationToken, Task> actionAsync);
public static IObservable<(IObservable<TResult> Result, Action Cancel)> FromAsyncWithCancel<TResult>(
Func<CancellationToken, Task<TResult>> actionAsync); My thinking here is that the root cause of the problems you're describing here are that you are attempting to use a single Each time you subscribe to an observable source representing a task, you need an The signatures above might look overcomplicated. Why do we need nested observables? I did initially think of this: // Won't work for multiple invocations
public static (IObservable<Unit> Result, Action Cancel) FromAsyncWithCancel(
Func<CancellationToken, Task> actionAsync); That's simpler, but the problem is that you get only a single cancellation callback. That's no good, because a feature of the observables you get back from So that won't work. That's why I'm proposing the more complex design. It resembles the existing design in which subscribing to the observable source representing the task will never produce more than one item. The change is that instead of getting a notification after the task is done, this observable now produces a single notification to tell you that the work has begun, and at the same time it hands you a separate (Aside: it did occur to me that we could actually just return an The crucial difference is that you would now have a way to cancel the underlying task without also unsubscribing from notifications that would have kept you information about the task's progress. This shows an outline of how you could use this to provide the behaviour you want: (UPDATE 2023/06/02: I've now created a complete illustration at #3556 - it's not quite the same as the code shown here, because I there were a few details I hadn't considered when I sketched this out back in April.) public ReactiveCommand
{
public static ReactiveCommand<Unit, Unit> CreateFromTask(
Func<CancellationToken, Task> execute,
IObservable<bool>? canExecute = null,
IScheduler? outputScheduler = null)
{
if (execute is null)
{
throw new ArgumentNullException(nameof(execute));
}
return CreateFromObservable(() => ObservableEx.FromAsyncWithCancel(execute), canExecute, outputScheduler);
}
public static ReactiveCommand<Unit, TResult> CreateFromObservable<TResult>(
Func<IObservable<(IObservable<TResult> Result, Action Cancel)>> execute,
IObservable<bool>? canExecute = null,
IScheduler? outputScheduler = null)
{
if (execute is null)
{
throw new ArgumentNullException(nameof(execute));
}
return new ReactiveCommand<Unit, TResult>(
_ => execute(),
canExecute ?? Observable.Return(true),
outputScheduler);
}}
}
public ReactiveCommand<TParam, TResult>
{
private readonly Subject<Exception> _exceptions = new();
private readonly ISubject<ExecutionInfo, ExecutionInfo> _synchronizedExecutionInfo;
private readonly Func<TParam, IObservable<(IObservable<TResult> Result, Action Cancel)>> _execute;
private readonly IObservable<bool> _isExecuting;
private readonly IObservable<bool> _canExecute;
public ReactiveCommand(
Func<TParam, IObservable<(IObservable<TResult> Result, Action Cancel)>> execute,
IObservable<bool> canExecute,
IScheduler? scheduler)
{
ArgumentNullException.ThrowIfNull(canExecute);
_execute = execute;
Subject<ExecutionInfo> executionInfoSubject = new();
_synchronizedExecutionInfo = Subject.Synchronize(executionInfoSubject, scheduler ?? Scheduler.Default);
_isExecuting = _synchronizedExecutionInfo
.Scan(
0,
(acc, next) =>
{
return next.Demarcation switch
{
ExecutionDemarcation.Begin => acc + 1,
ExecutionDemarcation.End => acc - 1,
_ => acc
};
})
.Select(inFlightCount => inFlightCount > 0)
.StartWith(false)
.DistinctUntilChanged()
.Replay(1)
.RefCount();
_canExecute = canExecute
.Catch<bool, Exception>(
ex =>
{
_exceptions.OnNext(ex);
return Observable.Return(false);
})
.StartWith(false)
.CombineLatest(_isExecuting, (canEx, isEx) => canEx && !isEx)
.DistinctUntilChanged()
.Replay(1)
.RefCount();
}
public IObservable<bool> IsExecuting => _isExecuting;
public IObservable<TResult> Execute()
{
IObservable<(IObservable<TResult> Result, Action Cancel)> sourceAndCancellation = _execute(default!);
return Observable.Defer(
() =>
{
// We want to subscribe to the sourceAndCancellation, and we essentially need to
// make the nested IObservable<TResult> that it returns our effective result.
// However, we need to ensure that we don't unsubscribe from the underlying source until it's done,
// so we can't just return it directly. Moreover, the IObservable we return must not
// be the only thing subscribed to that source.
// We essentially want the part that sends notifications to the
// _synchronizedExecutionInfo to survive until the source says its done, even
// if the code that subscribed to the IObservable<TResult> that we returned
// decides to cancel the operation by unsubscribing early. This means that the
// observable we return must be distinct from the subscription that makes
// IsExecutable and Exceptions work so that the caller can be Dispose the
// subscription we return without us losing the notifications that keep us
// informed about the progress of the underlying task.
IConnectableObservable<(IObservable<TResult> Result, Action Cancel)> sharedSource =
sourceAndCancellation.PublishLast();
// This enables separation between our internal subscription that we
// use to monitor the progress of the task, and the caller's subscription.
Subject<TResult> executeResult = new();
_synchronizedExecutionInfo.OnNext(ExecutionInfo.CreateBegin());
sharedSource
.SelectMany(sourceAndCancellation => sourceAndCancellation.Result)
.Do(result =>
{
_synchronizedExecutionInfo.OnNext(ExecutionInfo.CreateResult(result));
executeResult.OnNext(result);
})
.Catch<TResult, Exception>(
ex =>
{
_exceptions.OnNext(ex);
return Observable.Throw<TResult>(ex);
})
.Finally(() => _synchronizedExecutionInfo.OnNext(ExecutionInfo.CreateEnd()))
.Subscribe(); // Will remain subscribed until the underlying task says it's done.
// We can't just return the executeResult directly. We need to ensure that if the
// caller unsubscribes from the observable we return, we trigger cancellation.
return sharedSource
.RefCount()
.SelectMany(
sourceAndCancellation =>
{
// It's important that we don't directly subscribe to the source passed in
// here, because that would trigger a second execution of the callback that
// runs the task.
(_, Action cancel) = sourceAndCancellation;
return executeResult
.Finally(() => cancel());
});
});
}
} That's a bit scrappy, and will need some work to get it to production quality, but it does demonstrate the concept. I used that from this program: Stopwatch sw = Stopwatch.StartNew();
var cmd = RxCommand.CreateFromTask(async (token) =>
{
Console.WriteLine($"{sw.Elapsed}: started command");
try
{
await Task.Delay(10000, token);
}
catch (OperationCanceledException)
{
Console.WriteLine($"{sw.Elapsed}: starting cancelling command");
await Task.Delay(5000);
Console.WriteLine($"{sw.Elapsed}: finished cancelling command");
throw;
}
Console.WriteLine($"{sw.Elapsed}: finished command");
});
cmd.IsExecuting.Subscribe(isExecuting =>
{
Console.WriteLine($"command executing = {isExecuting}");
});
Console.WriteLine($"{sw.Elapsed}: Starting command");
IDisposable subscription = cmd.Execute().Subscribe(
_ => { Console.WriteLine($"{sw.Elapsed} Execute.OnNext"); },
ex => { Console.WriteLine($"{sw.Elapsed} Execute.OnError {ex}"); },
() => { Console.WriteLine($"{sw.Elapsed} Execute.OnCompleted"); });
await Task.Delay(1000);
Console.WriteLine($"{sw.Elapsed}: Unsubscribing from IObservable returned by Execute");
subscription.Dispose();
Console.WriteLine($"{sw.Elapsed}: Unsubscribed");
Console.ReadLine(); and got the following output:
The critical point here is that the If you agree with me that merely fixing Rx issue 1256 won't help you at all, and that you need a mechanism of this kind, in which cancellation of the underlying task is separated from the observation of the task's progress, then I would ask you to create a new issue in Rx asking for this new feature. Note that you don't technically need the feature to be built into Rx. The spike I created to do the research behind this message just referenced the Rx libraries and implemented its own |
@idg10 Hi Ian, I know that you have spent much time over the past few days looking into this, I am very grateful for your assistance in getting this issue resolved, as you can see its been open for a while. Personally I agree that a different function is required due to the underlying functionality of StartAsync not fitting the requirements of ReactiveCommand.CreateFromTask with Cancellation Token. This event trail is the trail that I understand we require to ensure that every execution of the command returns a value. Normal Execution run till Task completion
Cancel Execution to Task Cancellation is complete
The user may have the command locked for a longer period of time than normal in the event of Task execution cancellation however presently the operation does not return ever due to the cancellation exception being swallowed internally in StartAsync, therefore leaving the command locked out indefinitely. I am currently in Saudi Arabia, but should return to the UK on Friday evening |
I'm going to copy out your final sequence of events but with numbers, because I want to ask a question:
Which observable is handling the I think it is possible for users of If you want the Of course nothing stops you from writing an observable source that disobeys the rules. Nothing stops you from writing an observable source that calls This text has been in the Rx Design Guidelines at least as far back as 2010 (my emphasis):
This is in section 4.4. And although the document technically only defines "guidelines", I'd point out that the grammar rules for calling I don't intend to modify (BTW, I'll be off work all of next week, so I'll be unavailable just as you get back...) |
Yes its the same Execute observable - In section 5.9 of the Rx Design Guidelines it says that messages can continue to be pushed until either the Dispose (UnSubscribe) method has returned or OnError has been raised or OnCompleted has been raised, after this point no further messages are expected. In my mind this still gives opportunity for us to push the OnError with a OperationCanceledException and a inner exception containing an AggregateException of any errors from the Task being Executed before the Unsubscribe has completed without breaking these rules. in very simple terms this is what I am proposing try
{
observer.OnNext(await Task.Delay(10000, token));
observer.OnCompleted();
}
catch (OperationCanceledException ex)
{
// Disposing
TaskCleanUpAction();
observer.OnError(ex);
} This is basically what happens with try
{
observer.OnNext(await Task.Delay(10000, token));
observer.OnCompleted();
}
catch (Exception)
{
// Disposing
// Nothing happens, exceptions are swallowed
} I understand that in some cases task cancellation may take some time, but giving an optional TimeOut input parameter to the function would allow the end user to control how long they wish to wait before giving up the wait for the Task to clean up, however OnError should still be fired at this point with something like a FailedToCleanUpException as a inner exception as part of the OperationCanceledException. Modifying |
The exact wording from section 5.9 is:
The critical point here is that it says "can" and not "will". And reading through this all again after having been away, I've realised that it might not be completely clear that there are two important dimensions to consider:
In cases where we remain subscribed until the source completes it's straightforward. Early unsubscription is where it gets complex. Taking that document as a whole, the implication of sections 4 and 5 is that there are three phases for a subscription:
And here's what you can presume about the two aspects above (assuming we unsubscribed before the source completed naturally):
The implication of both section 4 and 5 is that there are no guarantees in 2. For as long as phase 2 persists, it's possible that you might get notifications (because unsubscription takes some time to complete; 5.9 makes this explicit) but it's also possible that you won't even though the underlying process represented by the observable source might still be in progress (section 4.4 makes this clear). You also can't detect anything about whether the underlying process is complete either in phase 2 or phase 3 (as section 4.4 makes clear). The only way you know anything about the state of the underlying process is if you remain subscribed until the source completes or fails. Any design that makes assumptions about what will emerge during phase 2 is broken. That's true whether you presume that notifications will continue to emerge, or you presume that they won't. Both of those presumptions will be wrong at least some of the time. Any design that relies on either presumption will fail at least some of the time. Furthermore, any design that makes assumptions about the state of the underlying process represented by an IObservable (e.g., a That's true because there's no guarantee that the underlying process represented by the observable source has actually been stopped just because you've reached phase 3. (Section 4.4 makes it explicitly clear that work in progress might continue to run even after As it happens, the Rx.NET implementation has always erred heavily on the side of shutting down notifications early: for over a decade it has done that before making any attempt to stop the underlying activity. If you need to be able to know how a task finished, then you simply cannot use the subscription that achieves this as the mechanism by which you also cancel a task. These are mutually exclusive usage patterns. That final row of that table above make it clear why you can never achieve both cancellation and notification of completion with a single subscription: once you're in phase 3 you know for certain that you're never receiving any more notifications, but as far as you know, the underlying work might still be in progress. Since completion might not occur until after you've entered phase 3, you're not going to be notified of it. Now I think you might be still hoping for an implementation in which we a) prevent |
This is a response to reactiveui#2153 For a while now, the ReactiveUI team has asserted that the root cause for this issue (and some other related ones) is that if you ask Rx to wrap a Task<T> as an IObservable<T>, you don't get completion or error notifications if you unsubscribe before the task completes. This behaviour is by design, but it is problematic in cases where you want to cancel the task, because task cancellation is triggered by unsubscription. Our view is that if you're using Rx's task wrapping, then you can either get notifications through to completion, or you can unsubscribe early to cancel the task, but you can't do both. (Rx always shuts down notifications as the first act of unsubscription. This is permitted within the rules of IObservable, and is deeply baked into the framework. Although it is not technically illegal to implement an IObservable that continues to produce notifications up until the call to Dispose returns, you shouldn't rely on that, because such behaviour intentionally leaves implementations some wiggle room. Basically, once you've called Dispose, all best are off.) But since we want to engage constructively with Rx users, we don't just want to say "by design" and leave it at that. So this PR is an illustrative spike showing one way to solve this that doesn't rely on post-Dispose notifications.
To demonstrate the viability of what I'm proposing, I've created a spike showing the basic idea at #3556 I'm not proposing we merge that in its current state. It's mainly to:
The code has a lot of comments in to explain my thinking, because the point of this PR is to explain the idea, rather than to submit actual code. If the ReactiveUI team decides that they are happy with this direction, there are a few things we could do as next steps. Firstly, note that this includes an Then there's the code in the spike itself. We could try to hammer it into a shape where you'd be happy to accept the PR. Or you could just use it just as an illustration for developing your own solution. Then again, there might be something terribly wrong with this approach that I haven't spotted, in which case please let me know and I'll have another think. |
…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
This should now be resolved, please raise a new issue with any new findings, many thanks |
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. |
Platform: Windows/WPF
ReactiveUI 9.19.0.0
ReactiveUI.WPF 9.19.0.0
It appears the behavior of IsExecuting is not completely accurate. Assuming the command:
This simulates a task that needs to do some cleanup work after a cancellation was requested. While this cleanup work is happening, IsExecuting should still be true, because the task is still running and the command should not allow execution until cleanup is completed
And the code to test the command:
executing the command:
disposable = SomeCommand.Execute().Subscribe();
cancelling the command while the command runs:
disposable.Dispose();
This is the debug output:
started command
command executing = True
Exception thrown: 'System.Threading.Tasks.TaskCanceledException' in mscorlib.dll
starting cancelling command
command executing = False
finished cancelling command
Exception thrown: 'System.Threading.Tasks.TaskCanceledException' in mscorlib.dll
IsExecuting becomes False as soon as the command cancellation is requested, instead it should stay True for an extra 5000ms. The debug output should be:
started command
command executing = True
Exception thrown: 'System.Threading.Tasks.TaskCanceledException' in mscorlib.dll
starting cancelling command
finished cancelling command
Exception thrown: 'System.Threading.Tasks.TaskCanceledException' in mscorlib.dll
command executing = False
I am trying to trace the code inside ReactiveCommand but still don't understand why this would happen.
The text was updated successfully, but these errors were encountered: