[tool] Guard process writes to frontend server in `ResidentCompiler` #152358

andrewkolos · 2024-07-26T05:31:19Z

Contributes to fixing #137184.
Cleaned up version of earlier PR, #152187.

This PR guards all the writes to Process::stdin by wrapping them with ProcessUtils.writelnToStdinUnsafe. This way, if any writes fail, we should at least get a stacktrace in our crash reporting.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

andrewkolos · 2024-07-30T21:01:16Z

packages/flutter_tools/lib/src/compile.dart

+  // TODO(andrewkolos): Concurrent calls to ProcessUtils.writelnToStdinUnsafe
+  //  against the same stdin will result in an exception. To guard against this,
+  //  we need to force calls to run serially. Ideally, this wouldn't be
+  //  necessary since we shouldn't have multiple concurrent writes to the
+  //  compiler process. However, we do (https://github.com/flutter/flutter/issues/152577).


I'm okay with not shaving this yak now, because 1) event loop turns and stdin flushes are pretty fast, so the risk of it changing any existing races probably isn't high and 2) this is (potentially) blocking us getting stack traces on the top crasher of the flutter tool, which I want to resolve ASAP.

Side note: maybe instead of managing a pool and having ProcessUtils.writelnToStdinUnsafe and friends, we should create an abstraction (say, ProcessStdinWriter) that manages this stuff for us. While we are at it, maybe we should just replace Process from dart:io with our own wrapper entirely 🙃.

This is needed because the caller might call testTimeRecorder.print before this method got a chance to call testTimeRecorder.stop

christopherfujino · 2024-08-05T21:26:19Z

packages/flutter_tools/lib/src/compile.dart

+    final PoolResource request = await _serverStdinWritePool.request();
+    try {
+      for (final String line in lines) {
+        await ProcessUtils.writelnToStdinUnsafe(stdin: server.stdin, line: line);


i worry that we're adding a lot of flush calls and asynchronous suspensions here. Would it work if we do:

await ProcessUtils.writelnToStdinUnsafe(stdin: server.stdin, line: lines.join('\n'));

Effectively batching the writelns before a single flush call?

Hmm, a hot reload test timed out:

24:00 +58 -1: test/integration.shard/hot_reload_test.dart: hot restart works without error [E] TimeoutException after 0:15:00.000000: Test timed out after 15 minutes. dart:isolate _RawReceivePort._handleMessage

Did the batching break the compiler?

This may be a manifestation of #152220 (or something with the same root cause), but now that you point it out...let me see if there's anything in the logs that can support this.

I think this is different. I find this from the logs interesting:

<=stdout= [{"event":"app.stop","params":{"appId":"0b7edf75-f1fe-442c-ad22-155775e36df2"}}] Exception: Received app.stop event while waiting for response to request 1

for reference:

Performing hot restart... =stdin=> [{"id":1,"method":"app.restart","params":{"appId":"0b7edf75-f1fe-442c-ad22-155775e36df2","fullRestart":true,"pause":false,"debounce":false,"debounceDurationOverrideMs":null}}]

So the tool never gets a response from the VM service for the app.restart call? Unfortunately it looks like the last message from the VM service is truncated in the logs.

christopherfujino

LGTM

flutteractionsbot · 2024-08-07T03:47:29Z

Failed to create CP due to merge conflicts.
You will need to create the PR manually. See the cherrypick wiki for more info.

…mpiler` (flutter/flutter#152358)

Manual roll requested by [email protected] flutter/flutter@1dd7141...0a7f8af 2024-08-06 [email protected] Support clearing selection programmatically through SelectableRegionState (flutter/flutter#152882) 2024-08-06 [email protected] Roll Flutter Engine from aabd582e9c7e to 206e86ee8a40 (4 revisions) (flutter/flutter#152962) 2024-08-06 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.3.4 to 4.3.6 (flutter/flutter#152964) 2024-08-06 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.15 to 3.26.0 (flutter/flutter#152965) 2024-08-06 [email protected] [tool] Guard process writes to frontend server in `ResidentCompiler` (flutter/flutter#152358) 2024-08-06 [email protected] Fix Linux_android_emu tests late initialization errors (flutter/flutter#152932) 2024-08-06 [email protected] Marks Mac integration_ui_test_test_macos to be flaky (flutter/flutter#152213) 2024-08-06 [email protected] Roll pub packages (flutter/flutter#152956) 2024-08-06 [email protected] [API Examples] `scroll_direction.0_test.dart` & `growth_direction.0_test.dart` (flutter/flutter#152941) 2024-08-06 [email protected] [devicelab] opt gallery benchmarks and platform view test into merged thread mode. (flutter/flutter#152940) 2024-08-06 [email protected] Roll Flutter Engine from e97955bf12ac to aabd582e9c7e (1 revision) (flutter/flutter#152948) 2024-08-06 [email protected] Roll pub packages (flutter/flutter#152945) 2024-08-06 [email protected] [material/menu_anchor.dart] MenuAnchor focus refactoring for RawMenuAnchor (flutter/flutter#150950) 2024-08-06 [email protected] Roll Flutter Engine from 99da2cab3f6d to e97955bf12ac (2 revisions) (flutter/flutter#152944) 2024-08-06 [email protected] fix: add parameter to maintainState of SearchDelegate (flutter/flutter#152444) 2024-08-06 [email protected] Feat: Add fillColor property for cupertinoCheckbox (flutter/flutter#151761) 2024-08-06 [email protected] Slider does not show changed label value for keyboard users fix (flutter/flutter#152886) 2024-08-06 [email protected] Add Nate Wilson to authors (flutter/flutter#152907) 2024-08-06 [email protected] Roll Flutter Engine from 7fc38bef4775 to 99da2cab3f6d (1 revision) (flutter/flutter#152934) 2024-08-06 [email protected] Roll Packages from 82e8d1e to 551bde5 (12 revisions) (flutter/flutter#152930) 2024-08-06 [email protected] Roll Flutter Engine from 84116159ebfc to 7fc38bef4775 (1 revision) (flutter/flutter#152924) 2024-08-06 [email protected] Roll Flutter Engine from f75ffb65cfe3 to 84116159ebfc (1 revision) (flutter/flutter#152917) 2024-08-06 [email protected] Roll Flutter Engine from f5f6e966e7e7 to f75ffb65cfe3 (2 revisions) (flutter/flutter#152915) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

zanderso · 2024-08-07T15:55:02Z

packages/flutter_tools/lib/src/devfs.dart

+      // TODO(andrewkolos): This is a hack. Adding any more awaits to the compiler's
+      //  recompile method will cause this to be insufficent.
+      //  https://github.com/flutter/flutter/issues/151255.
+      await null;


There is some shady looking stuff going on in this PR. I am skeptical about cp'ing this to stable.

This was an extension of an existing hack (there was already one await null here); but, yes, I agree there are definitely a few unshaven yaks when it comes to Dart (re)-compilation and/or hot-reloading in the tool.

While not CP'ing this change could delay getting stack traces on #137184 (easily the top crasher in the tool), I certainly understand not wanting to CP this.

P.S. I saw #153026. Yes, hot reload has been flaky for a while. I'm doing some archeology to put a summary together here. However, if hot reload has become especially flaky (well over 2%) since this PR landed, I'm okay with reverting.

I think we should do a speculative revert, yeah. I will add the tag.

zanderso · 2024-08-07T16:09:11Z

Reason for revert: Speculative revert to determine whether this PR is related to #153026

…mpiler` (#152358)" This reverts commit d848262.

…ompiler` (#152358)" (#153028) Reverts: #152358 Initiated by: zanderso Reason for reverting: Speculative revert to determine whether this PR is related to #153026 Original PR Author: andrewkolos Reviewed By: {christopherfujino} This change reverts the following previous change: Contributes to fixing #137184. Cleaned up version of earlier PR, #152187. This PR guards all the writes to `Process::stdin` by wrapping them with `ProcessUtils.writelnToStdinUnsafe`. This way, if any writes fail, we should at least get a stacktrace in our crash reporting.

…lutter#152358) Contributes to fixing flutter#137184. Cleaned up version of earlier PR, flutter#152187. This PR guards all the writes to `Process::stdin` by wrapping them with `ProcessUtils.writelnToStdinUnsafe`. This way, if any writes fail, we should at least get a stacktrace in our crash reporting.

…ompiler` (flutter#152358)" (flutter#153028) Reverts: flutter#152358 Initiated by: zanderso Reason for reverting: Speculative revert to determine whether this PR is related to flutter#153026 Original PR Author: andrewkolos Reviewed By: {christopherfujino} This change reverts the following previous change: Contributes to fixing flutter#137184. Cleaned up version of earlier PR, flutter#152187. This PR guards all the writes to `Process::stdin` by wrapping them with `ProcessUtils.writelnToStdinUnsafe`. This way, if any writes fail, we should at least get a stacktrace in our crash reporting.

…lutter#152358) Contributes to fixing flutter#137184. Cleaned up version of earlier PR, flutter#152187. This PR guards all the writes to `Process::stdin` by wrapping them with `ProcessUtils.writelnToStdinUnsafe`. This way, if any writes fail, we should at least get a stacktrace in our crash reporting.

…ompiler` (flutter#152358)" (flutter#153028) Reverts: flutter#152358 Initiated by: zanderso Reason for reverting: Speculative revert to determine whether this PR is related to flutter#153026 Original PR Author: andrewkolos Reviewed By: {christopherfujino} This change reverts the following previous change: Contributes to fixing flutter#137184. Cleaned up version of earlier PR, flutter#152187. This PR guards all the writes to `Process::stdin` by wrapping them with `ProcessUtils.writelnToStdinUnsafe`. This way, if any writes fail, we should at least get a stacktrace in our crash reporting.

…mpiler` (flutter/flutter#152358)

andrewkolos added the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 26, 2024

andrewkolos force-pushed the guard-process-writes-in-compile branch 3 times, most recently from f82a07e to 7e24822 Compare July 26, 2024 20:45

andrewkolos mentioned this pull request Jul 30, 2024

[tool] Tool can accept/reject compilation results from a compilation after already sending out a second compilation request #152577

Closed

andrewkolos force-pushed the guard-process-writes-in-compile branch from a5d97f0 to e4adba7 Compare July 30, 2024 20:38

andrewkolos commented Jul 30, 2024

View reviewed changes

andrewkolos marked this pull request as ready for review July 31, 2024 17:41

andrewkolos added 6 commits July 31, 2024 11:14

init

d114fd3

accept compiler result before notifying caller

d1b5b43

This is needed because the caller might call testTimeRecorder.print before this method got a chance to call testTimeRecorder.stop

add more await null's to DevFS::update 🙃

7550621

use pool

b6c6c38

add TODO about serializing writes

648567c

tweak doc strings

e5273a8

andrewkolos force-pushed the guard-process-writes-in-compile branch from ee0acd1 to e5273a8 Compare July 31, 2024 18:14

andrewkolos requested a review from christopherfujino August 5, 2024 17:47

christopherfujino reviewed Aug 5, 2024

View reviewed changes

andrewkolos added 2 commits August 5, 2024 14:31

experiment: avoid asynchronous suspensions where convenient

27d8c3c

Merge branch 'master' into guard-process-writes-in-compile

6684a01

christopherfujino approved these changes Aug 6, 2024

View reviewed changes

andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 6, 2024

auto-submit bot merged commit d848262 into flutter:master Aug 6, 2024

andrewkolos added the cp: stable cherry pick this pull request to stable release candidate branch label Aug 7, 2024

engine-flutter-autoroll mentioned this pull request Aug 7, 2024

Manual roll Flutter from 1dd71413f4ad to 0a7f8af6d146 (23 revisions) flutter/packages#7328

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 7, 2024

d848262 [tool] Guard process writes to frontend server in `ResidentCo…

f70fae1

…mpiler` (flutter/flutter#152358)

zanderso reviewed Aug 7, 2024

View reviewed changes

zanderso mentioned this pull request Aug 7, 2024

Migrate the frontend server to a persistent compilation daemon with an easier to maintain protocol #128608

Open

zanderso added the revert Autorevert PR (with "Reason for revert:" comment) label Aug 7, 2024

auto-submit bot pushed a commit that referenced this pull request Aug 7, 2024

Revert "[tool] Guard process writes to frontend server in `ResidentCo…

16442d7

…mpiler` (#152358)" This reverts commit d848262.

auto-submit bot mentioned this pull request Aug 7, 2024

Reverts "[tool] Guard process writes to frontend server in ResidentCompiler (#152358)" #153028

Merged

9 tasks

auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Aug 7, 2024

andrewkolos mentioned this pull request Aug 8, 2024

☂️ Hot restart integration tests are quite flaky #153049

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2024

d848262 [tool] Guard process writes to frontend server in `ResidentCo…

a66de65

…mpiler` (flutter/flutter#152358)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 12, 2024

d848262 [tool] Guard process writes to frontend server in `ResidentCo…

a7df908

…mpiler` (flutter/flutter#152358)

[tool] Guard process writes to frontend server in ResidentCompiler #152358

[tool] Guard process writes to frontend server in ResidentCompiler #152358

Uh oh!

Conversation

andrewkolos commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

andrewkolos Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

andrewkolos Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

christopherfujino Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

andrewkolos Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

andrewkolos Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

flutteractionsbot commented Aug 7, 2024

Uh oh!

zanderso Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

andrewkolos Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanderso Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

zanderso commented Aug 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[tool] Guard process writes to frontend server in `ResidentCompiler` #152358

[tool] Guard process writes to frontend server in `ResidentCompiler` #152358

andrewkolos commented Jul 26, 2024 •

edited

Loading

andrewkolos Jul 30, 2024 •

edited

Loading

andrewkolos Aug 7, 2024 •

edited

Loading