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.

Sign up for GitHub

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

Jump to bottom

[go_router]: fix leaking GoRouter.replace and GoRouter.pushReplacement completer, preserve it #6451

Closed
wants to merge 3 commits into from
Closed

[go_router]: fix leaking GoRouter.replace and GoRouter.pushReplacement completer, preserve it #6451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a36b7d
[go_router]: fix leaking GoRouter.replace and GoRouter.pushReplacemen…
pawlowskim Apr 3, 2024
7a01c23
Update CHANGELOG.md and pubspec.yaml
pawlowskim Apr 3, 2024
cca5798
Fix formatting
pawlowskim Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/go_router/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion packages/go_router/lib/src/configuration.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
17 changes: 10 additions & 7 deletions packages/go_router/lib/src/match.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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<Object?> completer;
/// The completers for the future returned by [GoRouter.push].
final List<Completer<Object?>> completers;

/// Called when the corresponding [Route] associated with this route match is
/// completed.
void complete([dynamic value]) {
completer.complete(value);
for (final Completer<Object?> completer in completers) {
completer.complete(value);
}
}

@override
Expand All @@ -464,13 +466,14 @@ class ImperativeRouteMatch extends RouteMatch {
@override
bool operator ==(Object other) {
return other is ImperativeRouteMatch &&
completer == other.completer &&
const ListEquality<Object?>().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.
Expand Down Expand Up @@ -959,7 +962,7 @@ class _RouteMatchListDecoder
pageKey: pageKey,
// TODO(chunhtai): Figure out a way to preserve future.
// https://github.com/flutter/flutter/issues/128122.
completer: Completer<Object?>(),
completers: <Completer<Object?>>[Completer<Object?>()],
matches: imperativeMatchList,
);
matchList = matchList.push(imperativeMatch);
Expand Down
18 changes: 15 additions & 3 deletions packages/go_router/lib/src/parser.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,25 +171,37 @@ class GoRouteInformationParser extends RouteInformationParser<RouteMatchList> {
return baseRouteMatchList!.push(
ImperativeRouteMatch(
pageKey: _getUniqueValueKey(),
completer: completer!,
completers: <Completer<Object?>>[completer!],
matches: newMatchList,
),
);
case NavigatingType.pushReplacement:
final RouteMatch routeMatch = baseRouteMatchList!.last;
final List<Completer<Object?>> completers = <Completer<Object?>>[
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<Completer<Object?>> completers = <Completer<Object?>>[
completer!
];
if (routeMatch is ImperativeRouteMatch) {
completers.addAll(routeMatch.completers);
}
return baseRouteMatchList.remove(routeMatch).push(
ImperativeRouteMatch(
pageKey: routeMatch.pageKey,
completer: completer!,
completers: completers,
matches: newMatchList,
),
);
Expand Down
2 changes: 1 addition & 1 deletion packages/go_router/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

Expand Down
42 changes: 42 additions & 0 deletions packages/go_router/test/delegate_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Object?> 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not look right. the once page-1 is replaced, its future should never be completed. The pop result on line 373 is only meant for page-2. It should not be used to complete page-1.

Copy link
Author

Choose a reason for hiding this comment

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

This does not look right. the once page-1 is replaced, its future should never be completed. The pop result on line 373 is only meant for page-2. It should not be used to complete page-1.

On the other hand, is it good approach to throw any Future into the void (in any case, not only in this particular case)?

This implies that users of go router must carefully design their flow, because feature never ends. And it differs from how the navigator works itself - it completes the future on replacement, which is actually pretty logically.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the other hand, is it good approach to throw any Future into the void (in any case, not only in this particular case)?

No, but sometimes it is unavoidable.

it differs from how the navigator works itself - it completes the future on replacement

Ah you are right, I missed that the navigator completes the previous route for pushReplacement, although the way it completes previous route is different from this pr. We can add a result parameter to pushReplacement to complete previous route. The current implementation uses the same result for both routes seem wrong to me.

The NavigatorState.replace however removes the route without completing it. I think we should keep it that way in go_router

});
});

group('pushReplacementNamed', () {
Expand Down Expand Up @@ -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<Object?> sourceFeature = goRouter.push('/page-1');
await tester.pumpAndSettle();

goRouter.replace<void>('/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', () {
Expand Down
32 changes: 24 additions & 8 deletions packages/go_router/test/match_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: <Completer<Object?>>[completer1]);
ImperativeRouteMatch match2 = ImperativeRouteMatch(
pageKey: key1, matches: matchList1, completer: completer1);
pageKey: key1,
matches: matchList1,
completers: <Completer<Object?>>[completer1]);
expect(match1 == match2, isTrue);
expect(match1.hashCode == match2.hashCode, isTrue);

match1 = ImperativeRouteMatch(
pageKey: key1, matches: matchList1, completer: completer1);
pageKey: key1,
matches: matchList1,
completers: <Completer<Object?>>[completer1]);
match2 = ImperativeRouteMatch(
pageKey: key2, matches: matchList1, completer: completer1);
pageKey: key2,
matches: matchList1,
completers: <Completer<Object?>>[completer1]);
expect(match1 == match2, isFalse);
expect(match1.hashCode == match2.hashCode, isFalse);

match1 = ImperativeRouteMatch(
pageKey: key1, matches: matchList1, completer: completer1);
pageKey: key1,
matches: matchList1,
completers: <Completer<Object?>>[completer1]);
match2 = ImperativeRouteMatch(
pageKey: key1, matches: matchList2, completer: completer1);
pageKey: key1,
matches: matchList2,
completers: <Completer<Object?>>[completer1]);
expect(match1 == match2, isFalse);
expect(match1.hashCode == match2.hashCode, isFalse);

match1 = ImperativeRouteMatch(
pageKey: key1, matches: matchList1, completer: completer1);
pageKey: key1,
matches: matchList1,
completers: <Completer<Object?>>[completer1]);
match2 = ImperativeRouteMatch(
pageKey: key1, matches: matchList1, completer: completer2);
pageKey: key1,
matches: matchList1,
completers: <Completer<Object?>>[completer2]);
expect(match1 == match2, isFalse);
expect(match1.hashCode == match2.hashCode, isFalse);
});
Expand Down
2 changes: 1 addition & 1 deletion packages/go_router/test/matching_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ void main() {
list1.push(ImperativeRouteMatch(
pageKey: const ValueKey<String>('/b-p0'),
matches: list2,
completer: Completer<Object?>()));
completers: <Completer<Object?>>[Completer<Object?>()]));

final Map<Object?, Object?> encoded = codec.encode(list1);
final RouteMatchList decoded = codec.decode(encoded);
Expand Down