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

Fix potential deadlock in TestMdns #6721

Merged
merged 3 commits into from
May 12, 2021

Conversation

andy31415
Copy link
Contributor

@andy31415 andy31415 commented May 12, 2021

Problem

if testmdns fails (e.g. avahi not running), the test will deadlock forever:

  • wait_for requires that lock is aquired when exiting
  • lock was set for the entire 'run main loop' which never finishes

Summary of Changes

Narrow down the scope of the loops.
Add a start and done condition as separate tasks.
Attempt to cleanly shut down the chip stack on error (this does NOT work, however I was unable to debug as to why ... this test has logic flaws in terms of how the chip main loop is handled)

std::condition_variable is supposed to be able to hold the lock mutex
when exiting after wait_for. If the lock on the mutex is held throughout
the 'RunMainLoop', the condition variable can never exit.

Updated the scope of the locks.
@todo
Copy link

todo bot commented May 12, 2021

the above does not seem to actually reliably shut down the chip

// TODO: the above does not seem to actually reliably shut down the chip
// Program will abort with core because chip thread will still run.
doneCondition.wait_for(lock, std::chrono::seconds(1));
if (!done)
{
fprintf(stderr, "Orderly shutdown of the platform main loop failed as well.\n");
}
retVal = EXIT_FAILURE;
}
}
return retVal;


This comment was generated by todo based on a TODO comment in a88d8ee in #6721. cc @andy31415.

@todo
Copy link

todo bot commented May 12, 2021

the above does not seem to actually reliably shut down the chip stack.

// TODO: the above does not seem to actually reliably shut down the chip stack.
// Program will abort with core because chip thread will still run.
doneCondition.wait_for(lock, std::chrono::seconds(1));
if (!done)
{
fprintf(stderr, "Orderly shutdown of the platform main loop failed as well.\n");
}
retVal = EXIT_FAILURE;
}
}
return retVal;


This comment was generated by todo based on a TODO comment in 5724b4b in #6721. cc @andy31415.

@woody-apple
Copy link
Contributor

@saurabhst?

@woody-apple woody-apple merged commit 6dacc8d into project-chip:master May 12, 2021
@andy31415 andy31415 deleted the fix_locking_in_mdns_test branch October 28, 2021 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants