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

Restyle Fix all detectable thread races in chip-tool #7559

Closed
wants to merge 2 commits into from

Conversation

restyled-io[bot]
Copy link
Contributor

@restyled-io restyled-io bot commented Jun 11, 2021

A duplicate of #7478 with additional commits that automatically address
incorrect style, created by Restyled.

⚠️ Even though this PR is not a Fork, it contains outside contributions.
Please review accordingly.

Since the original Pull Request was opened as a fork in a contributor's
repository, we are unable to create a Pull Request branching from it with only
the style fixes.

The following Restylers made fixes:

To incorporate these changes, you can either:

  1. Merge this Pull Request instead of the original, or

  2. Ask your contributor to locally incorporate these commits and push them to
    the original Pull Request

    Expand for example instructions
    ```console
    git remote add upstream https://github.com/project-chip/connectedhomeip.git
    git fetch upstream pull/<this PR number>/head
    git merge --ff-only FETCH_HEAD
    git push
    ```
    

NOTE: As work continues on the original Pull Request, this process will
re-run and update (force-push) this Pull Request with updated style fixes as
necessary. If the style is fixed manually at any point (i.e. this process finds
no fixes to make), this Pull Request will be closed automatically.

Sorry if this was unexpected. To disable it, see our documentation.

mrjerryjohns and others added 2 commits June 11, 2021 12:09
Problem:

This PR achieves the following to fix-up the various thread-races detected by tsan in chip-tool:

Change:

- Following the pattern of 'external synchronization', sprinkled LockChipStack() and UnlockChipStack() calls around key call sites that called into the stack from the various command logic in chip-tool
- Removed usleep and global instance hacks.
- Reverts changes in #7494
- Re-structured Command::Run to now have the bulk of the stack initialization and shutdown be managed before Run() is called in Commands::Run(), and an ExecutionContext object pointer be stashed inside the Command for convenient access. This reduces the changes of people writing new commands of getting stack initialization wrong.
- Instead of sometimes using chip::Controller::DeviceController and sometimes DeviceCommissioner, just used the latter in all commands since that is the super-set class anyways.
- Added a new 'StopEventLoopTask' that is thread-safe, that is needed to be called by application logic before DeviceController::Shutdown() can be called with external synchronization.
- Pivots PlatformMgr::Shutdown() to not handle stopping the event queue,
  but only focus on cleaning up the stack objects.
- Fixed up TestMdns as well along the way.

Testing:

- Enabled tsan using 'is_tsan' build arg and used that catch well over
  10+ races, with not a single false-positive.
- Ran through all the chip-tool command groups (pairing, IM, discover,
  testcluster, payload, etc) 10x each to ensure no regressions in
  functionality as well as ensuring clean shutdown with tsan.
@restyled-io restyled-io bot added the restyled label Jun 11, 2021
@todo
Copy link

todo bot commented Jun 11, 2021

This doesn't hold true on Darwin, issue #7557 tracks the problem.

// TODO: This doesn't hold true on Darwin, issue #7557 tracks the problem.
//
mController.Shutdown();
return (err == CHIP_NO_ERROR) ? EXIT_SUCCESS : EXIT_FAILURE;
}
CHIP_ERROR Commands::RunCommand(NodeId localId, NodeId remoteId, int argc, char ** argv)
{
CHIP_ERROR err = CHIP_NO_ERROR;
std::map<std::string, CommandsVector>::iterator cluster;


This comment was generated by todo based on a TODO comment in 02baad5 in #7559. cc @project-chip.

@todo
Copy link

todo bot commented Jun 11, 2021

This variable is very similar to mMainLoopIsStarted, track the

// TODO: This variable is very similar to mMainLoopIsStarted, track the
// cleanup and consolidation in this issue:
//
bool mEventQueueHasStopped = false;
pthread_attr_t mChipTaskAttr;
struct sched_param mChipTaskSchedParam;


This comment was generated by todo based on a TODO comment in 02baad5 in #7559. cc @project-chip.

@restyled-io restyled-io bot closed this Jun 11, 2021
@restyled-io restyled-io bot deleted the restyled/pull-7478 branch June 11, 2021 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants