Skip to content
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.

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

Reland "Add support for Dart Development Service (DDS) in Flutter Tools (#61276)" #62147

Merged
merged 3 commits into from
Jul 29, 2020

Conversation

bkonyi
Copy link
Contributor

@bkonyi bkonyi commented Jul 23, 2020

This reverts commit adc9dde.

Changes for attempt 6 (see diff between commits):

  • Fixed issue where FallbackDiscovery would hold on to a VmService when launching on iOS devices, causing DDS to fail to start
  • Fixed flutter drive case where DDS is already running in another flutter_tools instance

@fluttergithubbot fluttergithubbot added team Infra upgrades, team productivity, code health, technical debt. See also team: labels. tool Affects the "flutter" command-line tool. See also t: labels. labels Jul 23, 2020

// We absolutely must dispose this vmService instance, otherwise
// DDS will fail to start.
vmService.dispose();
Copy link
Member

Choose a reason for hiding this comment

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

nit: don't use "we"

Can this be tested somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if it's worth testing here since it's tested in the SDK, but DDS will throw an exception if another client is already connected to the VM service.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose if this were accidentally removed, the devicelab tests would already be failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the last time I tried to land this after I stopped swallowing the exception I found this issue. The iOS devicelab tests were very unhappy so it'd be hard to miss.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

…` when launching on iOS devices, causing DDS to fail to start

- Fixed `flutter drive` case where DDS is already running in another flutter_tools instance
@bkonyi bkonyi merged commit 3a5a3ea into master Jul 29, 2020
@bkonyi bkonyi deleted the dds_reland_take6 branch July 29, 2020 17:05
Pragya007 pushed a commit to Pragya007/flutter that referenced this pull request Aug 11, 2020
…ls (flutter#61276)" (flutter#62147)

This reverts commit adc9dde.

- Fixed issue where `FallbackDiscovery` would hold on to a `VmService` when launching on iOS devices, causing DDS to fail to start
- Fixed `flutter drive` case where DDS is already running in another flutter_tools instance
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
…ls (flutter#61276)" (flutter#62147)

This reverts commit adc9dde.

- Fixed issue where `FallbackDiscovery` would hold on to a `VmService` when launching on iOS devices, causing DDS to fail to start
- Fixed `flutter drive` case where DDS is already running in another flutter_tools instance
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
team Infra upgrades, team productivity, code health, technical debt. See also team: labels. tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants