Azure-Samples / azure-maps-leaflet

A leafletjs plugin that makes it easy to overlay tile layers from the Azure Maps tile services.
https://azure.com/maps
MIT License
17 stars 7 forks source link

Plugin is causing high CPU usage #9

Open b-caby opened 7 months ago

b-caby commented 7 months ago

This issue is for a:

- [x] bug report
- [ ] feature request
- [ ] documentation issue or request
- [ ] regression (a behavior that used to work and stopped in a new release)

Minimal steps to reproduce

Expected/desired behavior

The map should not be consistently use 100% of the browser CPU

Versions

Reproducible using the latest version of the plugin and the latest version of Leaflet (1.9.4 as of today)

Mention any other details that might be useful

Web Workers have been introduced in this PR on July 29, 2022 to fix the issue that setTimeout is not called when the tab is inactive. The worker code contains this line of code while (Date.now() < before + delay) { }; which starts an infinite loop until the number of milliseconds defined in delay is elapsed. This infinite recursion is responsible for the high CPU usage.

I suggest to reimplement the web worker with a proper implementation. I can make a PR to fix this issue.

pn-andrej-zecevic commented 4 months ago

We also experienced this issue.

It was further compounded by the fact that we use NextJS and its client-side navigation. Page navigations appeared to start further workers which would eventually eat up all available CPU and crash the tab.

The loop code highlighted appears to be the culprit.

We have actually taken on a copy of this repository's code due to other unresolved issues. Our solution was to retain the worker, but replace it's workload with a call to the native timer API's setTimeout function. In our testing this worked fine. We opened a session and switched to another tab then every day for 4 days we returned, switched to the test tab, panned and zoomed the map, and tiles loaded successfully via token auth.

Our worker code for reference:

onmessage = function (event) {
  var delay = event.data.time;
  setTimeout(function() {
    postMessage({id: event.data.id});
  }, delay);
};

The worker + loop approach also appears in this SO question: https://stackoverflow.com/questions/72534800

An alternative answer to that question refers to a library that appears to use the native timers API's setTimeout function inside a worker:

Based on the above we will proceed with using setTimeout inside the worker.

@b-caby is this what you ended up with? It would be great to resolve the issue in this repository.

b-caby commented 3 months ago

@pn-andrej-zecevic Thanks for the confirmation of the issue.

We are a Blazor development team with low proficiency in JS. In our case, we had tried to do something similar to your final solution but eventually ended up completely removing the workers and use the native setTimeout (basically reverting the commit I had linked in my previous message). It worked well enough for our use case.

For other internal reasons, we have made the decision to not use Leaflet recently and to now use the Azure Map web control so I wont track this issue anymore. However, I do think your solution is correct and should be merged in.