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

Engine->Framework roll failing due to hot reload on Linux/Windows #89406

Closed
bdero opened this issue Sep 3, 2021 · 7 comments
Closed

Engine->Framework roll failing due to hot reload on Linux/Windows #89406

bdero opened this issue Sep 3, 2021 · 7 comments
Labels
dependency: dart Dart team may need to help us engine flutter/engine repository. See also e: labels. f: integration_test The flutter/packages/integration_test plugin P0 Critical issues such as a build break or regression

Comments

@bdero
Copy link
Member

bdero commented Sep 3, 2021

Engine roll into Framework failed with (#89401), looks like a hot reload problem for windows and linux:

[LOG]:"[{"id":1,"result":{"code":1,"message":"hot reload failed to complete"}}]"

07:58 +16 ~2 -1: test\integration.shard\background_isolate_test.dart: Hot reload updates background isolates [E]                                                                                       
  Hot reload request failed
  
  [{"id":1,"result":{"code":1,"message":"hot reload failed to complete"}}]
  test\integration.shard\test_driver.dart 747:5             FlutterRunTestDriver._throwErrorResponse
  test\integration.shard\test_driver.dart 662:7             FlutterRunTestDriver._restart
  ===== asynchronous gap ===========================
  test\integration.shard\background_isolate_test.dart 84:5  main.<fn>
  

07:58 +16 ~2 -1: loading test\integration.shard\background_isolate_test.dart                                                                                                                           
Consider enabling the flag chain-stack-traces to receive more detailed exceptions.
For example, 'dart test --chain-stack-traces'.

Roll contains only a Dart SDK bump: https://dart.googlesource.com/sdk.git/+log/3e5ab8e22dc3..edf2936a5e06

Re-running the failed tests to verify that the failure's consistent, but suspecting we'll need to bisect this one.

@bdero bdero added engine flutter/engine repository. See also e: labels. dependency: dart Dart team may need to help us f: integration_test The flutter/packages/integration_test plugin labels Sep 3, 2021
@bdero
Copy link
Member Author

bdero commented Sep 3, 2021

@mkustermann
Copy link
Member

mkustermann commented Sep 3, 2021

@aam If you have time, could you look at this during MTV time today?

I think flutter/engine#26011 was the basic support, but there's still something missing for reload. I suspect it's mostly around removing some code.

If it cannot be fixed quickly in engine, it's also ok to revert for now.

@zanderso zanderso added the P0 Critical issues such as a build break or regression label Sep 3, 2021
@jason-simmons
Copy link
Member

The test passes if I revert dart-lang/sdk@57de39d in a local build.

To reproduce it, patch https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/integration.shard/test_driver.dart#L106 to pass the local engine flags:

    _process = await _processManager.start(
      <String>[flutterBin]
        .followedBy(['--local-engine', 'host_debug_unopt'])
        .followedBy(arguments)
        .toList(),

Then run the test in the framework tree:

cd packages/flutter_tools
flutter test test/integration.shard/background_isolate_test.dart

@bdero
Copy link
Member Author

bdero commented Sep 3, 2021

Revert pending review: https://dart-review.googlesource.com/c/sdk/+/212345

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Sep 3, 2021
This reverts commit 57de39d.

Reason for revert: Breakage in flutter engine roll -- see: flutter/flutter#89406

Original change's description:
> [vm/concurrency] Enable isolate groups by-default in all modes
>
> Various VM embedders have already explicitly opted into it
> AOT mode for a long time.
>
> Now we turn it on by default, also because we'd like to collect
> feedback from the field on this - in good time before the next
> stable branch will be released.
>
> All of our isolate related tests have already
>
>     // VMOptions=--enable-isolate-groups ...
>     // VMOptions=--no-enable-isolate-groups ...
>
> So they will continue to exercise the tests in both modes.
>
> Issue #36097
>
> TEST=Existing test suite.
>
> Change-Id: I1d031e8a5f2173b48eb32c53b08daccda75dc51d
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/208649
> Reviewed-by: Slava Egorov <vegorov@google.com>
> Reviewed-by: Siva Annamalai <asiva@google.com>
> Reviewed-by: Alexander Aprelev <aam@google.com>
> Commit-Queue: Martin Kustermann <kustermann@google.com>

TBR=vegorov@google.com,kustermann@google.com,aam@google.com,asiva@google.com

Change-Id: I4f20f3a998508b2465e5c16b64b363419da62775
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/212345
Auto-Submit: Brandon DeRosier <bdero@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
@aam
Copy link
Member

aam commented Sep 3, 2021

The issue seems to be caused by flutter_tools hot reload code attempting to reload all isolates (at once): and VM not handling it well: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/run_hot.dart#L1175

Generally speaking hot reload request has to be issued only to one isolate in an isolate group, reloading all isolates in the group is redundant. But we also need to tighten VM code to handle it better.

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Sep 3, 2021
loaded_blobs need to be visited as part of IsolateGroup::Visit method rather than Isolate::Visit one.
Before this change loaded_blobs could become corrupted because GC requires objects being visited only once during collection cycles.

Fixes flutter/flutter#89406

TEST=ci, flutter

Change-Id: Ic61498c205c5cc072e5f45928264a507a7752e0d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/212440
Commit-Queue: Alexander Aprelev <aam@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
@bdero
Copy link
Member Author

bdero commented Sep 3, 2021

Engine roll is succeeding again after revert, so closing this one out.

@bdero bdero closed this as completed Sep 3, 2021
@github-actions
Copy link

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 Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependency: dart Dart team may need to help us engine flutter/engine repository. See also e: labels. f: integration_test The flutter/packages/integration_test plugin P0 Critical issues such as a build break or regression
Projects
None yet
Development

No branches or pull requests

5 participants