Fix data races in chip-tool on Darwin

examples/chip-tool/commands/clusters/ModelCommand.cpp

@@ -23,10 +23,6 @@
 
 using namespace ::chip;
 
-namespace {
-constexpr uint16_t kWaitDurationInSeconds = 10;
-} // namespace
-
 void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId,
                                   chip::TLV::TLVReader & aReader, Command * apCommandObj)
 {
@@ -37,29 +33,16 @@ void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aC
         "Default DispatchSingleClusterCommand is called, this should be replaced by actual dispatched for cluster commands");
 }
 
-CHIP_ERROR ModelCommand::Run(NodeId localId, NodeId remoteId)
+CHIP_ERROR ModelCommand::Run()
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
 
-    //
-    // Set this to true first BEFORE we send commands to ensure we don't
-    // end up in a situation where the response comes back faster than we can
-    // set the variable to true, which will cause it to block indefinitely.
-    //
-    UpdateWaitForResponse(true);
-
-    {
-        chip::DeviceLayer::StackLock lock;
-
-        err = GetExecContext()->commissioner->GetConnectedDevice(remoteId, &mOnDeviceConnectedCallback,
-                                                                 &mOnDeviceConnectionFailureCallback);
-        VerifyOrExit(err == CHIP_NO_ERROR,
-                     ChipLogError(chipTool, "Failed in initiating connection to the device: %" PRIu64 ", error %d", remoteId, err));
-    }
-
-    WaitForResponse(kWaitDurationInSeconds);
+    auto * ctx = GetExecContext();
 
-    VerifyOrExit(GetCommandExitStatus(), err = CHIP_ERROR_INTERNAL);
+    err = ctx->commissioner->GetConnectedDevice(ctx->remoteId, &mOnDeviceConnectedCallback, &mOnDeviceConnectionFailureCallback);
+    VerifyOrExit(
+        err == CHIP_NO_ERROR,
+        ChipLogError(chipTool, "Failed in initiating connection to the device: %" PRIu64 ", error %d", ctx->remoteId, err));
 
 exit:
     return err;
@@ -78,5 +61,5 @@ void ModelCommand::OnDeviceConnectionFailureFn(void * context, NodeId deviceId,
     ModelCommand * command = reinterpret_cast<ModelCommand *>(context);
     ChipLogError(chipTool, "Failed in connecting to the device %" PRIu64 ". Error %d", deviceId, error);
     VerifyOrReturn(command != nullptr, ChipLogError(chipTool, "ModelCommand context is null"));
-    command->SetCommandExitStatus(false);
+    command->SetCommandExitStatus(error);
 }

examples/chip-tool/commands/clusters/ModelCommand.h

@@ -39,7 +39,8 @@ class ModelCommand : public Command
     void AddArguments() { AddArgument("endpoint-id", CHIP_ZCL_ENDPOINT_MIN, CHIP_ZCL_ENDPOINT_MAX, &mEndPointId); }
 
     /////////// Command Interface /////////
-    CHIP_ERROR Run(NodeId localId, NodeId remoteId) override;
+    CHIP_ERROR Run() override;
+    uint16_t GetWaitDurationInSeconds() const override { return 10; }
 
     virtual CHIP_ERROR SendCommand(ChipDevice * device, uint8_t endPointId) = 0;
 

examples/chip-tool/commands/common/Command.h

@@ -101,6 +101,8 @@ class Command
         ChipDeviceCommissioner * commissioner;
         chip::Controller::ExampleOperationalCredentialsIssuer * opCredsIssuer;
         PersistentStorage * storage;
+        chip::NodeId localId;
+        chip::NodeId remoteId;
     };
 
     Command(const char * commandName) : mName(commandName) {}
@@ -162,10 +164,23 @@ class Command
         return AddArgument(name, min, max, reinterpret_cast<void *>(out), Number_uint64);
     }
 
-    virtual CHIP_ERROR Run(NodeId localId, NodeId remoteId) = 0;
+    // Will be called in a setting in which it's safe to touch the CHIP
+    // stack. The rules for Run() are as follows:
+    //
+    // 1) If error is returned, Run() must not call SetCommandExitStatus.
+    // 2) If success is returned Run() must either have called
+    //    SetCommandExitStatus() or scheduled async work that will do that.
+    virtual CHIP_ERROR Run() = 0;
 
-    bool GetCommandExitStatus() const { return mCommandExitStatus; }
-    void SetCommandExitStatus(bool status)
+    // Get the wait duration, in seconds, before the command times out.
+    virtual uint16_t GetWaitDurationInSeconds() const = 0;
+
+    // Shut down the command, in case any work needs to be done after the event
+    // loop has been stopped.
+    virtual void Shutdown() {}
+
+    CHIP_ERROR GetCommandExitStatus() const { return mCommandExitStatus; }
+    void SetCommandExitStatus(CHIP_ERROR status)
     {
         mCommandExitStatus = status;
         UpdateWaitForResponse(false);
@@ -183,8 +198,8 @@ class Command
     size_t AddArgument(const char * name, int64_t min, uint64_t max, void * out, ArgumentType type);
     size_t AddArgument(const char * name, int64_t min, uint64_t max, void * out);
 
-    bool mCommandExitStatus = false;
-    const char * mName      = nullptr;
+    CHIP_ERROR mCommandExitStatus = CHIP_ERROR_INTERNAL;
+    const char * mName            = nullptr;
     std::vector<Argument> mArgs;
 
     std::condition_variable cvWaitingForResponse;

examples/chip-tool/commands/common/Commands.cpp

@@ -42,6 +42,7 @@ int Commands::Run(NodeId localId, NodeId remoteId, int argc, char ** argv)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
     chip::Controller::CommissionerInitParams initParams;
+    Command * command = nullptr;
 
     err = chip::Platform::MemoryInit();
     VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init Memory failure: %s", chip::ErrorStr(err)));
@@ -72,14 +73,19 @@ int Commands::Run(NodeId localId, NodeId remoteId, int argc, char ** argv)
     err = mController.ServiceEvents();
     VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Run Loop: %s", chip::ErrorStr(err)));
 
-    err = RunCommand(localId, remoteId, argc, argv);
+    err = RunCommand(localId, remoteId, argc, argv, &command);
     SuccessOrExit(err);
 
 exit:
 #if CONFIG_DEVICE_LAYER
     chip::DeviceLayer::PlatformMgr().StopEventLoopTask();
 #endif
 
+    if (command)
+    {
+        command->Shutdown();
+    }
+
     //
     // We can call DeviceController::Shutdown() safely without grabbing the stack lock
     // since the CHIP thread and event queue have been stopped, preventing any thread
@@ -90,7 +96,7 @@ int Commands::Run(NodeId localId, NodeId remoteId, int argc, char ** argv)
     return (err == CHIP_NO_ERROR) ? EXIT_SUCCESS : EXIT_FAILURE;
 }
 
-CHIP_ERROR Commands::RunCommand(NodeId localId, NodeId remoteId, int argc, char ** argv)
+CHIP_ERROR Commands::RunCommand(NodeId localId, NodeId remoteId, int argc, char ** argv, Command ** ranCommand)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
     std::map<std::string, CommandsVector>::iterator cluster;
@@ -158,10 +164,21 @@ CHIP_ERROR Commands::RunCommand(NodeId localId, NodeId remoteId, int argc, char
         execContext.commissioner  = &mController;
         execContext.opCredsIssuer = &mOpCredsIssuer;
         execContext.storage       = &mStorage;
+        execContext.localId       = localId;
+        execContext.remoteId      = remoteId;
 
         command->SetExecutionContext(execContext);
-
-        err = command->Run(localId, remoteId);
+        *ranCommand = command;
+
+        //
+        // Set this to true first BEFORE we send commands to ensure we don't end
+        // up in a situation where the response comes back faster than we can
+        // set the variable to true, which will cause it to block indefinitely.
+        //
+        command->UpdateWaitForResponse(true);
+        chip::DeviceLayer::PlatformMgr().ScheduleWork(RunQueuedCommand, reinterpret_cast<intptr_t>(command));
+        command->WaitForResponse(command->GetWaitDurationInSeconds());
+        err = command->GetCommandExitStatus();
         if (err != CHIP_NO_ERROR)
         {
             ChipLogError(chipTool, "Run command failure: %s", chip::ErrorStr(err));
@@ -173,6 +190,16 @@ CHIP_ERROR Commands::RunCommand(NodeId localId, NodeId remoteId, int argc, char
     return err;
 }
 
+void Commands::RunQueuedCommand(intptr_t commandArg)
+{
+    auto * command = reinterpret_cast<Command *>(commandArg);
+    CHIP_ERROR err = command->Run();
+    if (err != CHIP_NO_ERROR)
+    {
+        command->SetCommandExitStatus(err);
+    }
+}
+
 std::map<std::string, Commands::CommandsVector>::iterator Commands::GetCluster(std::string clusterName)
 {
     for (auto & cluster : mClusters)

examples/chip-tool/commands/common/Commands.h

@@ -33,7 +33,11 @@ class Commands
     int Run(NodeId localId, NodeId remoteId, int argc, char ** argv);
 
 private:
-    CHIP_ERROR RunCommand(NodeId localId, NodeId remoteId, int argc, char ** argv);
+    // *ranCommand will be set to the command we ran if we get as far as running
+    // it.  If it's not null, we need to call Shutdown() on the command after we
+    // shut down the event loop.
+    CHIP_ERROR RunCommand(NodeId localId, NodeId remoteId, int argc, char ** argv, Command ** ranCommand);
+    static void RunQueuedCommand(intptr_t commandArg);
     std::map<std::string, CommandsVector>::iterator GetCluster(std::string clusterName);
     Command * GetCommand(CommandsVector & commands, std::string commandName);
     Command * GetGlobalCommand(CommandsVector & commands, std::string commandName, std::string attributeName);

examples/chip-tool/commands/discover/Commands.h

@@ -47,13 +47,13 @@ class Resolve : public DiscoverCommand, public chip::Mdns::ResolverDelegate
         nodeData.mAddress.ToString(addrBuffer);
         ChipLogProgress(chipTool, "NodeId Resolution: %" PRIu64 " Address: %s, Port: %" PRIu16, nodeData.mPeerId.GetNodeId(),
                         addrBuffer, nodeData.mPort);
-        SetCommandExitStatus(true);
+        SetCommandExitStatus(CHIP_NO_ERROR);
     }
 
     void OnNodeIdResolutionFailed(const chip::PeerId & peerId, CHIP_ERROR error) override
     {
         ChipLogProgress(chipTool, "NodeId Resolution: failed!");
-        SetCommandExitStatus(false);
+        SetCommandExitStatus(CHIP_ERROR_INTERNAL);
     }
     void OnNodeDiscoveryComplete(const chip::Mdns::DiscoveredNodeData & nodeData) override {}
 };
@@ -82,7 +82,7 @@ class Update : public DiscoverCommand
             ChipLogError(chipTool, "Failed to update the device address: %s", chip::ErrorStr(error));
         }
 
-        SetCommandExitStatus(CHIP_NO_ERROR == error);
+        SetCommandExitStatus(error);
     }
 };
 

examples/chip-tool/commands/discover/DiscoverCommand.cpp

@@ -18,35 +18,8 @@
 
 #include "DiscoverCommand.h"
 
-constexpr uint16_t kWaitDurationInSeconds = 30;
-
-CHIP_ERROR DiscoverCommand::Run(NodeId localId, NodeId remoteId)
+CHIP_ERROR DiscoverCommand::Run()
 {
-    CHIP_ERROR err;
-
-    //
-    // Set this to true first BEFORE we send commands to ensure we don't
-    // end up in a situation where the response comes back faster than we can
-    // set the variable to true, which will cause it to block indefinitely.
-    //
-    UpdateWaitForResponse(true);
-
-    {
-        chip::DeviceLayer::StackLock lock;
-
-        GetExecContext()->commissioner->RegisterDeviceAddressUpdateDelegate(this);
-        err = RunCommand(mNodeId, mFabricId);
-        SuccessOrExit(err);
-    }
-
-    WaitForResponse(kWaitDurationInSeconds);
-
-exit:
-    if (err != CHIP_NO_ERROR)
-    {
-        return err;
-    }
-
-    VerifyOrReturnError(GetCommandExitStatus(), CHIP_ERROR_INTERNAL);
-    return CHIP_NO_ERROR;
+    GetExecContext()->commissioner->RegisterDeviceAddressUpdateDelegate(this);
+    return RunCommand(mNodeId, mFabricId);
 }

examples/chip-tool/commands/discover/DiscoverCommand.h

@@ -35,7 +35,8 @@ class DiscoverCommand : public Command, public chip::Controller::DeviceAddressUp
     void OnAddressUpdateComplete(NodeId nodeId, CHIP_ERROR error) override{};
 
     /////////// Command Interface /////////
-    CHIP_ERROR Run(NodeId localId, NodeId remoteId) override;
+    CHIP_ERROR Run() override;
+    uint16_t GetWaitDurationInSeconds() const override { return 30; }
 
     virtual CHIP_ERROR RunCommand(NodeId remoteId, uint64_t fabricId) = 0;
 

examples/chip-tool/commands/discover/DiscoverCommissionersCommand.cpp

@@ -21,25 +21,13 @@
 
 using namespace ::chip;
 
-constexpr uint16_t kWaitDurationInSeconds = 3;
-
-CHIP_ERROR DiscoverCommissionersCommand::Run(NodeId localId, NodeId remoteId)
+CHIP_ERROR DiscoverCommissionersCommand::Run()
 {
-    //
-    // Set this to true first BEFORE we send commands to ensure we don't
-    // end up in a situation where the response comes back faster than we can
-    // set the variable to true, which will cause it to block indefinitely.
-    //
-    UpdateWaitForResponse(true);
-
-    {
-        chip::DeviceLayer::StackLock lock;
-
-        ReturnErrorOnFailure(mCommissionableNodeController.DiscoverCommissioners());
-    }
-
-    WaitForResponse(kWaitDurationInSeconds);
+    return mCommissionableNodeController.DiscoverCommissioners();
+}
 
+void DiscoverCommissionersCommand::Shutdown()
+{
     int commissionerCount = 0;
     for (int i = 0; i < CHIP_DEVICE_CONFIG_MAX_DISCOVERED_NODES; i++)
     {
@@ -86,6 +74,5 @@ CHIP_ERROR DiscoverCommissionersCommand::Run(NodeId localId, NodeId remoteId)
         }
     }
 
-    printf("Total of %d commissioner(s) discovered in %d sec\n", commissionerCount, kWaitDurationInSeconds);
-    return CHIP_NO_ERROR;
+    printf("Total of %d commissioner(s) discovered in %" PRIu16 " sec\n", commissionerCount, GetWaitDurationInSeconds());
 }

examples/chip-tool/commands/discover/DiscoverCommissionersCommand.h

@@ -25,7 +25,9 @@ class DiscoverCommissionersCommand : public Command
 {
 public:
     DiscoverCommissionersCommand() : Command("discover-commissioners") {}
-    CHIP_ERROR Run(NodeId localId, NodeId remoteId) override;
+    CHIP_ERROR Run() override;
+    uint16_t GetWaitDurationInSeconds() const override { return 3; }
+    void Shutdown() override;
 
 private:
     chip::Controller::CommissionableNodeController mCommissionableNodeController;