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

Improvement: Added CancellationToken to IProvider.GetFeaturesAsync and remove IDisposable from Map #2564

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

inforithmics
Copy link
Contributor

@inforithmics inforithmics commented Mar 23, 2024

For better Aborting of Features I added a CancellationToken to IProvider.GetFeaturesAsync

I removed the Dispose from the Map because instead of Dispose AbortFecht is now called, it should now abort faster because the CancellationToken is now provided on httpClient calls

@inforithmics inforithmics changed the title Improvement: Added CancellationToken to IProvider.GetFeaturesAsync Improvement: Added CancellationToken to IProvider.GetFeaturesAsync and remove IDisposable from Map Mar 23, 2024
Copy link
Member

@pauldendulk pauldendulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am undecisive about this one. Making the Map none-disposable is an important improvement, but I feel uneasy about adding the cancellation token to an important interface. I hope we can think of an entirely different solution where we do not have to deal with disposing, async and cancelation at this level. First I will take a look at BruTile, where I intent to remove the HttpClient altogether. The caller should have the responsibility of managing the HttpClient (creating (using threadpool?) and disposing). In this case it will be Mapsui, so that won't fix things for us, but maybe we can make similar changes in Mapsui.

@inforithmics
Copy link
Contributor Author

inforithmics commented Mar 30, 2024

I agree, because mapsui heavily depends on Brutile, it's best to have a strategy for this in Brutile and when it's implemented use a similar strategy in mapsui.

In .net most of the Async IO Interfaces have a CancellationToken Parameter for cancelling IO operations. So I simply made the same changes here so that the CancellationToken can be relayed to httpClient, File IO operations. Sqlite pcl net does not support cancellationtokens yet (Or I haven't found out how, so mbtiles-brutile cannot use it).

As I pointed out in the discussion the CancellationToken could be an Additional Property in the FetchInfo, avoiding breaking changes. But then the Code Analyzers wouldn't detect that If it's forgotten to relay the Cancellation Token.

If there is concern about the removing of IDisposable from Map. I could do that in a seperate Pull request. Because with the WeakEvents the only thing that dispose does in the Map is aborting the fetches. Which can be called directly in the Map instead of calling Dispose.

Sample for Cancellation Token interfaces in the .NET Api
https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpclient.getasync?view=net-8.0

Here every method has a variant with and without a CancellationToken.

For easier usage I changed the Interface to this.

public interface IProvider
{
    string? CRS { get; set; }

    MRect? GetExtent();

    Task<IEnumerable<IFeature>> GetFeaturesAsync(FetchInfo fetchInfo)
    {
        return GetFeaturesAsync(fetchInfo, CancellationToken.None);
    }

    Task<IEnumerable<IFeature>> GetFeaturesAsync(FetchInfo fetchInfo, CancellationToken cancellationToken);
}

Which means users don't have to recompile or change code because, if you don't use Cancellation Tokens it will internally call the new Interface with the CancellationToken.None.

Only implementers need to implement the GetFeaturesAsync(FetchInfo fetchInfo, CancellationToken cancellationToken); They need to recompile and change Source Code

In my opinion CancellationToken and Disposability are somehow orthogonally connected.
Meaning the CancellationToken aborts a request faster if it isn't needed anymore, but keeps for example the httpClient alive. which helps for reliability and speed for example when fast zooming or panning aborting not needed requests.
The disposing helps in Disposing unmanged resources and memory faster if something isn't used anymore helping with overall Resource/Memory usage. When the Garbage Collector runs unmanaged resources will be disposed (in the finalizer thread) but it will take longer, than explicitly calling dispose.
The disposing might cancel the request but I'm not sure how the implementation is done in the httpClient for that.

@inforithmics
Copy link
Contributor Author

The failing ArcGisDynamicSample test is fixed here #2574

@inforithmics inforithmics changed the title Improvement: Added CancellationToken to IProvider.GetFeaturesAsync and remove IDisposable from Map WIP Improvement: Added CancellationToken to IProvider.GetFeaturesAsync and remove IDisposable from Map Apr 28, 2024
@inforithmics inforithmics changed the title WIP Improvement: Added CancellationToken to IProvider.GetFeaturesAsync and remove IDisposable from Map Improvement: Added CancellationToken to IProvider.GetFeaturesAsync and remove IDisposable from Map Apr 30, 2024
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

Successfully merging this pull request may close these issues.

None yet

2 participants