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

Remove AsyncLock #2245

Merged
merged 2 commits into from Nov 6, 2023
Merged

Remove AsyncLock #2245

merged 2 commits into from Nov 6, 2023

Conversation

pauldendulk
Copy link
Member

@pauldendulk pauldendulk commented Nov 5, 2023

Some thoughts.

  • This is old code. Originally there was a lock on the provider, added long ago.
  • Later the AsyncLock was added because the method was made async.
  • The FeatureFetcher is the only place where the AsyncLock is used.
  • The FeatureFetcher itself is only called in the ImageLayer. ImageFetcher would have been a more appropriate name.
  • It is not really clear what the purpose of the lock was. To limit the number of simultaneous request seems a logical answer but such lock would result in queing of the various requests, in the context of Mapsui having a queue of items to fetch often does not make sense. This is because the map viewport changes quickly through panning and scrolling and data needed in a previous step is no longer relevant in the next. It is often better to replace an existing pending request. The best solution for Mapsui would be a central fetcher. We have an item on the backlog for this somewhere.
  • The timer in the ImageLayer would prevent simultaneous requests from one ImageLayer. So, the lock would not do anything in that scenario. With multiple ImageLayers it could prevent simultaneous requests. It is not clear if this was ever a real problem.

@@ -30,10 +25,7 @@ public FeatureFetcher(FetchInfo fetchInfo, IProvider provider, DataArrivedDelega

public async Task FetchOnThreadAsync()
{
using (await _providerLock.LockAsync())
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was useful before the Timer Triggered Map Referesh. Now it is just Cargo Programming. (Code without useful functionality).
It could only go into the lock if the request takes longer than 1 second. But then it could hinder Performance because first it waits for the previous request to finish causing to queue all requests. The beauty of async is that several requests could be executed in parallel. And I think the Provider should be Thread Safe anyway.

And RasterizingTileLayer already calls the Providers Multithreadead so they should be Thread Safe (at least the Providers in Mapsui)

Copy link
Contributor

Choose a reason for hiding this comment

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

If an AsyncLock is needed a good AsyncLock Implementation is provided in DotNext.Threading.

DotNext nuget packages are a Testing Ground for new .NET Framework Classes. That might be integrated into the next .NET Framework.

@pauldendulk pauldendulk merged commit c1dceef into master Nov 6, 2023
5 checks passed
@pauldendulk pauldendulk deleted the feature/remove-async-lock branch November 6, 2023 07:53
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