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

flutter attach should not assume that the application dill and local filesystem are in sync #38320

Closed
jonahwilliams opened this issue Aug 12, 2019 · 5 comments
Labels
a: existing-apps Integration with existing apps via the add-to-app flow a: quality A truly polished experience P2 Important issues not at the top of the work list tool Affects the "flutter" command-line tool. See also t: labels.

Comments

@jonahwilliams
Copy link
Member

During flutter run, a dill file (1) contain all sources is built and bundled into the APK/IPA that is installed onto the device. Then, when the tool attaches to the observatory for debugging and hot reload, it initializes the frontend server from the existing dill file and recompiles all sources (2). For here on out, any time a hot reload is requested, the filesystem is stat'd and updated sources are passed into the frontend server. This produces an incremental dill based on the updated sources and the state of (2). By construction (1) and (2) are identical files, so this works as expected.

During flutter attach, an APK/IPA that has already been built in some other tool/process/session is started on a device. This must contain a dill file (1) from some previous compile. We connect to the observatory and initialize the frontend server from an existing dill file (if it exists) and recompile all sources into a new dill file (2). For here on out, any time a hot reload is requested, the filesystem is stat'd and updated sources are passed into the frontend server. Unlike flutter run, there is no guarantee that (1) and (2) were the same file.

Consider the following sequence of user actions:

  1. User edits flutter sources and builds APK, installs on device.
  2. User further edits source code.
  3. User starts app and runs flutter attach.
  4. User requests hot reload, changes from (2) are not observed.

By the time the tool has reached (4), it assumes based on the flutter run workflow that the initial compile from the frontend server represents the source of truth. Therefore the hot reload does not result in any changes, since in this case the stat tells us the file was updated prior to the compile.

Fix:
In the case of flutter attach, the first hot reload should assume that every source file has been invalidated. This will significantly slow down the first reload, but will not effect subsequent reloads.

See also: #37976

cc @zanderso @feinstein

@jonahwilliams jonahwilliams added the tool Affects the "flutter" command-line tool. See also t: labels. label Aug 12, 2019
@zanderso
Copy link
Member

Is it also the case that a sufficiently speedy person (or bot/ide) could modify sources between (1) and (2) during flutter run?

@zanderso zanderso added a: existing-apps Integration with existing apps via the add-to-app flow a: quality A truly polished experience labels Aug 12, 2019
@jonahwilliams
Copy link
Member Author

Yes, that's a good point. I'll confirm locally but changes after the APK/IPA has been built but before the attach are likely to be lost in the run case as well.

@kf6gpe kf6gpe added this to the December 2019 (Add-to-App) milestone Aug 29, 2019
@Hixie Hixie modified the milestones: December 2019 (Add-to-App), Overdue, Goals Jan 7, 2020
@jmagman jmagman added this to Awaiting triage in Add-to-app - iOS tool review via automation Jan 9, 2020
@jmagman jmagman added this to Awaiting triage in Add-to-app - Android tool review via automation Jan 9, 2020
@jmagman jmagman moved this from Awaiting triage to Engineer reviewed in Add-to-app - iOS tool review Jan 10, 2020
@jmagman jmagman moved this from Awaiting triage to Engineer reviewed in Tools - existing app integration review Apr 7, 2020
@kf6gpe kf6gpe added the P2 Important issues not at the top of the work list label May 29, 2020
@Hixie Hixie removed this from the None. milestone Aug 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this issue Dec 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this issue Dec 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this issue Dec 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this issue Dec 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this issue Dec 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this issue Dec 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this issue Dec 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this issue Dec 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this issue Dec 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this issue Dec 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this issue Dec 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this issue Dec 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this issue Dec 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this issue Dec 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this issue Dec 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this issue Dec 16, 2022
auto-submit bot pushed a commit that referenced this issue Dec 16, 2022
…117242)

* e28b26e1d [linux] Allow overriding asset, ICU data path (flutter/engine#38296)

* 35bdb8bfc Roll Skia from 9f728d78f10d to f549128104ba (1 revision) (flutter/engine#38319)

* 97e246cb5 Roll Dart SDK from 358d0d1aa3e7 to 1dd5b1bf1099 (7 revisions) (flutter/engine#38320)

* d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179)

* c9e9fa5a9 Update web_sdk -> package test dependency to get updated package matcher (flutter/engine#38323)

* a7ec07f13 [fuchsia] Manually roll Fuchsia Linux SDK. (flutter/engine#38324)

* 61e95bacb Remove doc reference to the compute method (flutter/engine#38246)

* 353d6949e make sure CanvasRecorder updates clip bounds methods (flutter/engine#38325)

* 47a358c5e Started using FlutterEngineGroups by default on Android  (flutter/engine#37822)

* 7d8e10652 Bump github/codeql-action from 2.1.35 to 2.1.36 (flutter/engine#38210)

* 8915b81d4 Update buildroot to b2ab6e1908b3eb2. (flutter/engine#38329)

* 4fe620643 Revert "Roll Dart SDK from 358d0d1aa3e7 to 1dd5b1bf1099 (7 revisions) (#38320)" (flutter/engine#38331)

* 2ff490c1e Roll Skia from f549128104ba to 5e69caecd166 (11 revisions) (flutter/engine#38333)

* 22251857f Add missing include to FlutterThreadSynchronizer (flutter/engine#38337)

* 467cfd7ef Roll Fuchsia Mac SDK from VEOIaacOA75U7PYyz... to KtItDj-MERuua77aS... (flutter/engine#38339)

* 010f4ee7a Roll Fuchsia Linux SDK from zwfwHRSLdmV61hYqe... to urDNtEiHFAcBBhYe0... (flutter/engine#38340)

* 773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#38327)

* 70439f606 Roll Skia from 5e69caecd166 to 62f22c9c7d67 (3 revisions) (flutter/engine#38341)

* bc1647f0d Roll the test package used by Web in preparation for a Dart 3 SDK roll (flutter/engine#38342)

* cac228aff Roll Dart SDK from 358d0d1aa3e7 to 7b4d4ec3cad1 (14 revisions) (flutter/engine#38344)

* 13ae6eb75 Revert "Started using FlutterEngineGroups by default on Android  (#37822)" (flutter/engine#38351)

* ed8063861 Add an explicit constraint on the matcher package version to ensure Dart 3 compatibility (flutter/engine#38352)

* dcafebf44 Roll Skia from 62f22c9c7d67 to 1b1f53d77ced (1 revision) (flutter/engine#38343)

* 6f6158580 Roll Fuchsia Mac SDK from KtItDj-MERuua77aS... to bn5VF1-xDf-wKjIw8... (flutter/engine#38348)

* 0c00bc0a9 Remove 30fps cap from playgrounds (flutter/engine#38347)

* 38340bb57 [Impeller] Fix SceneC crash for nodes with children (flutter/engine#38346)

* 3a6b3f986 Roll Fuchsia Linux SDK from urDNtEiHFAcBBhYe0... to H6B0UgW07fc1nBtnc... (flutter/engine#38357)

* 81b453535 Roll Skia from 1b1f53d77ced to 7b0a9d9a3008 (8 revisions) (flutter/engine#38358)

* d91e20879 Port touch-based tests from embedder integration test (flutter/engine#38234)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this issue Jan 19, 2023
…lutter#117242)

* e28b26e1d [linux] Allow overriding asset, ICU data path (flutter/engine#38296)

* 35bdb8bfc Roll Skia from 9f728d78f10d to f549128104ba (1 revision) (flutter/engine#38319)

* 97e246cb5 Roll Dart SDK from 358d0d1aa3e7 to 1dd5b1bf1099 (7 revisions) (flutter/engine#38320)

* d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179)

* c9e9fa5a9 Update web_sdk -> package test dependency to get updated package matcher (flutter/engine#38323)

* a7ec07f13 [fuchsia] Manually roll Fuchsia Linux SDK. (flutter/engine#38324)

* 61e95bacb Remove doc reference to the compute method (flutter/engine#38246)

* 353d6949e make sure CanvasRecorder updates clip bounds methods (flutter/engine#38325)

* 47a358c5e Started using FlutterEngineGroups by default on Android  (flutter/engine#37822)

* 7d8e10652 Bump github/codeql-action from 2.1.35 to 2.1.36 (flutter/engine#38210)

* 8915b81d4 Update buildroot to b2ab6e1908b3eb2. (flutter/engine#38329)

* 4fe620643 Revert "Roll Dart SDK from 358d0d1aa3e7 to 1dd5b1bf1099 (7 revisions) (flutter#38320)" (flutter/engine#38331)

* 2ff490c1e Roll Skia from f549128104ba to 5e69caecd166 (11 revisions) (flutter/engine#38333)

* 22251857f Add missing include to FlutterThreadSynchronizer (flutter/engine#38337)

* 467cfd7ef Roll Fuchsia Mac SDK from VEOIaacOA75U7PYyz... to KtItDj-MERuua77aS... (flutter/engine#38339)

* 010f4ee7a Roll Fuchsia Linux SDK from zwfwHRSLdmV61hYqe... to urDNtEiHFAcBBhYe0... (flutter/engine#38340)

* 773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#38327)

* 70439f606 Roll Skia from 5e69caecd166 to 62f22c9c7d67 (3 revisions) (flutter/engine#38341)

* bc1647f0d Roll the test package used by Web in preparation for a Dart 3 SDK roll (flutter/engine#38342)

* cac228aff Roll Dart SDK from 358d0d1aa3e7 to 7b4d4ec3cad1 (14 revisions) (flutter/engine#38344)

* 13ae6eb75 Revert "Started using FlutterEngineGroups by default on Android  (flutter#37822)" (flutter/engine#38351)

* ed8063861 Add an explicit constraint on the matcher package version to ensure Dart 3 compatibility (flutter/engine#38352)

* dcafebf44 Roll Skia from 62f22c9c7d67 to 1b1f53d77ced (1 revision) (flutter/engine#38343)

* 6f6158580 Roll Fuchsia Mac SDK from KtItDj-MERuua77aS... to bn5VF1-xDf-wKjIw8... (flutter/engine#38348)

* 0c00bc0a9 Remove 30fps cap from playgrounds (flutter/engine#38347)

* 38340bb57 [Impeller] Fix SceneC crash for nodes with children (flutter/engine#38346)

* 3a6b3f986 Roll Fuchsia Linux SDK from urDNtEiHFAcBBhYe0... to H6B0UgW07fc1nBtnc... (flutter/engine#38357)

* 81b453535 Roll Skia from 1b1f53d77ced to 7b0a9d9a3008 (8 revisions) (flutter/engine#38358)

* d91e20879 Port touch-based tests from embedder integration test (flutter/engine#38234)
@jonahwilliams
Copy link
Member Author

I'm closing this issue because I don't think this is possible to fix in a way that is generally useful. Performing a hot restart to bring things in sync is not particularly expensive, but doing it every time adds a lot of overhead

@feinstein
Copy link
Contributor

But wouldn't it be needed only once, after flutter attach has started?

@github-actions
Copy link

github-actions bot commented Mar 4, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: existing-apps Integration with existing apps via the add-to-app flow a: quality A truly polished experience P2 Important issues not at the top of the work list tool Affects the "flutter" command-line tool. See also t: labels.
Projects
Add-to-app - iOS tool review
  
Engineer reviewed
Development

Successfully merging a pull request may close this issue.

5 participants