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

[Bug]: Cancelling a task from an object in a command cause the application to crash #3450

Closed
SteezCram opened this issue Dec 27, 2022 · 3 comments

Comments

@SteezCram
Copy link

SteezCram commented Dec 27, 2022

Describe the bug 🐞

I was developing an object to synchronize some files over the network. I want the user to be able to stop the synchronization. I ended up utilizing Task to achieve that and a CancellationTokenSource to cancel the task.

I use this documentation to achieve it: https://learn.microsoft.com/en-us/dotnet/standard/parallel-programming/task-cancellation

So far, everything works as it was intended. But when I try to stop the synchronization by using a ReactiveCommand, my application exit (see https://www.reactiveui.net/docs/handbook/default-exception-handler/). Despite catching the exception, the default exception handler triggered an exception because my task was canceled...

Step to reproduce

Simple object to reproduce:

class MyObject
{
    private CancellationTokenSource _cancellationTokenSource = new();

    public MyObject() { }

    public async Task<bool> StartAsync()
    {
        for (int i = 0; i < 100; i++)
        {            
            try
            {
                await BackgroundMethodAsync();
            }
            catch (OperationCanceledException ex)
            {
                return false;
            }
        }
        
        return true;
    }

    public async Task StopAsync()
    {
        _cancellationTokenSource.Cancel();
        // Wait task to finish
        await Task.Delay(10000);
        _cancellationTokenSource.Dispose();
    }

    private async Task BackgroundMethodAsync()
    {
        for (int i = 0; i < 100; i++)
        {
            _cancellationTokenSource.Token.ThrowIfCancellationRequested();
            await Task.Delay(10000);
        }
    }
}

Just create 2 commands that use both StartAsync and StopAsync. If you called StopAsync and you abord the task, it should close the application.

Reproduction repository

No response

Expected behavior

When catching the exception it should not exit the application.

Screenshots 🖼️

No response

IDE

Visual Studio 2022

Operating system

Windows

Version

Windows 11

Device

No response

ReactiveUI Version

13.2.10

Additional information ℹ️

The version of Avalonia 0.10.18 has been tested

@SteezCram SteezCram added the bug label Dec 27, 2022
@SteezCram
Copy link
Author

SteezCram commented Dec 27, 2022

EDIT: if the stop method is not a task, we do not have the bug

There is a new object that does not produce the bug:

class MyObject
{
    private CancellationTokenSource _cancellationTokenSource = new();

    public MyObject() { }

    public async Task<bool> StartAsync()
    {
        for (int i = 0; i < 100; i++)
        {            
            try
            {
                await BackgroundMethodAsync();
            }
            catch (OperationCanceledException ex)
            {
                _cancellationTokenSource.Dispose();
                return false;
            }
        }
        
        return true;
    }

    public void Stop()
    {
        _cancellationTokenSource.Cancel();
    }

    private async Task BackgroundMethodAsync()
    {
        for (int i = 0; i < 100; i++)
        {
            _cancellationTokenSource.Token.ThrowIfCancellationRequested();
            await Task.Delay(10000);
        }
    }
}

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

2 participants