[google_map_flutter] Fix map object regression due to async changes #4171

stuartmorgan · 2023-06-09T15:34:59Z

In #4067 I changed several implicitly unawaited futures to await, not noticing that doing so would change the timing on updating fields that were used to compute diffs for future updates. Since the update flow is unawaited at higher levels, this meant that multiple calls to update map objects in rapid succession would compute incorrect diffs due to using the wrong base objects. This fixes that by restoring the old flow using unawaited.

Adds new tests that introduce synthetic awaits into the fake platform interface object and perform multiple updates in a row that should have different base object sets, which fail without the fix.

In order to simplify adding that behavior to the fake, and to pay down technical debt in the unit tests, I replaced the old test fake that is based on method channel interception (which predates the federation of the plugin and is implicitly relying on the method channel implementation from a different package never changing) with a fake platform interface implementation (substantially simplifying the fake).

Fixes flutter/flutter#128042

Pre-launch Checklist

ditman · 2023-06-13T21:36:09Z

because google_maps_flutter_web_integration_tests depends on google_maps_flutter from path, version solving failed.

I think this must have gotten messed up with my federation changes:

https://github.com/flutter/packages/blob/main/packages/google_maps_flutter/google_maps_flutter_web/example/pubspec.yaml#L23

There's a circular dependency in the web version of the integration tests so we can render a real map. Maybe that test needs to be moved (actually, "made work") in the core plugin?

stuartmorgan · 2023-06-14T15:18:16Z

There's a circular dependency in the web version of the integration tests so we can render a real map. Maybe that test needs to be moved (actually, "made work") in the core plugin?

I've hacked around it for now, but removing the circular dependency would definitely be better in the longer term.

tarrinneal

.

tarrinneal

I don't see anything clearly wrong here, but I'm not familiar with the plugin either. Stamped with reservations. If @ditman has time to take a gander, that would be swell.

stuartmorgan · 2023-06-15T19:29:22Z

@ditman I'll plan to land this by EoD tomorrow to get the regression fix out.

ditman

Hit it! (This is so weird, though!)

ditman · 2023-06-16T03:30:36Z

packages/google_maps_flutter/google_maps_flutter/lib/src/google_map.dart

-    await controller._updatePolygons(
-        PolygonUpdates.from(_polygons.values.toSet(), widget.polygons));
+    unawaited(controller._updatePolygons(
+        PolygonUpdates.from(_polygons.values.toSet(), widget.polygons)));


Do you have any idea on what can be improved in the plugin so it can go back to the non-unawaited version? This feels a little bit like the plugin is relying in race conditions all the time to work!

Whether the overall design of these enclosing functions not every being awaited can be addressed without breaking changes to clients I'm not sure yet. I believe these could be fixed locally to await by changing the flow of each one to:

copy the current value of _foo to oldFoo

update _foo (currently done after the unawaited async)

do an awaited call using oldFoo instead of _foo as the previous value argument.

I considered doing that in this PR, or leaving a TODO in the code to do it later, but decided that since the only current use of these private methods is from a non-async function that can't await them anyway it wasn't clear that there was a real benefit to doing so. We could certainly revisit though, since I agree it feels a bit weird.

…hanges (flutter/packages#4171)

flutter/packages@f9314a3...59d93d6 2023-06-16 [email protected] [tool] Add command aliases (flutter/packages#4207) 2023-06-16 [email protected] [google_map_flutter] Fix map object regression due to async changes (flutter/packages#4171) 2023-06-16 [email protected] [url_launcher] Add ignores for deprecated member to test (flutter/packages#4220) 2023-06-15 [email protected] Roll Flutter from 95be76a to b0188cd (10 revisions) (flutter/packages#4221) 2023-06-15 [email protected] [camera_android] Upgrading roboelectric from 4.5 to 4.10.3 (flutter/packages#4018) 2023-06-15 [email protected] [go_router] Fixes bug that GoRouterState in top level redirect doesn'â�¦ (flutter/packages#4173) 2023-06-15 [email protected] [go_router]Updates documentations around GoRouter.of, GoRouter.maybeOf, and BuildContext extension. (flutter/packages#4176) 2023-06-15 [email protected] [tool] Support code excerpts for any .md file (flutter/packages#4212) 2023-06-15 [email protected] [webview_flutter] Add support for limitsNavigationsToAppBoundDomains (flutter/packages#4026) 2023-06-15 [email protected] [webview_flutter][webview_flutter_android] Add android support for handling geolocation permissions (flutter/packages#3795) 2023-06-15 [email protected] [image_picker] add getMedia method (flutter/packages#3892) 2023-06-15 [email protected] [image_picker] getMedia platform implementations (flutter/packages#4175) 2023-06-14 [email protected] Ignore `textScaleFactor` deprecation (flutter/packages#4209) 2023-06-14 [email protected] [pigeon] Enable Obj-C integration tests in CI (flutter/packages#4215) 2023-06-14 [email protected] [flutter_adaptive_scaffold] Support RTL (flutter/packages#4204) 2023-06-14 [email protected] Manual roll Flutter from 09b7e56 to 95be76a (14 revisions) (flutter/packages#4214) 2023-06-14 [email protected] Roll Flutter (stable) from 682aa38 to 796c8ef (5 revisions) (flutter/packages#4213) 2023-06-14 [email protected] Roll Flutter from 353b8bc to 09b7e56 (17 revisions) (flutter/packages#4206) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

jokerttu · 2023-08-17T06:08:26Z

@stuartmorgan this issue is still present at example_google_map.dart files used by iOS and Android implementation examples and integration tests; where await is used instead of unawaited causing sync issues.

stuartmorgan · 2023-08-17T14:14:44Z

@stuartmorgan this issue is still present at example_google_map.dart files used by iOS and Android implementation examples and integration tests; where await is used instead of unawaited causing sync issues.

Whoops! Thanks for catching that; I'll get a PR posted shortly.

Applies the fixes from flutter#4171 to the minimal ExampleGoogleMaps class used in the Android and iOS implementation package examples, as they had the same issue. Also sets up unit testing of the examples in order to add testing of this fix. The fake is copied directly from the app-facing package, and the unit tests are slightly modified versions of the tests in the app-facing package as well.

stuartmorgan · 2023-08-17T14:36:34Z

#4729 will fix the implementation packages.

Applies the fixes from #4171 to the minimal ExampleGoogleMaps class used in the Android and iOS implementation package examples, as they had the same issue. Also sets up unit testing of the examples in order to add testing of this fix. The fake is copied directly from the app-facing package, and the unit tests are slightly modified versions of the tests in the app-facing package as well.

stuartmorgan added 7 commits June 9, 2023 09:29

Extract fake platform impl to a separate file

6693a3d

Wire up a platform recorder

0c41040

Update all map object tests to use the new recorder

8b2580b

Convert remaining test and remove old fake controller

4c0d146

Add new multi-update tests with fake delays

7791289

Remove incorrect awaits

4ea63c7

Version bump

80f0354

stuartmorgan requested review from ditman and bparrishMines June 9, 2023 15:34

github-actions bot added the p: google_maps_flutter label Jun 9, 2023

stuartmorgan added 2 commits June 9, 2023 11:36

Merge branch 'main' into maps-marker-regression-fix

488becc

Merge branch 'main' into maps-marker-regression-fix

3faf721

stuartmorgan requested a review from tarrinneal June 13, 2023 19:44

Add a dependency override in the web example

a4c502c

github-actions bot added the platform-web label Jun 14, 2023

tarrinneal reviewed Jun 15, 2023

View reviewed changes

tarrinneal self-requested a review June 15, 2023 19:21

tarrinneal approved these changes Jun 15, 2023

View reviewed changes

ditman approved these changes Jun 16, 2023

View reviewed changes

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 16, 2023

auto-submit bot merged commit 754405d into flutter:main Jun 16, 2023

stuartmorgan deleted the maps-marker-regression-fix branch June 16, 2023 12:43

engine-flutter-autoroll mentioned this pull request Jun 16, 2023

Roll Packages from f9314a3b833a to 59d93d64cb83 (18 revisions) flutter/flutter#129014

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2023

754405d [google_map_flutter] Fix map object regression due to async c…

a8ffc7b

…hanges (flutter/packages#4171)

engine-flutter-autoroll mentioned this pull request Jun 16, 2023

Roll Packages from f9314a3b833a to 59d93d64cb83 (18 revisions) flutter/flutter#129021

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2023

754405d [google_map_flutter] Fix map object regression due to async c…

55c3b92

…hanges (flutter/packages#4171)

This was referenced Jul 7, 2023

[Tool] New tool to download android dependencies #4408

Merged

[camera] tree fix format 2023 05 12 #4450

Closed

[tooling] mark infra steps as infra steps in yaml files #4473

Merged

reidbaker mentioned this pull request Jul 26, 2023

[Tooling] Add google owned cache for dependencies as an option in ci #4567

Merged

11 tasks

reidbaker mentioned this pull request Aug 4, 2023

[All] Expand artifact hub to all plugins #4645

Merged

11 tasks

stuartmorgan mentioned this pull request Aug 17, 2023

[google_maps_flutter] Fix async handling in examples #4729

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[google_map_flutter] Fix map object regression due to async changes #4171

[google_map_flutter] Fix map object regression due to async changes #4171

stuartmorgan commented Jun 9, 2023

ditman commented Jun 13, 2023

stuartmorgan commented Jun 14, 2023

tarrinneal left a comment •

edited

Loading

tarrinneal left a comment

stuartmorgan commented Jun 15, 2023

ditman left a comment

ditman Jun 16, 2023

stuartmorgan Jun 16, 2023

jokerttu commented Aug 17, 2023 •

edited

Loading

stuartmorgan commented Aug 17, 2023

stuartmorgan commented Aug 17, 2023

[google_map_flutter] Fix map object regression due to async changes #4171

[google_map_flutter] Fix map object regression due to async changes #4171

Conversation

stuartmorgan commented Jun 9, 2023

Pre-launch Checklist

ditman commented Jun 13, 2023

stuartmorgan commented Jun 14, 2023

tarrinneal left a comment • edited Loading

Choose a reason for hiding this comment

tarrinneal left a comment

Choose a reason for hiding this comment

stuartmorgan commented Jun 15, 2023

ditman left a comment

Choose a reason for hiding this comment

ditman Jun 16, 2023

Choose a reason for hiding this comment

stuartmorgan Jun 16, 2023

Choose a reason for hiding this comment

jokerttu commented Aug 17, 2023 • edited Loading

stuartmorgan commented Aug 17, 2023

stuartmorgan commented Aug 17, 2023

tarrinneal left a comment •

edited

Loading

jokerttu commented Aug 17, 2023 •

edited

Loading