Curious: is there a reason why i++ is less desirable?

in this case, not really. In the general case, ++ suffix can be less efficient (since it has to store the value, then increment it, then use it), so ++ prefix is preferred. But that's ugly. So I pre-emptively turn ++ into the equivalent += 1 to make people more comfortable with += 1 so that the issue of ++ prefix can go away. :-)

@visibleForTesting?

Does it matter in this case?

Should/can this be List<Object> too, since you change dynamic to Object below?

I think the JSON logic creates List<dynamic>s, not List<Object>s.

=> invokeRpcRaw('getVM');

done

I think my intent was not to await here, so refreshViews could continue without blocking. Was there a reason you put await back?

The caller of refreshViews() can choose to not await the returned future. The use of async/await here is semantically equivalent to what you were trying to do with the Completer.

Ok, got it.

yeah these are identical semantically. I happened to have already made this change on my branch; I didn't put the await back so much as have to resolve the merge, and I went with the shorter option. :-)

Similarly, can we avoid awaiting in this method and instead let caller await if it decided it needed to?

The return value of this method will be a future that will complete when all the futures have completed. It's still up to the caller to await the future returned by this method or not.

await future; ...rest of method... is exactly equivalent to return future.then(...rest of method...).

Add a comment explaining that upon completion of this RPC, _viewCache will be updated.

done

Can listViews ever legitimately turn up no FlutterView instances? If so, we'll retry here until we eventually fail, even though there was no failure...

Though I guess that's what waitForViews is for (why why it defaults to false)?

I don't know the answer to your first question, but if so, I don't know how we would distinguish that case from the normal case where we don't want to return until we have a view.

The argument defaults to false so as to not change the default behaviour in the regular case; only in the specific case where I believe we definitely need a view did I set it to true.

No need for async

fixed

This is the change that breaks named_isolates_test.
From what I can tell flutter engine is getting stuck processing two concurrent listViews requests https://github.com/flutter/engine/blob/master/runtime/service_protocol.cc#L237.

cc @jason-simmons @chinmaygarde

To reproduce the failure pick up this new implementation of refreshViews() method and run named_isolates_test locally from $FH/flutter/dev/devicelab: $FH/flutter/bin/cache/dart-sdk/bin/dart bin/run.dart -t named_isolates_test

I am fixing refreshViews so it is app-wide, rather than isolate-specific message, that should fix deadlock in the engine((in 89665ac). @chinmaygarde is making engine more robust to these kind of scenarios.

awesome.