Skip to content

Commit

Permalink
Fix multiple subscriptions to the same attribute in chip-tool. (#18758)
Browse files Browse the repository at this point in the history 
Because the command object is the same for subscriptions to the same attribute, we were reusing the same mSubscribeClient unique_ptr for the new subscription, which killed off the old one.

The changes here are as follows:

1) Keep track of a list of subscribe clients in InteractionModelReports.
2) To fix a (pre-existing) issue where we would crash on exit from
   interactive mode if we had alive subscription, introduce a Cleanup
   function on CHIPCommand which is called either immediately after
   Shutdown or when we exit interactive mode, depending on what the
   command wants.
3) Rearrange the attribute/event read/report commands to make sure we
   handle cleanup correctly and don't leak ReadClients (even
   temporarily, in the error cases).

Fixes #18245
  • Loading branch information
@bzbarsky-apple @pull
bzbarsky-apple authored and pull[bot] committed Jul 8, 2022
1 parent 65a82df commit 1611900
Show file tree
Hide file tree
Showing 6 changed files with 153 additions and 94 deletions.
165 changes: 91 additions & 74 deletions examples/chip-tool/commands/clusters/ReportCommand.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ class ReportCommand : public InteractionModelReports, public ModelCommand, publi
InteractionModelReports(this), ModelCommand(commandName, credsIssuerConfig, /* supportsMultipleEndpoints = */ true)
{}

virtual void OnSubscription(){};

/////////// ReadClient Callback Interface /////////
void OnAttributeData(const chip::app::ConcreteDataAttributePath & path, chip::TLV::TLVReader * data,
const chip::app::StatusIB & status) override
Expand Down Expand Up @@ -96,13 +94,14 @@ class ReportCommand : public InteractionModelReports, public ModelCommand, publi
mError = error;
}

void OnDone(chip::app::ReadClient *) override
void Shutdown() override
{
InteractionModelReports::Shutdown();
SetCommandExitStatus(mError);
// We don't shut down InteractionModelReports here; we leave it for
// Cleanup to handle.
ModelCommand::Shutdown();
}

void OnSubscriptionEstablished(chip::SubscriptionId subscriptionId) override { OnSubscription(); }
void Cleanup() override { InteractionModelReports::Shutdown(); }

protected:
// Use a 3x-longer-than-default timeout because wildcard reads can take a
Expand All @@ -115,42 +114,94 @@ class ReportCommand : public InteractionModelReports, public ModelCommand, publi
CHIP_ERROR mError = CHIP_NO_ERROR;
};

class ReadAttribute : public ReportCommand
class ReadCommand : public ReportCommand
{
protected:
ReadCommand(const char * commandName, CredentialIssuerCommands * credsIssuerConfig) :
ReportCommand(commandName, credsIssuerConfig)
{}

void OnDone(chip::app::ReadClient * aReadClient) override
{
InteractionModelReports::CleanupReadClient(aReadClient);
SetCommandExitStatus(mError);
}
};

class SubscribeCommand : public ReportCommand
{
protected:
SubscribeCommand(const char * commandName, CredentialIssuerCommands * credsIssuerConfig) :
ReportCommand(commandName, credsIssuerConfig)
{}

void OnSubscriptionEstablished(chip::SubscriptionId subscriptionId) override
{
mSubscriptionEstablished = true;
SetCommandExitStatus(CHIP_NO_ERROR);
}

void OnDone(chip::app::ReadClient * aReadClient) override
{
if (!mSubscriptionEstablished)
{
InteractionModelReports::CleanupReadClient(aReadClient);
SetCommandExitStatus(mError);
}

// else we must be getting here from Cleanup(), which means we have
// already done our exit status thing, and have done the ReadClient
// cleanup.
}

void Shutdown() override
{
mSubscriptionEstablished = false;
ReportCommand::Shutdown();
}

bool DeferInteractiveCleanup() override { return mSubscriptionEstablished; }

private:
bool mSubscriptionEstablished = false;
};

class ReadAttribute : public ReadCommand
{
public:
ReadAttribute(CredentialIssuerCommands * credsIssuerConfig) : ReportCommand("read-by-id", credsIssuerConfig)
ReadAttribute(CredentialIssuerCommands * credsIssuerConfig) : ReadCommand("read-by-id", credsIssuerConfig)
{
AddArgument("cluster-ids", 0, UINT32_MAX, &mClusterIds,
"Comma-separated list of cluster ids to read from (e.g. \"6\" or \"8,0x201\").\n Allowed to be 0xFFFFFFFF to "
"indicate a wildcard cluster.");
AddAttributeIdArgument();
AddCommonArguments();
ReportCommand::AddArguments();
ReadCommand::AddArguments();
}

ReadAttribute(chip::ClusterId clusterId, CredentialIssuerCommands * credsIssuerConfig) :
ReportCommand("read-by-id", credsIssuerConfig), mClusterIds(1, clusterId)
ReadCommand("read-by-id", credsIssuerConfig), mClusterIds(1, clusterId)
{
AddAttributeIdArgument();
AddCommonArguments();
ReportCommand::AddArguments();
ReadCommand::AddArguments();
}

ReadAttribute(chip::ClusterId clusterId, const char * attributeName, chip::AttributeId attributeId,
CredentialIssuerCommands * credsIssuerConfig) :
ReportCommand("read", credsIssuerConfig),
ReadCommand("read", credsIssuerConfig),
mClusterIds(1, clusterId), mAttributeIds(1, attributeId)
{
AddArgument("attr-name", attributeName);
AddCommonArguments();
ReportCommand::AddArguments();
ReadCommand::AddArguments();
}

~ReadAttribute() {}

CHIP_ERROR SendCommand(chip::DeviceProxy * device, std::vector<chip::EndpointId> endpointIds) override
{
return ReportCommand::ReadAttribute(device, endpointIds, mClusterIds, mAttributeIds, mFabricFiltered, mDataVersion);
return ReadCommand::ReadAttribute(device, endpointIds, mClusterIds, mAttributeIds, mFabricFiltered, mDataVersion);
}

private:
Expand All @@ -175,60 +226,43 @@ class ReadAttribute : public ReportCommand
chip::Optional<std::vector<chip::DataVersion>> mDataVersion;
};

class SubscribeAttribute : public ReportCommand
class SubscribeAttribute : public SubscribeCommand
{
public:
SubscribeAttribute(CredentialIssuerCommands * credsIssuerConfig) : ReportCommand("subscribe-by-id", credsIssuerConfig)
SubscribeAttribute(CredentialIssuerCommands * credsIssuerConfig) : SubscribeCommand("subscribe-by-id", credsIssuerConfig)
{
AddArgument("cluster-ids", 0, UINT32_MAX, &mClusterIds,
"Comma-separated list of cluster ids to subscribe to (e.g. \"6\" or \"8,0x201\").\n Allowed to be 0xFFFFFFFF "
"to indicate a wildcard cluster.");
AddAttributeIdArgument();
AddCommonArguments();
ReportCommand::AddArguments();
SubscribeCommand::AddArguments();
}

SubscribeAttribute(chip::ClusterId clusterId, CredentialIssuerCommands * credsIssuerConfig) :
ReportCommand("subscribe-by-id", credsIssuerConfig), mClusterIds(1, clusterId)
SubscribeCommand("subscribe-by-id", credsIssuerConfig), mClusterIds(1, clusterId)
{
AddAttributeIdArgument();
AddCommonArguments();
ReportCommand::AddArguments();
SubscribeCommand::AddArguments();
}

SubscribeAttribute(chip::ClusterId clusterId, const char * attributeName, chip::AttributeId attributeId,
CredentialIssuerCommands * credsIssuerConfig) :
ReportCommand("subscribe", credsIssuerConfig),
SubscribeCommand("subscribe", credsIssuerConfig),
mClusterIds(1, clusterId), mAttributeIds(1, attributeId)
{
AddArgument("attr-name", attributeName);
AddCommonArguments();
ReportCommand::AddArguments();
SubscribeCommand::AddArguments();
}

~SubscribeAttribute() {}

CHIP_ERROR SendCommand(chip::DeviceProxy * device, std::vector<chip::EndpointId> endpointIds) override
{
return ReportCommand::SubscribeAttribute(device, endpointIds, mClusterIds, mAttributeIds, mMinInterval, mMaxInterval,
mFabricFiltered, mDataVersion, mKeepSubscriptions);
}

void OnSubscription() override
{
// The ReadClient instance can not be released directly into the OnSubscription
// callback since it happens to be called by ReadClient itself which is doing additional
// work after that.
chip::DeviceLayer::PlatformMgr().ScheduleWork(
[](intptr_t arg) {
auto * command = reinterpret_cast<SubscribeAttribute *>(arg);
if (!command->IsInteractive())
{
command->InteractionModelReports::Shutdown();
}
command->SetCommandExitStatus(CHIP_NO_ERROR);
},
reinterpret_cast<intptr_t>(this));
return SubscribeCommand::SubscribeAttribute(device, endpointIds, mClusterIds, mAttributeIds, mMinInterval, mMaxInterval,
mFabricFiltered, mDataVersion, mKeepSubscriptions);
}

private:
Expand Down Expand Up @@ -263,43 +297,43 @@ class SubscribeAttribute : public ReportCommand
chip::Optional<bool> mKeepSubscriptions;
};

class ReadEvent : public ReportCommand
class ReadEvent : public ReadCommand
{
public:
ReadEvent(CredentialIssuerCommands * credsIssuerConfig) : ReportCommand("read-event-by-id", credsIssuerConfig)
ReadEvent(CredentialIssuerCommands * credsIssuerConfig) : ReadCommand("read-event-by-id", credsIssuerConfig)
{
AddArgument("cluster-id", 0, UINT32_MAX, &mClusterIds);
AddArgument("event-id", 0, UINT32_MAX, &mEventIds);
AddArgument("fabric-filtered", 0, 1, &mFabricFiltered);
AddArgument("event-min", 0, UINT64_MAX, &mEventNumber);
ReportCommand::AddArguments();
ReadCommand::AddArguments();
}

ReadEvent(chip::ClusterId clusterId, CredentialIssuerCommands * credsIssuerConfig) :
ReportCommand("read-event-by-id", credsIssuerConfig), mClusterIds(1, clusterId)
ReadCommand("read-event-by-id", credsIssuerConfig), mClusterIds(1, clusterId)
{
AddArgument("event-id", 0, UINT32_MAX, &mEventIds);
AddArgument("fabric-filtered", 0, 1, &mFabricFiltered);
AddArgument("event-min", 0, UINT64_MAX, &mEventNumber);
ReportCommand::AddArguments();
ReadCommand::AddArguments();
}

ReadEvent(chip::ClusterId clusterId, const char * eventName, chip::EventId eventId,
CredentialIssuerCommands * credsIssuerConfig) :
ReportCommand("read-event", credsIssuerConfig),
ReadCommand("read-event", credsIssuerConfig),
mClusterIds(1, clusterId), mEventIds(1, eventId)
{
AddArgument("event-name", eventName);
AddArgument("fabric-filtered", 0, 1, &mFabricFiltered);
AddArgument("event-min", 0, UINT64_MAX, &mEventNumber);
ReportCommand::AddArguments();
ReadCommand::AddArguments();
}

~ReadEvent() {}

CHIP_ERROR SendCommand(chip::DeviceProxy * device, std::vector<chip::EndpointId> endpointIds) override
{
return ReportCommand::ReadEvent(device, endpointIds, mClusterIds, mEventIds, mFabricFiltered, mEventNumber);
return ReadCommand::ReadEvent(device, endpointIds, mClusterIds, mEventIds, mFabricFiltered, mEventNumber);
}

private:
Expand All @@ -309,10 +343,10 @@ class ReadEvent : public ReportCommand
chip::Optional<chip::EventNumber> mEventNumber;
};

class SubscribeEvent : public ReportCommand
class SubscribeEvent : public SubscribeCommand
{
public:
SubscribeEvent(CredentialIssuerCommands * credsIssuerConfig) : ReportCommand("subscribe-event-by-id", credsIssuerConfig)
SubscribeEvent(CredentialIssuerCommands * credsIssuerConfig) : SubscribeCommand("subscribe-event-by-id", credsIssuerConfig)
{
AddArgument("cluster-id", 0, UINT32_MAX, &mClusterIds);
AddArgument("event-id", 0, UINT32_MAX, &mEventIds);
Expand All @@ -321,24 +355,24 @@ class SubscribeEvent : public ReportCommand
AddArgument("fabric-filtered", 0, 1, &mFabricFiltered);
AddArgument("event-min", 0, UINT64_MAX, &mEventNumber);
AddArgument("keepSubscriptions", 0, 1, &mKeepSubscriptions);
ReportCommand::AddArguments();
SubscribeCommand::AddArguments();
}

SubscribeEvent(chip::ClusterId clusterId, CredentialIssuerCommands * credsIssuerConfig) :
ReportCommand("subscribe-event-by-id", credsIssuerConfig), mClusterIds(1, clusterId)
SubscribeCommand("subscribe-event-by-id", credsIssuerConfig), mClusterIds(1, clusterId)
{
AddArgument("event-id", 0, UINT32_MAX, &mEventIds);
AddArgument("min-interval", 0, UINT16_MAX, &mMinInterval);
AddArgument("max-interval", 0, UINT16_MAX, &mMaxInterval);
AddArgument("fabric-filtered", 0, 1, &mFabricFiltered);
AddArgument("event-min", 0, UINT64_MAX, &mEventNumber);
AddArgument("keepSubscriptions", 0, 1, &mKeepSubscriptions);
ReportCommand::AddArguments();
SubscribeCommand::AddArguments();
}

SubscribeEvent(chip::ClusterId clusterId, const char * eventName, chip::EventId eventId,
CredentialIssuerCommands * credsIssuerConfig) :
ReportCommand("subscribe-event", credsIssuerConfig),
SubscribeCommand("subscribe-event", credsIssuerConfig),
mClusterIds(1, clusterId), mEventIds(1, eventId)
{
AddArgument("event-name", eventName, "Event name.");
Expand All @@ -350,32 +384,15 @@ class SubscribeEvent : public ReportCommand
AddArgument("event-min", 0, UINT64_MAX, &mEventNumber);
AddArgument("keepSubscriptions", 0, 1, &mKeepSubscriptions,
"false - Terminate existing subscriptions from initiator.\n true - Leave existing subscriptions in place.");
ReportCommand::AddArguments();
SubscribeCommand::AddArguments();
}

~SubscribeEvent() {}

CHIP_ERROR SendCommand(chip::DeviceProxy * device, std::vector<chip::EndpointId> endpointIds) override
{
return ReportCommand::SubscribeEvent(device, endpointIds, mClusterIds, mEventIds, mMinInterval, mMaxInterval,
mFabricFiltered, mEventNumber, mKeepSubscriptions);
}

void OnSubscription() override
{
// The ReadClient instance can not be released directly into the OnEventSubscription
// callback since it happens to be called by ReadClient itself which is doing additional
// work after that.
chip::DeviceLayer::PlatformMgr().ScheduleWork(
[](intptr_t arg) {
auto * command = reinterpret_cast<SubscribeEvent *>(arg);
if (!command->IsInteractive())
{
command->InteractionModelReports::Shutdown();
}
command->SetCommandExitStatus(CHIP_NO_ERROR);
},
reinterpret_cast<intptr_t>(this));
return SubscribeCommand::SubscribeEvent(device, endpointIds, mClusterIds, mEventIds, mMinInterval, mMaxInterval,
mFabricFiltered, mEventNumber, mKeepSubscriptions);
}

private:
Expand Down
21 changes: 21 additions & 0 deletions examples/chip-tool/commands/common/CHIPCommand.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#endif // CHIP_CONFIG_TRANSPORT_TRACE_ENABLED

std::map<std::string, std::unique_ptr<chip::Controller::DeviceCommissioner>> CHIPCommand::mCommissioners;
std::set<CHIPCommand *> CHIPCommand::sDeferredCleanups;

using DeviceControllerFactory = chip::Controller::DeviceControllerFactory;

Expand Down Expand Up @@ -180,8 +181,19 @@ CHIP_ERROR CHIPCommand::Run()

CHIP_ERROR err = StartWaiting(GetWaitDuration());

bool deferCleanup = (IsInteractive() && DeferInteractiveCleanup());

Shutdown();

if (deferCleanup)
{
sDeferredCleanups.insert(this);
}
else
{
Cleanup();
}

ReturnErrorOnFailure(MaybeTearDownStack());

return err;
Expand Down Expand Up @@ -422,3 +434,12 @@ void CHIPCommand::StopWaiting()
LogErrorOnFailure(chip::DeviceLayer::PlatformMgr().StopEventLoopTask());
#endif // CONFIG_USE_SEPARATE_EVENTLOOP
}

void CHIPCommand::ExecuteDeferredCleanups()
{
for (auto * cmd : sDeferredCleanups)
{
cmd->Cleanup();
}
sDeferredCleanups.clear();
}
Loading

0 comments on commit 1611900

Please sign in to comment.