[flutter_tools] Generalize waitForExtension #77220
Conversation
2f54c5b to
9832835
Compare
| ]); | ||
| return isolateRef; | ||
| } finally { | ||
| await service.streamCancel(vm_service.EventStreams.kIsolate); |
There was a problem hiding this comment.
I don't know if this is needed - it seems like it's a common pattern here to not cancel streams e.g.
I can follow up and add more streamCancels elsewhere if that is desired though
| // Do nothing, since the tool is already subscribed. | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
I would prefer if this was structured similar to the original code, in a more clearly imperative manner
There was a problem hiding this comment.
Done, please take a look again 🙂
There was a problem hiding this comment.
you need to handle the case where isolates is empty. While you are at it, I would consider what happens if there is more than one uiIsolate.
There was a problem hiding this comment.
I was under the impression that getFlutterViews keeps polling until the isolates are found. Is that not the case here?
Also regarding handling more than one isolate, would it be acceptable to run the querying logic (L813-814) over each uiIsolate found?
There was a problem hiding this comment.
I believe it will return an empty list if the vm service disconnects.
There was a problem hiding this comment.
Sorry, I don't fully understand enough of the VM service to know what to do when it disconnects. Is this a recoverable error where we want to reconnect and make the same call again, or should we fail with an exception?
There was a problem hiding this comment.
In this case, the tool will already trigger a shutdown - but that may be done separate from this call. You could either throw an error that all callsites would need to catch, or return some sort of sentinel
There was a problem hiding this comment.
As discussed over chat, made it throw an exception here, and updated resident_devtools_handler.dart to ignore the current FlutterDevice when this happens
613e0cc to
97c292e
Compare
Genaralizes
waitForExtensionfromresident_devtools_handler.dartand move it tovmservice.dart.This will allow the same logic to be reused in other places (#76200) where waiting for an extension to be registered is needed.
Slight changes in behavior to make it more general purpose:
_flutter.listViewsinstead to find the Flutter isolateFlutter.FrameworkInitialization, which might happen before the vmservice is connected and we won't get the message. Let me know if this isn't okresident_devtools_handler.dart