Make AppContext immutable and race-free by tvolkert · Pull Request #15984 · flutter/flutter

tvolkert · 2018-03-27T18:01:50Z

This updates AppContext per the recommendations in #15352

tvolkert · 2018-03-27T18:02:34Z

@yjbanov @Hixie I committed this in steps to make it easier to review. I recommend viewing one commit at a time.

Hixie · 2018-03-27T20:22:33Z

packages/flutter_tools/lib/src/context_runner.dart

-    context.putIfAbsent(Usage, () => new DisabledUsage());
-    return runner(args);
-  });
+T runInContext<T>(


I recommend making this return Future<T>.

This is because otherwise if you ever consider adding any async code here, and you create a Completer, you won't be able to complete it, because passing a Future to a Completer unwraps the future and causes a type error.

Or perhaps FutureOr<T>?

No, runner had to be made FutureOr<T>, but this now definitely returns Future<T>

yjbanov · 2018-03-27T21:11:29Z

packages/flutter_tools/lib/src/base/context.dart

+  String toString() {
+    final StringBuffer buf = new StringBuffer('Dependency cycle detected: ');
+    buf.write(cycle.join(' -> '));
+    return buf.toString();


nit: this is the same as: => 'Dependency cycle detected: ${cycle.join(' -> ')}'

yjbanov · 2018-03-27T21:30:15Z

packages/flutter_tools/test/base/context_test.dart

+            },
+          );
+          expect(called, isTrue);
+          expect(value, 'child');


Did you mean to put called = true into a parent generator? I can't tell which check shows that the parent was consulted.

No, this just exists to make sure that context.run didn't skip the body entirely. I originally had a "parent consulted" check, but that can't work, because if the parent is consulted, it means it had a generator, so the fallback would never be consulted...

Ah, got it. I was confused by the test description. I thought the test somehow tracked when the fallbacks are consulted relative to the parent.

yjbanov · 2018-03-27T21:34:02Z

packages/flutter_tools/lib/src/context_runner.dart

-    context.putIfAbsent(Usage, () => new DisabledUsage());
-    return runner(args);
-  });
+T runInContext<T>(


Or perhaps FutureOr<T>?

yjbanov · 2018-03-27T21:50:05Z

yjbanov · 2018-03-28T04:42:06Z

packages/flutter_tools/test/base/context_test.dart

+            },
+          );
+          expect(called, isTrue);
+          expect(value, 'child');


Ah, got it. I was confused by the test description. I thought the test somehow tracked when the fallbacks are consulted relative to the parent.

Hixie · 2018-03-28T18:03:56Z

This mostly seems fine but I'm still a bit concerned that you sometimes run with a type that's a Future<x>. I think I would just always use async, that way it's always consistent about what you're doing.

The problem with sometimes expecting a Future<x> is that if you ever need to wrap it in something async, it's going to try to unpack the value, which will be a source of weird bugs.

Also sometimes you don't type the call to run<T>.

tvolkert · 2018-03-28T18:22:09Z

#16034

Follow-up comments to #15984

aam · 2018-03-29T04:59:46Z

This seems to have broken running flutter with local engine:

flutter run -v --local-engine android_debug_unopt_x86

## exception

ArgumentError: Invalid argument(s): join(null, "../engine/src"): part 0 was null, but part 1 was not.

#0      _validateArgList (package:path/src/context.dart:1088:5)
#1      Context.join (package:path/src/context.dart:231:5)
#2      FlutterCommandRunner._findEnginePath (package:flutter_tools/src/runner/flutter_command_runner.dart:327:51)

Reason for the failure is that now we use Cache.flutterRoot in _findEnginePath before it had a chance to get initialized Cache.flutterRoot = fs.path.normalize(fs.path.absolute(flutterRoot));

tvolkert · 2018-03-29T05:47:23Z

@aam, fixed in #16057

It's required to be set before we detect local engine. Was broken by #15984

This updates AppContext per the recommendations in flutter#15352 Fixes flutter#15352

Follow-up comments to flutter#15984

It's required to be set before we detect local engine. Was broken by flutter#15984

tvolkert added 4 commits March 27, 2018 10:56

Update Appcontext API and tests

a8f0e85

Update usages of context.runInZone to use new API

0360708

Update context-aware entities to use new API

6095d54

Update affected tests

6a5cfcd

googlebot added the cla: yes label Mar 27, 2018

tvolkert requested review from Hixie and yjbanov March 27, 2018 18:01

Hixie reviewed Mar 27, 2018

View reviewed changes

Review comments and test improvements

2d03c6c

yjbanov reviewed Mar 27, 2018

View reviewed changes

yjbanov approved these changes Mar 27, 2018

View reviewed changes

yjbanov approved these changes Mar 28, 2018

View reviewed changes

tvolkert added 2 commits March 28, 2018 09:06

Better toString() impl per review comment

c52a91b

Remove skip:true in test that was accidentally checked in

f8a04af

tvolkert merged commit 8d11f5c into flutter:master Mar 28, 2018

tvolkert deleted the context branch March 28, 2018 17:58

tvolkert mentioned this pull request Mar 28, 2018

Make AppContext API not be prone to Future oddities #16034

Merged

tvolkert added a commit that referenced this pull request Mar 28, 2018

Make AppContext API not be prone to Future oddities (#16034)

f8058d7

Follow-up comments to #15984

tvolkert mentioned this pull request Mar 29, 2018

Move Cache.flutterRoot initialization up sooner #16057

Merged

tvolkert added a commit that referenced this pull request Mar 29, 2018

Move Cache.flutterRoot initialization up sooner (#16057)

f69d125

It's required to be set before we detect local engine. Was broken by #15984

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018

Make AppContext immutable and race-free (flutter#15984)

89f61f2

This updates AppContext per the recommendations in flutter#15352 Fixes flutter#15352

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018

Make AppContext API not be prone to Future oddities (flutter#16034)

38a1d67

Follow-up comments to flutter#15984

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018

Move Cache.flutterRoot initialization up sooner (flutter#16057)

2910347

It's required to be set before we detect local engine. Was broken by flutter#15984

github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2021

Conversation

tvolkert commented Mar 27, 2018

Uh oh!

tvolkert commented Mar 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yjbanov commented Mar 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Mar 28, 2018

Uh oh!

tvolkert commented Mar 28, 2018

Uh oh!

aam commented Mar 29, 2018

Uh oh!

tvolkert commented Mar 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants