Fix data races in chip-tool on Darwin #7829
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
andy31415,
chrisdecenzo,
Damian-Nordic,
hawk248,
jepenven-silabs and
msandstedt
bzbarsky-apple
force-pushed
the
fix-chip-tool-races
branch
from
84ba1ba to
92802a6
Compare
woody-apple
approved these changes
Jun 23, 2021
msandstedt
approved these changes
Jun 23, 2021
…l commands. This makes Run() not take any arguments, which makes it easier to queue parts of it to run async.
Except init and shutdown.
bzbarsky-apple
force-pushed
the
fix-chip-tool-races
branch
from
92802a6 to
b87b8fc
Compare
|
@Damian-Nordic @jmartinez-silabs @andy31415 @mspang please take a look. Note that it's easier to review the individual changesets here than the whole thing at once. |
mrjerryjohns
approved these changes
Jun 23, 2021
jmartinez-silabs
approved these changes
Jun 23, 2021
| @@ -36,6 +36,7 @@ class DiscoverCommand : public Command, public chip::Controller::DeviceAddressUp | |||
|
|
|||
| /////////// Command Interface ///////// | |||
| CHIP_ERROR Run() override; | |||
| uint16_t GetWaitDurationInSeconds() const override { return 30; } | |||
There was a problem hiding this comment.
LGTM, Just wondering about the different GetWaitDurationInSeconds values here and in following command interfaces. How were they determined?
There was a problem hiding this comment.
@bzbarsky-apple can you follow up here?
There was a problem hiding this comment.
I kept the existing values, which I suspect were determined by "this should not take longer than, oh I guess this long" or so.
sharadb-amazon
added a commit
to sharadb-amazon/connectedhomeip
that referenced
this pull request
Jun 23, 2021
…HIP Tool commands
nikita-s-wrk
pushed a commit
to nikita-s-wrk/connectedhomeip
that referenced
this pull request
Sep 23, 2021
* Move the local and remote node ids into the exec context for chip-tool commands. This makes Run() not take any arguments, which makes it easier to queue parts of it to run async. * Switch SetCommandExitStatus to take a CHIP_ERROR argument * Regenerate generated code for SetCommandExitStatus changes * Change chip-tool to generally touch the stack only from a queued task. Except init and shutdown.
dmitrymaslovdsr
added a commit
to dmitrymaslovdsr/connectedhomeip
that referenced
this pull request
Apr 16, 2024
…idaiton; remove TimeReference, PeakPeriodStatus, PeakPeriodEndTime attributes validation based on PR project-chip#7829 Signed-off-by: Dmitry Maslov <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
We have some remaining data races in chip-tool on Darwin.
Change overview
Queue the initial command
Runmethod over to the CHIP thread so that all the work apart from initial setup and final teardown of the CHIP stack happens on that thread, avoiding races.Testing
Ran #7797 without this change (fails every time) and with this change (passes every time on Darwin).
Note that this change makes #7797 fail on Linux until #7828 is fixed.
Reviewer note: There are 4 changesets here; reviewing them separately is probably a lot easier than reviewing the whole thing all at once.