From 7a36b7de3495472eeba45f2a8ca590ab00dc4b21 Mon Sep 17 00:00:00 2001 From: "marcin.pawlowski" Date: Wed, 3 Apr 2024 11:37:47 +0200 Subject: [PATCH 1/3] [go_router]: fix leaking GoRouter.replace and GoRouter.pushReplacement completer, preserve it (#141251) When route from the stack is replaced, its completer is lost, and causes futures in routes chain to never finish. --- packages/go_router/lib/src/configuration.dart | 2 +- packages/go_router/lib/src/match.dart | 16 +++---- packages/go_router/lib/src/parser.dart | 18 ++++++-- packages/go_router/test/delegate_test.dart | 42 +++++++++++++++++++ packages/go_router/test/match_test.dart | 32 ++++++++++---- packages/go_router/test/matching_test.dart | 2 +- 6 files changed, 92 insertions(+), 20 deletions(-) diff --git a/packages/go_router/lib/src/configuration.dart b/packages/go_router/lib/src/configuration.dart index 09764747aae7..90c1234c216d 100644 --- a/packages/go_router/lib/src/configuration.dart +++ b/packages/go_router/lib/src/configuration.dart @@ -324,7 +324,7 @@ class RouteConfiguration { pageKey: imperativeMatch.pageKey, matches: findMatch(imperativeMatch.matches.uri.toString(), extra: imperativeMatch.matches.extra), - completer: imperativeMatch.completer); + completers: imperativeMatch.completers); result = result.push(match); } return result; diff --git a/packages/go_router/lib/src/match.dart b/packages/go_router/lib/src/match.dart index fba08f838656..85b173ec3930 100644 --- a/packages/go_router/lib/src/match.dart +++ b/packages/go_router/lib/src/match.dart @@ -422,7 +422,7 @@ class ShellRouteMatch extends RouteMatchBase { class ImperativeRouteMatch extends RouteMatch { /// Constructor for [ImperativeRouteMatch]. ImperativeRouteMatch( - {required super.pageKey, required this.matches, required this.completer}) + {required super.pageKey, required this.matches, required this.completers}) : super( route: _getsLastRouteFromMatches(matches), matchedLocation: _getsMatchedLocationFromMatches(matches), @@ -446,13 +446,15 @@ class ImperativeRouteMatch extends RouteMatch { /// The matches that produces this route match. final RouteMatchList matches; - /// The completer for the future returned by [GoRouter.push]. - final Completer completer; + /// The completers for the future returned by [GoRouter.push]. + final List> completers; /// Called when the corresponding [Route] associated with this route match is /// completed. void complete([dynamic value]) { - completer.complete(value); + for (final Completer completer in completers) { + completer.complete(value); + } } @override @@ -464,13 +466,13 @@ class ImperativeRouteMatch extends RouteMatch { @override bool operator ==(Object other) { return other is ImperativeRouteMatch && - completer == other.completer && + const ListEquality().equals(completers, other.completers) && matches == other.matches && super == other; } @override - int get hashCode => Object.hash(super.hashCode, completer, matches.hashCode); + int get hashCode => Object.hash(super.hashCode, Object.hashAll(completers), matches.hashCode); } /// The list of [RouteMatchBase] objects. @@ -959,7 +961,7 @@ class _RouteMatchListDecoder pageKey: pageKey, // TODO(chunhtai): Figure out a way to preserve future. // https://github.com/flutter/flutter/issues/128122. - completer: Completer(), + completers: >[Completer()], matches: imperativeMatchList, ); matchList = matchList.push(imperativeMatch); diff --git a/packages/go_router/lib/src/parser.dart b/packages/go_router/lib/src/parser.dart index 9cd92c1848b6..ac0f65928c37 100644 --- a/packages/go_router/lib/src/parser.dart +++ b/packages/go_router/lib/src/parser.dart @@ -171,25 +171,37 @@ class GoRouteInformationParser extends RouteInformationParser { return baseRouteMatchList!.push( ImperativeRouteMatch( pageKey: _getUniqueValueKey(), - completer: completer!, + completers: >[completer!], matches: newMatchList, ), ); case NavigatingType.pushReplacement: final RouteMatch routeMatch = baseRouteMatchList!.last; + final List> completers = >[ + completer! + ]; + if (routeMatch is ImperativeRouteMatch) { + completers.addAll(routeMatch.completers); + } return baseRouteMatchList.remove(routeMatch).push( ImperativeRouteMatch( pageKey: _getUniqueValueKey(), - completer: completer!, + completers: completers, matches: newMatchList, ), ); case NavigatingType.replace: final RouteMatch routeMatch = baseRouteMatchList!.last; + final List> completers = >[ + completer! + ]; + if (routeMatch is ImperativeRouteMatch) { + completers.addAll(routeMatch.completers); + } return baseRouteMatchList.remove(routeMatch).push( ImperativeRouteMatch( pageKey: routeMatch.pageKey, - completer: completer!, + completers: completers, matches: newMatchList, ), ); diff --git a/packages/go_router/test/delegate_test.dart b/packages/go_router/test/delegate_test.dart index 6513a1c9665f..d617bb7514ab 100644 --- a/packages/go_router/test/delegate_test.dart +++ b/packages/go_router/test/delegate_test.dart @@ -355,6 +355,27 @@ void main() { ); }, ); + + testWidgets('It should preserver source completer', + (WidgetTester tester) async { + final GoRouter goRouter = await createGoRouter(tester); + + goRouter.push('/page-0'); + await tester.pumpAndSettle(); + + final Future sourceFeature = goRouter.push('/page-1'); + await tester.pumpAndSettle(); + + goRouter.pushReplacement('/page-2'); + await tester.pumpAndSettle(); + + expect(goRouter.routerDelegate.currentConfiguration.matches.length, 3); + goRouter.routerDelegate.pop(true); + await tester.pumpAndSettle(); + + expect(goRouter.routerDelegate.currentConfiguration.matches.length, 2); + expect(await sourceFeature, true); + }); }); group('pushReplacementNamed', () { @@ -516,6 +537,27 @@ void main() { ); }, ); + + testWidgets('It should preserver source completer', + (WidgetTester tester) async { + final GoRouter goRouter = await createGoRouter(tester); + + goRouter.push('/page-0'); + await tester.pumpAndSettle(); + + final Future sourceFeature = goRouter.push('/page-1'); + await tester.pumpAndSettle(); + + goRouter.replace('/page-2'); + await tester.pumpAndSettle(); + + expect(goRouter.routerDelegate.currentConfiguration.matches.length, 3); + goRouter.routerDelegate.pop(true); + await tester.pumpAndSettle(); + + expect(goRouter.routerDelegate.currentConfiguration.matches.length, 2); + expect(await sourceFeature, true); + }); }); group('replaceNamed', () { diff --git a/packages/go_router/test/match_test.dart b/packages/go_router/test/match_test.dart index 57951b2ae236..eafadc0e4a68 100644 --- a/packages/go_router/test/match_test.dart +++ b/packages/go_router/test/match_test.dart @@ -223,30 +223,46 @@ void main() { test('can equal and has', () async { ImperativeRouteMatch match1 = ImperativeRouteMatch( - pageKey: key1, matches: matchList1, completer: completer1); + pageKey: key1, + matches: matchList1, + completers: >[completer1]); ImperativeRouteMatch match2 = ImperativeRouteMatch( - pageKey: key1, matches: matchList1, completer: completer1); + pageKey: key1, + matches: matchList1, + completers: >[completer1]); expect(match1 == match2, isTrue); expect(match1.hashCode == match2.hashCode, isTrue); match1 = ImperativeRouteMatch( - pageKey: key1, matches: matchList1, completer: completer1); + pageKey: key1, + matches: matchList1, + completers: >[completer1]); match2 = ImperativeRouteMatch( - pageKey: key2, matches: matchList1, completer: completer1); + pageKey: key2, + matches: matchList1, + completers: >[completer1]); expect(match1 == match2, isFalse); expect(match1.hashCode == match2.hashCode, isFalse); match1 = ImperativeRouteMatch( - pageKey: key1, matches: matchList1, completer: completer1); + pageKey: key1, + matches: matchList1, + completers: >[completer1]); match2 = ImperativeRouteMatch( - pageKey: key1, matches: matchList2, completer: completer1); + pageKey: key1, + matches: matchList2, + completers: >[completer1]); expect(match1 == match2, isFalse); expect(match1.hashCode == match2.hashCode, isFalse); match1 = ImperativeRouteMatch( - pageKey: key1, matches: matchList1, completer: completer1); + pageKey: key1, + matches: matchList1, + completers: >[completer1]); match2 = ImperativeRouteMatch( - pageKey: key1, matches: matchList1, completer: completer2); + pageKey: key1, + matches: matchList1, + completers: >[completer2]); expect(match1 == match2, isFalse); expect(match1.hashCode == match2.hashCode, isFalse); }); diff --git a/packages/go_router/test/matching_test.dart b/packages/go_router/test/matching_test.dart index 89cbccb8a5ee..416a99725767 100644 --- a/packages/go_router/test/matching_test.dart +++ b/packages/go_router/test/matching_test.dart @@ -97,7 +97,7 @@ void main() { list1.push(ImperativeRouteMatch( pageKey: const ValueKey('/b-p0'), matches: list2, - completer: Completer())); + completers: >[Completer()])); final Map encoded = codec.encode(list1); final RouteMatchList decoded = codec.decode(encoded); From 7a01c23da608e1a3300ed8bb9dea3536465f0b8c Mon Sep 17 00:00:00 2001 From: "marcin.pawlowski" Date: Wed, 3 Apr 2024 12:07:21 +0200 Subject: [PATCH 2/3] Update CHANGELOG.md and pubspec.yaml --- packages/go_router/CHANGELOG.md | 4 ++++ packages/go_router/pubspec.yaml | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/go_router/CHANGELOG.md b/packages/go_router/CHANGELOG.md index d268648720c3..82c70d902fd8 100644 --- a/packages/go_router/CHANGELOG.md +++ b/packages/go_router/CHANGELOG.md @@ -1,3 +1,7 @@ +## 13.2.3 + +- Fixes leaked future from route that was replaced ([#141251](https://github.com/flutter/flutter/issues/141251)) + ## 13.2.2 - Fixes restoreRouteInformation issue when GoRouter.optionURLReflectsImperativeAPIs is true and the last match is ShellRouteMatch diff --git a/packages/go_router/pubspec.yaml b/packages/go_router/pubspec.yaml index ac7945e62ece..1fc99858822f 100644 --- a/packages/go_router/pubspec.yaml +++ b/packages/go_router/pubspec.yaml @@ -1,7 +1,7 @@ name: go_router description: A declarative router for Flutter based on Navigation 2 supporting deep linking, data-driven routes and more -version: 13.2.2 +version: 13.2.3 repository: https://github.com/flutter/packages/tree/main/packages/go_router issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+go_router%22 From cca5798fbf790c87e32a7f32ee297444a98ed795 Mon Sep 17 00:00:00 2001 From: "marcin.pawlowski" Date: Wed, 3 Apr 2024 12:46:33 +0200 Subject: [PATCH 3/3] Fix formatting --- packages/go_router/lib/src/match.dart | 3 ++- packages/go_router/test/delegate_test.dart | 28 +++++++++++----------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/packages/go_router/lib/src/match.dart b/packages/go_router/lib/src/match.dart index 85b173ec3930..81f11072f1b5 100644 --- a/packages/go_router/lib/src/match.dart +++ b/packages/go_router/lib/src/match.dart @@ -472,7 +472,8 @@ class ImperativeRouteMatch extends RouteMatch { } @override - int get hashCode => Object.hash(super.hashCode, Object.hashAll(completers), matches.hashCode); + int get hashCode => + Object.hash(super.hashCode, Object.hashAll(completers), matches.hashCode); } /// The list of [RouteMatchBase] objects. diff --git a/packages/go_router/test/delegate_test.dart b/packages/go_router/test/delegate_test.dart index d617bb7514ab..cbd254af7796 100644 --- a/packages/go_router/test/delegate_test.dart +++ b/packages/go_router/test/delegate_test.dart @@ -539,25 +539,25 @@ void main() { ); testWidgets('It should preserver source completer', - (WidgetTester tester) async { - final GoRouter goRouter = await createGoRouter(tester); + (WidgetTester tester) async { + final GoRouter goRouter = await createGoRouter(tester); - goRouter.push('/page-0'); - await tester.pumpAndSettle(); + goRouter.push('/page-0'); + await tester.pumpAndSettle(); - final Future sourceFeature = goRouter.push('/page-1'); - await tester.pumpAndSettle(); + final Future sourceFeature = goRouter.push('/page-1'); + await tester.pumpAndSettle(); - goRouter.replace('/page-2'); - await tester.pumpAndSettle(); + goRouter.replace('/page-2'); + await tester.pumpAndSettle(); - expect(goRouter.routerDelegate.currentConfiguration.matches.length, 3); - goRouter.routerDelegate.pop(true); - await tester.pumpAndSettle(); + expect(goRouter.routerDelegate.currentConfiguration.matches.length, 3); + goRouter.routerDelegate.pop(true); + await tester.pumpAndSettle(); - expect(goRouter.routerDelegate.currentConfiguration.matches.length, 2); - expect(await sourceFeature, true); - }); + expect(goRouter.routerDelegate.currentConfiguration.matches.length, 2); + expect(await sourceFeature, true); + }); }); group('replaceNamed', () {