Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.57%. Comparing base (28bc599) to head (9385125).
Report is 421 commits behind head on dev.

@@            Coverage Diff             @@
##              dev    #9604      +/-   ##
==========================================
+ Coverage   89.82%   90.57%   +0.74%     
==========================================
  Files         412      406       -6     
  Lines       11878    12754     +876     
  Branches     2364     2473     +109     
==========================================
+ Hits        10670    11552     +882     
+ Misses        681      641      -40     
- Partials      527      561      +34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Wouldn't it be better to add a cancellation before the dispose in MudTable itself?

Or perhaps allow people to pass their own CancellationTokenSource via a [Parameter]. In that case, MudTable would use the one provided by the user instead of creating its own internal one. This way, the user would have full control over it, and there wouldn't be a need to use @ref?

@henon, thoughts?

Wouldn't it be better to add a cancellation before the dispose in MudTable itself?

MudBlazor/src/MudBlazor/Components/Table/MudTable.razor.cs

Lines 781 to 784 in 59248fe

	protected virtual void Dispose(bool disposing)
	{
	_cancellationTokenSrc?.Dispose();
	}

Or perhaps allow people to pass their own CancellationTokenSource via a [Parameter]. In that case, MudTable would use the one provided by the user instead of creating its own internal one. This way, the user would have full control over it, and there wouldn't be a need to use @ref?

@henon, thoughts?

I think the second option would be the best from my point of view. I think it's a good idea.

In my case, I create another CancellationTokenSource on the page for other database calls in other methods. If I could use a global CancellationTokenSource, I think it would be better. In the Dispose() method, this would be a global call to TokenSource.Cancel(); for all methods using the same CancellationTokenSource everywhere.

For the second proposal: on second thought, looking at the code, the CancellationTokenSource is renewed each time Server Loading is called. After a second call, the CancellationTokenSource reference provided by the Parameter will have disappeared.

For the second proposal: on second thought, looking at the code, the CancellationTokenSource is renewed each time Server Loading is called. After a second call, the CancellationTokenSource reference provided by the Parameter will have disappeared.

🤔Hmm right, what it was was two way bindable to refresh the reference

Something like that ?

/// <summary>
/// Gives the possibility to define an external CancellationTokenSource reference. 
/// </summary>
[Parameter]
[Category(CategoryTypes.Table.Behavior)]
public CancellationTokenSource CancellationTokenSrc { get; set; } = new();

/// <summary>
/// Occurs when <see cref="CancellationTokenSrc"/> has changed.
/// </summary>
[Parameter]
[Category(CategoryTypes.Table.Behavior)]
public EventCallback<CancellationTokenSource> CancellationTokenSrcChanged { get; set; }

private async Task CancelTokenAsync()
{
    try
    {
        CancellationTokenSrc.Cancel();
    }
    catch { /*ignored*/ }
    finally
    {
        CancellationTokenSrc = new CancellationTokenSource();
        await CancellationTokenSrcChanged.InvokeAsync(CancellationTokenSrc);
    }
}

PS : Adding that CancellationTokenSource is initialized by default. It is not the case today.

The new usage example should become :

@implements IDisposable

<MudTable @bind-CancellationTokenSrc="TokenSource" ...>

...

@code {
    private CancellationTokenSource TokenSource = new();

    private async Task<TableData<Element>> ServerReload(TableState state, CancellationToken token)
    {
        try
        {
             ...
             data = await DataService.GetDataAsync(DateTime.Now, token);
             ...
        }
        catch (TaskCanceledException)
        {
           // ignored
        }
    }

    public void Dispose()
    {
        TokenSource.Cancel();
    }
}

Something like that ?

Yeah, the overall idea would be that, but I'd prefer if we used our ParameterState framework to avoid writing to parameter directly:

private readonly ParameterState<CancellationTokenSource?> _cancellationTokenSrcState;

public MudTable()
{
	using var registerScope = CreateRegisterScope();
	_cancellationTokenSrcState = registerScope.RegisterParameter<CancellationTokenSource?>(nameof(CancellationTokenSource))
		.WithParameter(() => CancellationTokenSource)
		.WithEventCallback(() => CancellationTokenSourceChanged);
}

[Parameter]
public CancellationTokenSource? CancellationTokenSource { get; set; }

[Parameter]
public EventCallback<CancellationTokenSource?> CancellationTokenSourceChanged { get; set; }

private async Task CancelTokenAsync()
{
	try
	{
		_cancellationTokenSrcState.Value?.Cancel();
	}
	catch { /*ignored*/ }
	finally
	{
		await _cancellationTokenSrcState.SetValueAsync(new CancellationTokenSource());
	}
}

Not sure if we should support null CancellationTokenSource or it should be always initialized.

For the second proposal: on second thought, looking at the code, the CancellationTokenSource is renewed each time Server Loading is called. After a second call, the CancellationTokenSource reference provided by the Parameter will have disappeared.

Another way to overcome this and not use two way binding would be to use CancellationTokenSource.CreateLinkedTokenSource:

private CancellationTokenSource? _cancellationTokenSrc;

[Parameter]
public CancellationTokenSource CancellationTokenSrc { get; set; } = new();

private void CancelToken()
{
    try
    {
        _cancellationTokenSrc?.Cancel();
    }
    catch { /* ignored */ }
    finally
    {
        _cancellationTokenSrc = CancellationTokenSource.CreateLinkedTokenSource(CancellationTokenSrc.Token);
    }
}

Now, when you cancel your CancellationTokenSrc from the parameter, the internal _cancellationTokenSrc will receive IsCancellationRequested = true.

However, if the internal one calls Cancel inside _cancellationTokenSrc?.Cancel();, the external CancellationTokenSrc(provided by user) will not receive IsCancellationRequested.

But I don't think this is important since in ServerReload you would use the CancellationToken from the argument.

If we make this public we need a better name for CancelToken(). What does it cancel?

Also, wouldn't it be better to just make mudtable disposable and dispose the mudtable instead of making CancelToken() public and calling it in Dispose?

@henon The CancelToken method is no more public.
@ScarletKuro I have implemented your second last proposition in the updated PR. Works well.

Also, wouldn't it be better to just make mudtable disposable and dispose the mudtable instead of making CancelToken() public and calling it in Dispose?

MudTable is already disposable, but as I mentioned here, it only calls Dispose on the CancellationTokenSource. However, as the documentation states: Calling Dispose does not communicate a request for cancellation to consumers of the associated Token.

The idea is to request a cancel when you dispose the page containing the MudTable to abort the ServerReload request to the backend. Currently, this is not happening when you dispose of the MudTable.

Can you add a simple test for this?

So wouldn't cancelling the source in MudTable.Dispose() be enough then? No need for the user to know to manually cancel.

ServerLoad already has a cancellation token, so the cancel from the dispose can be received (if dispose calls cancel).
https://mudblazor.com/components/table#server-side-data-with-cancellation

So why would we need the cancellationtokensource parameter then?

Oh, the cancellation token in ServerLoad is supplied by the user to cancel it. But I still don't get why we need this extra parameter. I have a feeling that it is superficial and that the problem @boukenka has can be solved differently without adding another parameter to mudtable.

I guess it could be as simple as that.

        protected virtual void Dispose(bool disposing)
        {
            try { _cancellationTokenSrc?.TryCancel(); } catch { }
            _cancellationTokenSrc?.Dispose();
        }

No need for the mudtable user to need to understand when to cancel a self-supplied token source.

So wouldn't cancelling the source in MudTable.Dispose() be enough then? No need for the user to know to manually cancel.

I honestly don't know if this is recommended practice or not to have this logic internally on Dispose, Microsoft doesn't say anything about it and google doesn't give anything as well. That's why I recommended adding a parameter to have a control over it.

I guess it could be as simple as that.

        protected virtual void Dispose(bool disposing)
        {
            try { _cancellationTokenSrc?.TryCancel(); } catch { }
            _cancellationTokenSrc?.Dispose();
        }

There is no TryCancel btw,

No need for the mudtable user to need to understand when to cancel a self-supplied token source.

I'm not sure if there are no other cases when user would like to cancel the parent _cancellationTokenSrc other than on dispose.

The user shouldn't have to worry about that at all actually. What you suggest is white-box usage which requires intimate knowledge of how the component works. That is against our design philosophy of providing components which simply work with the simplest possible API surface.

The original problem was that the server load function wasn't cancelled on dispose which is clearly just a bug in mudtable that should be fixed and not put on the user's shoulders.

In addition to that, "maybe somebody might want to use it but I don't know if its really needed" is a very weak argument for adding a parameter.

There is no TryCancel btw,

You are right, I switched TaskCompletionSource with CancellationTokenSource in my head.

I'm not sure if there are no other cases when user would like to cancel the parent _cancellationTokenSrc other than on dispose.

I am pretty sure this is not necessary. The sole purpose of the CTS is to communicate to the user's ServerReload code that the result is not needed any more and can be aborted early.

The user has full control over their own ServerLoad code, so they don't need to "tunnel" a cancel through mudtable to cancel their own load func.

@henon @ScarletKuro Based on your feedback, I'll update this PR tonight, following the same logic/implementing the same code as the MudAutoComplete: Cancel during dispose. I hope it is Ok.

they don't need to "tunnel" a cancel through mudtable to cancel their own load func.

I have to say that this isn’t an uncommon practice in dotnet itself. For example, in BackgroundService, you can pass your own CancellationToken in StartAsync, and it will create a linked token using CreateLinkedTokenSource. So you can cancel not only from StopAsync but also from the user's parent CancellationToken.
However, I just realized that suggesting the passing of a CancellationTokenSource was a mistake, it would be better to pass a CancellationToken instead. The user would need to create a CancellationTokenSource and pass CancellationTokenSource.Token.

These are just examples, and I'm not sure if it makes sense to provide an external CancellationToken. It doesn't seem like major Blazor component libraries (FluentUI or Radzen) practice this in their components.

Yeah, in this case it doesn't make sense at all as there is nothing in mudtable that needs to be cancelled by the user. The serverload func may do something that can be cancelled (i.e. making a web request), but the user can always pass their own or a parent cancellation token directly into whatever they call if necessary.

@henon feel free to merge if you think the PR is ready.

Thanks!