Skip to content

Don't wait on stream listening in DevTools start up #3320

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
elliette merged 1 commit into from
Aug 31, 2021
Merged

Don't wait on stream listening in DevTools start up #3320

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/devtools_app/lib/src/service_manager.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ class ServiceConnectionManager {
serviceStreamName,
];

await Future.wait(streamIds.map((String id) async {
Copy link
Copy Markdown
Contributor

@jacob314 jacob314 Aug 31, 2021

Choose a reason for hiding this comment

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

remove the Future.wait as well. it isn't doing anything useful now that the result is unawaited.
This change could cause some tricky race conditions.
Main race condition: need to ensure that we are listening for isolate events before we fetch the isolate. Otherwise we could incorrectly believe the isolate is paused because we missed the resume event.
@bkonyi I know a lot of the streams now have history. If all the streams where race conditions matter are ones where we can fetch with history then we are safe.

Copy link
Copy Markdown
Member Author

@elliette elliette Aug 31, 2021

Choose a reason for hiding this comment

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

Oh interesting! Unless I'm misunderstanding something, it looks like currently we are fetching the isolate before listening for the isolate events. (await isolateManager.init(isolates) is the call right before this call to start listening to the streams). Might make sense to send off the stream listen requests earlier.

unawaited(Future.wait(streamIds.map((String id) async {
try {
await service.streamListen(id);
} catch (e) {
Expand All @@ -287,7 +287,7 @@ class ServiceConnectionManager {
);
}
}
}));
})));
if (service != this.service) {
// A different service has been opened.
return;
Expand Down