Problem/Motivation

When a marker's `iconSize` is left empty, `LeafletService::setSizeIfEmptyOrInvalid()` measures the icon by fetching it over HTTP.
There are at least three issues with this method:

  1. The request has no timeout
  2. Failures are never cached
  3. Icons served by the site itself are fetched over HTTP

Point 3 especially is dangerous, because the HTTP request loads the fully bootstrapped Drupal 404 page if the file doesn't exist.

Steps to reproduce

  1. Create a Leaflet view with a marker icon and leave Icon Size x/y empty
  2. Enter an icon URL that does not resolve
  3. Load the view with a few hundred results

Every marker performs a blocking HTTP request on every render, and nothing is cached between them.

Relationship to existing issues

  • #3254170 ("Using map icons from image/url path breaks view after upgrade") reports the same mechanism: `get_headers()` / `fileExists()` running once per map element, with reported figures in the region of 15s for 100 elements and an unusable page at 800. It is closed as works as designed. I would like to reopen the underlying concern: what was reported there as slowness is, on an infrastructure with a shared worker pool, an availability failure.
  • #3312282 ("Cache icon size in `setFeatureIconSizesIfEmptyOrInvalid()`") added the static cache in 2.2.10, and the persistent `leaflet_map_icon_size:` bin followed. Both are positive-only, so neither bounds the failure case.

Proposed resolution

  1. Read icons from disk when they are served by this site
  2. Do not request an own-host URL that does not resolve to a file
  3. Bound and cache the genuinely remote case

Remaining tasks

  • [X] Agree on the approach, in particular whether reading local files from disk is acceptable
  • [X] Merge request against 10.4.x
  • [-] Backport consideration for 10.2.x / 2.2.x
  • [X] Tests: a unit/kernel test asserting that an unresolvable icon is probed at most once
  • [X] Consider a warning on the icon settings form when both size fields are left empty

User interface changes

None

API changes

`LeafletService::__construct()` gains an `@http_client` argument.

Data model changes

None

Disclaimer: The help of AI was used for documentation of both this issue description and the MR code documentation.

Issue fork leaflet-3613592

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

randalv created an issue. See original summary.

randalv opened merge request !80

randalv’s picture

randalv commented
Issue summary: View changes
Status: Active » Needs review
itamair’s picture

itamair commented

thanks @randalv, going to review this asap ...

itamair’s picture

itamair commented
Status: Needs review » Reviewed & tested by the community

@randalv I tested your MR!80 and I should declare that is a very solid and effective contribution.

Great job!.

I tested it on a couple of complex websites implementing complex Leaflet Map views, with hundreds of markers,
and all wen good:
- no clear issues;
- not unexpected outcomes ...
and some appreciable performance improvements.

I would also agree on all your issue motivations, and the implemented code quality (both PHP and Drupal).
I didn't experience any application issue in the actual 10.4.x-dev head and 10.4.9 Leaflet release. So no clear backward issue ...

Thus, I am going to set this with RTBC state and I’d be happy to merge MR !80 into 10.4.x-dev branch.
What do you think? Should we move ahead with all this???

Regarding the other/remaining tasks you mention I have the following opinion at the moment:
[ ] Backport consideration for 10.2.x / 2.2.x, etc.
I don't think we should care about previous Leaflet dev branches anymore with this issue. Those are practically already unsupported for further devs …

[ ] Tests: a unit/kernel test asserting that an unresolvable icon is probed at most once
Well, I don’t have time to implement this at the moment. Would you now here? 
Otherwise we could open a specific new follow-up issue with that.

[ ] Consider a warning on the icon settings form when both size fields are left empty.
What kind of further warning are you specifically thinking about?
For the icon (and shadow icon) settings form element there is already the following warning:
“Size of the icon image in pixels (if empty the natural icon image size will be used).
Both support Replacement Patterns and should end up into an Integer (positive value)
If one value is null it will be derived from the populated one, according to the natural icon image size rate.”

Let me know what you think on all this. 
Is this ready indeed to be merged into the 10.4.x-dev … (the same as it looks to me)?

randalv’s picture

randalv commented

Hi @itamair,

Thanks for getting back so quickly!

I do think it is ready to be merged into 10.4.x-dev, regarding the remaining open tasks:

[ ] Backport consideration for 10.2.x / 2.2.x, etc.
--> Agreed that this isn't necessary, if further development will only happen on the 10.4.x branch!

[ ] Tests: a unit/kernel test asserting that an unresolvable icon is probed at most once
--> Let's keep this for a follow-up issue 😃
Actually, I would like to implement this still, I'll work on it and come back with an update ASAP. I think it's a valuable test to have

[ ] Consider a warning on the icon settings form when both size fields are left empty.
--> I think this part `(if empty the natural icon image size will be used)` could probably be extended to make sure the reader knows this means a file lookup happens (per unique marker) to retrieve the size (even if this can be taken from context as well), with the static cache and local filesystem lookup I figure this probably isn't as big of an issue anymore, but it still makes sense.

randalv’s picture

randalv commented
Issue summary: View changes

Update:

  • Added the unit test, passes, can be tested locally via: `vendor/bin/phpunit -c web/core web/modules/contrib/leaflet/tests/src/Unit/LeafletIconSizeTest.php`
  • Also extended the warning message a bit, making sure people know what happens beneath the surface when iconSize is left empty

Unit test result:

ddev exec vendor/bin/phpunit -c web/core web/modules/contrib/leaflet/tests/src/Unit/LeafletIconSizeTest.php
PHPUnit 9.6.35 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\leaflet\Unit\LeafletIconSizeTest
.                                                                   1 / 1 (100%)

Time: 00:00.038, Memory: 8.00 MB

OK (1 test, 5 assertions)
itamair’s picture

itamair commented
Status: Reviewed & tested by the community » Fixed

Great job @randalv, merging all this ...

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • itamair committed deddab36 on 10.4.x authored by randalv 
    fix: #3613592 Icon size lookup can exhaust the PHP worker pool

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.