[flutter_tools] migrate FlutterView to new vm_service #55341

jonahwilliams · 2020-04-21T23:32:30Z

Description

Move FlutterView and related RPCs to the package:vm_service implementation. Update some getIsolate calls with catchError to match previous behavior.

Updates tests that were previously mocking FlutterViews to use real views
Moves the FlutterView cache from VM to FlutterDevice
Catch SentinelException during Isolate.kill

#52179

Maybe #55552

…vm_service

jonahwilliams · 2020-04-24T14:05:53Z

packages/flutter_tools/lib/src/resident_runner.dart

+    for (final FlutterView flutterView in views) {
+      final vm_service.Isolate isolate = await vmService
+        .getIsolate(flutterView.uiIsolate.id)
+        .catchError((dynamic error, StackTrace stackTrace) {


I added a catchError field to all of these because it seems like that is what we were doing before:

https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/vmservice.dart#L1035

Though before we might have been re-using invalid isolates, which is probably worse

Would a trace message here be useful?

Also, this pattern is repeated several times, so creating a new method somewhere for this could be a good cleanup.

The previous trace was not useful, since we don't seem to hit this case often.

Updated to an extension method on VmService

jonahwilliams · 2020-04-24T14:08:02Z

packages/flutter_tools/lib/src/resident_runner.dart

  }
+  final List<FlutterView> _views = <FlutterView>[];

  List<FlutterView> get views {


Moved the filtering here instead of on VM

jonahwilliams · 2020-04-24T14:08:25Z

packages/flutter_tools/lib/src/run_hot.dart

    String reason,
    bool benchmarkMode = false,
  }) async {
-    if (!_isPaused()) {


I could not find a good justification for keeping the pause check

#9733 fixed flutter/flutter-intellij#969, make sure removing this check doesn't regress that.

Sorry, should have been more clear here:

We have a hot reload test in integration.shard that performs a hot restart while paused and that still passes with this removed. I've also verified locally that this listViews RPC returns successfully while paused:

An Observatory debugger and profiler on Pixel 4 is available at: http://127.0.0.1:50743/Nb1i5jI30FQ=/ Performing hot reload... The application is paused due in the debugger; interface might not update. Reloaded 0 of 660 libraries in 107ms. Performing hot reload... The application is paused due in the debugger; interface might not update. Reloaded 0 of 660 libraries in 100ms.

This was most likely a bug in the implementation of listVews. I can do some more research to try and figure out when it was fixed, but for now we should have good coverage

jonahwilliams · 2020-04-24T14:09:04Z

packages/flutter_tools/lib/src/run_hot.dart

-      case ServiceEvent.kPauseException: message.write('paused in the debugger after an exception was thrown'); break;
-      case ServiceEvent.kPausePostRequest: message.write('paused'); break;
-      case '': message.write('paused for various reasons'); break;
+      case vm_service.EventKind.kPauseStart:


Moved statements to different lines, updated constants to point to vm_service

zanderso · 2020-04-24T17:27:30Z

packages/flutter_tools/lib/src/resident_runner.dart

+    for (final FlutterView flutterView in views) {
+      final vm_service.Isolate isolate = await vmService
+        .getIsolate(flutterView.uiIsolate.id)
+        .catchError((dynamic error, StackTrace stackTrace) {


Would a trace message here be useful?

zanderso · 2020-04-24T17:32:48Z

packages/flutter_tools/lib/src/resident_runner.dart

+    for (final FlutterView flutterView in views) {
+      final vm_service.Isolate isolate = await vmService
+        .getIsolate(flutterView.uiIsolate.id)
+        .catchError((dynamic error, StackTrace stackTrace) {


Also, this pattern is repeated several times, so creating a new method somewhere for this could be a good cleanup.

zanderso · 2020-04-24T17:37:24Z

packages/flutter_tools/lib/src/resident_runner.dart

-        assert(!view.uiIsolate.pauseEvent.isPauseEvent);
        futures.add(vmService.flutterExit(
          isolateId: view.uiIsolate.id,
        ));


Regarding the comment below on 282, what would go wrong if we don't wait for flutterExit to fail?

Its not if it fails its that when it succeeds the engine goes down so there is no signal. I could re-arrange this to instead listen to the vm_service done signal, that would remove the need for the timeout

Updated the logic to be a bit simpler, and catch any SentinelException (would be thrown if we exited after the app crashed, for example)

zanderso · 2020-04-24T17:39:28Z

packages/flutter_tools/lib/src/run_hot.dart

-          }));
+      final vm_service.VM vm = await device.vmService.getVM();
+      for (final vm_service.IsolateRef isolateRef in vm.isolates) {
+        if (!uiIsolatesIds.contains(isolateRef.id)) {


Consider flipping the sense of the if with a continue under it, and unindenting the rest of the loop body.

zanderso · 2020-04-24T17:47:53Z

packages/flutter_tools/lib/src/run_hot.dart

+    // Reload the isolate data.
+    await Future.wait(<Future<void>>[
+      for (final FlutterDevice device in flutterDevices)
+        device.refreshViews()


Is device.refreshViews() now also doing the uiIsolate.reload()?

Yeah, we only need the isolateRef data so reloading the full isolate isn't necessary (and we do this as needed anyway below)

zanderso · 2020-04-24T17:49:39Z

packages/flutter_tools/lib/src/vmservice.dart

+         kind == vm_service.EventKind.kPauseException ||
+         kind == vm_service.EventKind.kPausePostRequest ||
+         kind == vm_service.EventKind.kNone;
+}


MIssing newline at EOF

zanderso · 2020-04-24T17:50:48Z

packages/flutter_tools/test/integration.shard/hot_reload_test.dart

    final Completer<void> sawDebuggerPausedMessage = Completer<void>();
    final StreamSubscription<String> subscription = _flutter.stdout.listen(
      (String line) {
+        print('LOG: $line');


stray print?

jmagman · 2020-04-24T17:50:24Z

packages/flutter_tools/lib/src/resident_runner.dart

      return;
    }
-    await flutterDeprecatedVmService.vm.refreshViews(waitForViews: true);
+    final List<FlutterView> newViews = await vmService.getFlutterViews();


How are they new? Isn't this all of them?

Updated to reflect this is an update of the view list

jmagman · 2020-04-24T18:01:53Z

packages/flutter_tools/lib/src/run_hot.dart

    String reason,
    bool benchmarkMode = false,
  }) async {
-    if (!_isPaused()) {


#9733 fixed flutter/flutter-intellij#969, make sure removing this check doesn't regress that.

jmagman · 2020-04-24T18:03:25Z

packages/flutter_tools/lib/src/run_hot.dart


    final Stopwatch reloadTimer = Stopwatch()..start();

-    if (!_isPaused()) {


Same here re: #9733

…tter into remove_more_vm_service

zanderso

lgtm

zanderso · 2020-04-27T18:52:09Z

packages/flutter_tools/lib/src/vmservice.dart

+  /// been collected.
+  Future<vm_service.Isolate> getIsolateOrNull(String isolateId) {
+    return getIsolate(isolateId)
+    .catchError((dynamic error, StackTrace stackTrace) {


nit: 2 space indent on a continued line.

This reverts commit 2e50fd7.

…" (#55772) This reverts commit 2e50fd7.

jonahwilliams added 2 commits April 21, 2020 16:31

migrate FlutterView to vm_service

81a60d6

remove commented out code

0eed17a

fluttergithubbot added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Apr 21, 2020

googlebot added the cla: yes label Apr 21, 2020

jonahwilliams added 5 commits April 22, 2020 15:54

Merge branch 'master' of github.com:flutter/flutter into remove_more_…

e1099b3

…vm_service

fix attach test mocks

42cdbcc

still dont work

625ed59

Merge branch 'master' of github.com:flutter/flutter into remove_more_…

974d72e

…vm_service

remove pause checks

75fc0c3

jonahwilliams commented Apr 24, 2020

View reviewed changes

jonahwilliams and others added 2 commits April 24, 2020 07:11

fix sentinel exception during kill

e0cbd88

Update run_hot.dart

5f5443f

jonahwilliams marked this pull request as ready for review April 24, 2020 14:26

jonahwilliams removed the work in progress; do not review label Apr 24, 2020

jonahwilliams requested review from jmagman and zanderso April 24, 2020 14:27

zanderso reviewed Apr 24, 2020

View reviewed changes

jmagman reviewed Apr 24, 2020

View reviewed changes

jonahwilliams added 4 commits April 24, 2020 13:57

Address comments

d1bd431

clean up exit logic and catch sentinel

2c1f70e

Merge branch 'remove_more_vm_service' of github.com:jonahwilliams/flu…

345b337

…tter into remove_more_vm_service

handle vmservice exit

60f6c6b

zanderso approved these changes Apr 27, 2020

View reviewed changes

Update vmservice.dart

e960fb1

jonahwilliams merged commit 2e50fd7 into flutter:master Apr 27, 2020

jonahwilliams deleted the remove_more_vm_service branch April 27, 2020 21:16

jonahwilliams pushed a commit that referenced this pull request Apr 27, 2020

Revert "[flutter_tools] migrate FlutterView to new vm_service (#55341)"

cf8fe65

This reverts commit 2e50fd7.

jonahwilliams mentioned this pull request Apr 27, 2020

Revert "[flutter_tools] migrate FlutterView to new vm_service" #55772

Merged

jonahwilliams pushed a commit that referenced this pull request Apr 27, 2020

Revert "[flutter_tools] migrate FlutterView to new vm_service (#55341)…

07c451f

…" (#55772) This reverts commit 2e50fd7.

jonahwilliams restored the remove_more_vm_service branch April 27, 2020 21:28

This was referenced Apr 27, 2020

[flutter_tools] reland migrate FlutterView to new vmservice #55774

Merged

[flutter_tools] reland migrate FlutterViews to package:vm_service #55797

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021


		final Stopwatch reloadTimer = Stopwatch()..start();

		if (!_isPaused()) {

[flutter_tools] migrate FlutterView to new vm_service #55341

[flutter_tools] migrate FlutterView to new vm_service #55341

Uh oh!

Conversation

jonahwilliams commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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!

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!

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!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jonahwilliams commented Apr 21, 2020 •

edited

Loading