Skip to content

Conversation

jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented May 3, 2020

Description

Fixes #56194

Remove caching of FlutterView. Perhaps the FlutterView RPC might return an empty list if the VM is not quite up yet? We had some old logic to poll the flutter views RPC for up to 200ms. That doesn't seem like a great approach, so instead we could forgo it entirely and trust that either the views come up before the developer tries to interact, or we crash.

https://github.com/flutter/flutter/blob/stable/packages/flutter_tools/lib/src/vmservice.dart#L1070

@jonahwilliams jonahwilliams marked this pull request as ready for review May 4, 2020 15:17
@jonahwilliams jonahwilliams requested review from jmagman and zanderso May 4, 2020 15:17
@jonahwilliams
Copy link
Contributor Author

nvm, i misread the code, we waited forever lol. I wonder if instead we should wait for isolate runnable? Otherwise I could patch wait forever back in, seems bad to do polling if we don't need to.

@zanderso
Copy link
Member

zanderso commented May 4, 2020

I think I don't quite follow, but waiting forever here sounds in line with the policy against timeouts.

@jmagman
Copy link
Member

jmagman commented May 4, 2020

#5437 was pretty unhelpful in trying to figure out why the cache was added in the first place.
EDIT: actually it was #5345

@jonahwilliams
Copy link
Contributor Author

I added back the poll forever logic

if (views.isNotEmpty || returnEarly) {
return views;
}
await Future<void>.delayed(delay);
Copy link
Member

Choose a reason for hiding this comment

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

This previously had a 'This is taking a long time' message. Should that also be restored, or is this already wrapped in a status spinner thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, status spinner is a good idea. Let me add that instead of adding a print

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jonahwilliams jonahwilliams requested a review from zanderso May 4, 2020 23:02
@jonahwilliams
Copy link
Contributor Author

Status spinner added, tests updated - PTAL

@jonahwilliams
Copy link
Contributor Author

The Linux failure is a flake that I cannot rerun

Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

LGTM, let's see if the attach test goes green.

@jonahwilliams jonahwilliams merged commit 209bdcb into flutter:master May 5, 2020
@jonahwilliams jonahwilliams deleted the remove_flutter_view_cache branch May 5, 2020 17:47
jonahwilliams pushed a commit that referenced this pull request May 5, 2020
jonahwilliams pushed a commit that referenced this pull request May 5, 2020
@jonahwilliams jonahwilliams restored the remove_flutter_view_cache branch May 5, 2020 18:16
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error connecting to the service protocol: No Flutter view is available on "device_name"

4 participants