-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Move IM standalone test run on a single event loop #8174
Move IM standalone test run on a single event loop #8174
Conversation
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.
thanks
void Shutdown() | ||
{ | ||
chip::app::InteractionModelEngine::GetInstance()->Shutdown(); | ||
ShutdownChip(); |
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.
This does not look right to me. This will call:
chip::DeviceLayer::PlatformMgr().StopEventLoopTask();
chip::DeviceLayer::PlatformMgr().Shutdown();
from inside RunEventLoop
. The documentation for StopEventLoopTask
says that when used this way it does not guarantee that all work is done until RunEventLoop
returns. So it's not safe to call PlatformMgr().Shutdown
until that point, as the documentation for RunEventLoop
explicitly says.
In fact, it's not safe to shut down the IM engine until that point either.
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 looks like #8100 had similar problems, in addition to possibly calling StopEventLoopTask
when there is no event loop running, which is also not ok.
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.
I updated the logic to only call StopEventLoopTask to trigger RunEventLoop to exit after all work item processing is stopped. And do platform shutdown only after the RunEventLoop returns.
err = SendCommandRequest(commandSender); | ||
SuccessOrExit(err); | ||
|
||
err = chip::DeviceLayer::SystemLayer.StartTimer(gMessageIntervalSeconds * 1000, CommandRequestTimerHandler, NULL); |
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.
So in this new setup each test will definitely run for gMessageIntervalSeconds
instead of completing when the response is received, right? Is that intended?
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.
Yes, it is intended. We always start new test after interval gMessageIntervalSeconds
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.
Alright. Seems like it will make the test run much slower, but maybe that's ok....
@@ -208,6 +208,7 @@ int main(int argc, char * argv[]) | |||
|
|||
chip::app::InteractionModelEngine::GetInstance()->Shutdown(); | |||
|
|||
chip::DeviceLayer::PlatformMgr().StopEventLoopTask(); |
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.
This does not make sense to me. This is after RunEventLoop
has returned, so what does it even mean to call this function here?
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.
Acked, we don't need to explicitly stop event loop after RunEventLoop returned
Size increase report for "esp32-example-build" from 046f46a
Full report output
|
* Move IM standalone test run on a single event loop * Shutdown the platform after RunEventLoop is returned
Problem
What is being fixed? Examples:
This PR is to fix the remaining race issues happens in IM test by running everything within a single event loop.
Currently, Platform Layer provides both API RunEventLoop to run Matter on the app main thread and StartEventLoopTask/StopEventLoopTask to run Matter on a separate thread. For internal testing, we plan to make all test apps single threaded to prevent any race issue occurs.
Fixes Move IM standalone test run inside chip thread. #7939
Change overview
Move IM standalone test run on a single event loop
Testing
How was this tested? (at least one bullet point required)
Setting up cirque environment
git submodule update --init
./scripts/tests/cirque_tests.sh bootstrap
Run IM test for 10 times and confirmed no race issue occurs
./scripts/tests/cirque_tests.sh run_test InteractionModelTest