Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@jason-simmons
Copy link
Member

The Impeller ContextVK contains a ConcurrentMessageLoop whose threads may invoke Dart timeline APIs. The Dart APIs will create a thread-local object that will be deleted during thread shutdown. Therefore, these threads should not outlive the engine/Shell and Dart VM.

Previously, RunTester held the ImpellerVulkanContextHolder on the stack, and its reference to the ContextVK would be dropped while exiting the function after the Shell is destructed.

This PR moves the contents of the tester's ImpellerVulkanContextHolder out of the instance on the stack and into a lambda owned by the Shell.

It also reenables the flutter_tester Impeller tests in the run_tests script.

…ller Vulkan context during shutdown

The Impeller ContextVK contains a ConcurrentMessageLoop whose threads may
invoke Dart timeline APIs.  The Dart APIs will create a thread-local object
that will be deleted during thread shutdown.  Therefore, these threads
should not outlive the engine/Shell and Dart VM.

Previously, RunTester held the ImpellerVulkanContextHolder on the stack,
and its reference to the ContextVK would be dropped while exiting the
function after the Shell is destructed.

This PR moves the contents of the tester's ImpellerVulkanContextHolder
out of the instance on the stack and into a lambda owned by the Shell.

It also reenables the flutter_tester Impeller tests in the run_tests script.
@jason-simmons jason-simmons requested a review from dnfield October 13, 2023 00:15
Copy link
Contributor

@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.

RSLGTM

@jason-simmons jason-simmons added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 16, 2023
@auto-submit auto-submit bot merged commit 6cc678c into flutter:main Oct 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 16, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Oct 16, 2023
flutter/engine@505c720...6cc678c

2023-10-16 jason-simmons@users.noreply.github.com Avoid a deadlock in the flutter_tester process when deleting the Impeller Vulkan context during shutdown (flutter/engine#46860)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jsimmons@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: 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
harryterkelsen pushed a commit that referenced this pull request Oct 23, 2023
…ller Vulkan context during shutdown (#46860)

The Impeller ContextVK contains a ConcurrentMessageLoop whose threads may invoke Dart timeline APIs.  The Dart APIs will create a thread-local object that will be deleted during thread shutdown.  Therefore, these threads should not outlive the engine/Shell and Dart VM.

Previously, RunTester held the ImpellerVulkanContextHolder on the stack, and its reference to the ContextVK would be dropped while exiting the function after the Shell is destructed.

This PR moves the contents of the tester's ImpellerVulkanContextHolder out of the instance on the stack and into a lambda owned by the Shell.

It also reenables the flutter_tester Impeller tests in the run_tests script.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants