- Notifications
You must be signed in to change notification settings - Fork30k
Fix a race in EmbedderTest.CanSpecifyCustomUITaskRunner#182649
Fix a race in EmbedderTest.CanSpecifyCustomUITaskRunner#182649jason-simmons wants to merge 3 commits intoflutter:masterfrom
Conversation
The UI task runner will be destroyed during the call to FlutterEngineShutdown. It should continue forwarding tasks to the engine until the destruction callback is invoked. After that, it should stop using the engine.The test had been using a mutex to guard the engine, but that does not work in this case because both the platform and UI threads must be able to run tasks during engine shutdown. If the test's platform thread holds the mutex during the call to engine.reset(), then the UI task runner can not acquire it to run its tasks.The test previously avoided that by calling engine.reset() without holding the mutex. That creates a race between engine shutdown and the UI task runner's check of engine.is_valid().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Code Review
This pull request addresses a race condition in theCanSpecifyCustomUITaskRunner test. The fix involves using a destruction callback on the custom UI task runner to set a flag, preventing tasks from being run on a shutdown engine. This replaces a mutex-based approach that could lead to deadlocks. TheEmbedderTestTaskRunner has been updated to usestd::function for its destruction callback, improving its flexibility. Additionally, astatic variable in another test was made non-static to improve test isolation.
engine/src/flutter/shell/platform/embedder/tests/embedder_unittests_util.h OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
…tests_util.hCo-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
gaaclarke left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice catch.
Test exempt: this is a test
| auto ui_thread = std::make_unique<fml::Thread>("test_ui_thread"); | ||
| auto ui_task_runner = ui_thread->GetTaskRunner(); | ||
| std::mutex ui_task_runner_mutex; | ||
| bool ui_task_runner_destroyed = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please add the IPLR_GUARDED_BY macro to this variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TheIPLR_ macros are defined in Impeller, but this test is in the engine tree outside Impeller. The engine/shell subsystems are not using the threading annotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's a couple of lines you can inline in this file. It's basically a preprocessor check for clang and an attribute on the variable.
Uh oh!
There was an error while loading.Please reload this page.
| }); | ||
| test_task_runner.SetDestructionCallback( | ||
| [](void* user_data) { destruction_callback_called = true; }); | ||
| [&]() { destruction_callback_called = true; }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The convention at google is to use explicit load() and store() functions with std::atomics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
| task_runner_description_.destruction_callback = [](void* user_data) { | ||
| auto thiz = reinterpret_cast<EmbedderTestTaskRunner*>(user_data); | ||
| if (thiz->destruction_callback_) { | ||
| thiz->destruction_callback_(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this executing on a different thread than SetDestructionCallback? If so there is a race condition and we'll need a mutex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Changed this callback to be immutable after construction
gaaclarke left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM modulo a nit about clang's guarded_by attribute. Thanks jason.
| auto ui_thread = std::make_unique<fml::Thread>("test_ui_thread"); | ||
| auto ui_task_runner = ui_thread->GetTaskRunner(); | ||
| std::mutex ui_task_runner_mutex; | ||
| bool ui_task_runner_destroyed = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's a couple of lines you can inline in this file. It's basically a preprocessor check for clang and an attribute on the variable.
The UI task runner will be destroyed during the call to FlutterEngineShutdown. It should continue forwarding tasks to the engine until the destruction callback is invoked. After that, it should stop using the engine.
The test had been using a mutex to guard the engine, but that does not work in this case because both the platform and UI threads must be able to run tasks during engine shutdown. If the test's platform thread holds the mutex during the call to engine.reset(), then the UI task runner can not acquire it to run its tasks. The test previously avoided that by calling engine.reset() without holding the mutex. That creates a race between engine shutdown and the UI task runner's check of engine.is_valid().