Call command handler with weak_ptr
Command handler should keep only weak_ptr to command and check on every
access. Long async commands will need to keep reference to the command.
BUG:24267885
Change-Id: I3ca3be9e31b9e9a942eca001ed21f1133973f0ea
Reviewed-on: https://weave-review.googlesource.com/1255
Reviewed-by: Alex Vakulenko <avakulenko@google.com>
diff --git a/libweave/examples/ubuntu/main.cc b/libweave/examples/ubuntu/main.cc
index a116178..28611f5 100644
--- a/libweave/examples/ubuntu/main.cc
+++ b/libweave/examples/ubuntu/main.cc
@@ -33,12 +33,26 @@
class CommandHandler {
public:
- explicit CommandHandler(weave::Device* device) : device_{device} {
+ CommandHandler(weave::Device* device,
+ weave::provider::TaskRunner* task_runner)
+ : device_{device}, task_runner_(task_runner) {
+ device->AddStateDefinitionsFromJson(R"({
+ "_greeter": {"_greetings_counter":"integer"},
+ "_ledflasher": {"_leds": {"items": "boolean"}}
+ })");
+
+ device->SetStatePropertiesFromJson(R"({
+ "_greeter": {"_greetings_counter": 0},
+ "_ledflasher":{"_leds": [false, false, false]}
+ })",
+ nullptr);
+
device->AddCommandDefinitionsFromJson(R"({
"_greeter": {
"_greet": {
"minimalRole": "user",
- "parameters": { "_name": "string"},
+ "parameters": { "_name": "string", "_count": "integer"},
+ "progress": { "_todo": "integer"},
"results": { "_greeting": "string" }
}
},
@@ -56,58 +70,76 @@
}
}
})");
-
- device->AddStateDefinitionsFromJson(R"({
- "_greeter": {"_greetings_counter":"integer"},
- "_ledflasher": {"_leds": {"items": "boolean"}}
- })");
-
- device->SetStatePropertiesFromJson(R"({
- "_greeter": {"_greetings_counter": 0},
- "_ledflasher":{"_leds": [false, false, false]}
- })",
- nullptr);
-
+ device->AddCommandHandler(
+ "_ledflasher._toggle",
+ base::Bind(&CommandHandler::OnFlasherToggleCommand,
+ weak_ptr_factory_.GetWeakPtr()));
device->AddCommandHandler("_greeter._greet",
base::Bind(&CommandHandler::OnGreetCommand,
weak_ptr_factory_.GetWeakPtr()));
device->AddCommandHandler("_ledflasher._set",
base::Bind(&CommandHandler::OnFlasherSetCommand,
weak_ptr_factory_.GetWeakPtr()));
- device->AddCommandHandler(
- "_ledflasher._toggle",
- base::Bind(&CommandHandler::OnFlasherToggleCommand,
- weak_ptr_factory_.GetWeakPtr()));
device->AddCommandHandler("",
base::Bind(&CommandHandler::OnUnhandledCommand,
weak_ptr_factory_.GetWeakPtr()));
}
private:
- void OnGreetCommand(weave::Command* cmd) {
- LOG(INFO) << "received command: " << cmd->GetName();
+ void DoGreet(const std::weak_ptr<weave::Command>& command, int todo) {
+ auto cmd = command.lock();
+ if (!cmd)
+ return;
+
std::string name;
if (!cmd->GetParameters()->GetString("_name", &name))
name = "anonymous";
- LOG(INFO) << cmd->GetName() << " command in progress";
- cmd->SetProgress(base::DictionaryValue{}, nullptr);
+ if (todo > 0) {
+ LOG(INFO) << "Hello " << name;
+
+ base::DictionaryValue progress;
+ progress.SetInteger("_todo", todo);
+ cmd->SetProgress(progress, nullptr);
+
+ base::DictionaryValue state;
+ state.SetInteger("_greeter._greetings_counter", ++counter_);
+ device_->SetStateProperties(state, nullptr);
+ }
+
+ if (--todo > 0) {
+ task_runner_->PostDelayedTask(
+ FROM_HERE, base::Bind(&CommandHandler::DoGreet,
+ weak_ptr_factory_.GetWeakPtr(), command, todo),
+ base::TimeDelta::FromSeconds(1));
+ return;
+ }
base::DictionaryValue result;
result.SetString("_greeting", "Hello " + name);
cmd->SetResults(result, nullptr);
LOG(INFO) << cmd->GetName() << " command finished: " << result;
- base::DictionaryValue state;
- state.SetInteger("_greeter._greetings_counter", ++counter_);
- device_->SetStateProperties(state, nullptr);
-
LOG(INFO) << "New state: " << *device_->GetState();
cmd->Done();
}
- void OnFlasherSetCommand(weave::Command* cmd) {
+ void OnGreetCommand(const std::weak_ptr<weave::Command>& command) {
+ auto cmd = command.lock();
+ if (!cmd)
+ return;
+ LOG(INFO) << "received command: " << cmd->GetName();
+
+ int todo = 1;
+ cmd->GetParameters()->GetInteger("_count", &todo);
+ DoGreet(command, todo);
+ }
+
+ void OnFlasherSetCommand(const std::weak_ptr<weave::Command>& command) {
+ auto cmd = command.lock();
+ if (!cmd)
+ return;
LOG(INFO) << "received command: " << cmd->GetName();
int32_t led_index;
bool cmd_value;
@@ -130,7 +162,10 @@
cmd->Abort();
}
- void OnFlasherToggleCommand(weave::Command* cmd) {
+ void OnFlasherToggleCommand(const std::weak_ptr<weave::Command>& command) {
+ auto cmd = command.lock();
+ if (!cmd)
+ return;
LOG(INFO) << "received command: " << cmd->GetName();
int32_t led_index;
if (cmd->GetParameters()->GetInteger("_led", &led_index)) {
@@ -144,7 +179,10 @@
cmd->Abort();
}
- void OnUnhandledCommand(weave::Command* cmd) {
+ void OnUnhandledCommand(const std::weak_ptr<weave::Command>& command) {
+ auto cmd = command.lock();
+ if (!cmd)
+ return;
LOG(INFO) << cmd->GetName() << " unimplemented command: ignored";
}
@@ -157,6 +195,8 @@
}
weave::Device* device_{nullptr};
+ weave::provider::TaskRunner* task_runner_{nullptr};
+
int counter_{0};
// Simulate LED status on this device so client app could explore
@@ -218,7 +258,7 @@
}
}
- CommandHandler handler(device.get());
+ CommandHandler handler(device.get(), &task_runner);
task_runner.Run();
LOG(INFO) << "exit";
diff --git a/libweave/include/weave/command.h b/libweave/include/weave/command.h
index 911da1e..6c399e1 100644
--- a/libweave/include/weave/command.h
+++ b/libweave/include/weave/command.h
@@ -27,25 +27,6 @@
class Command {
public:
- // This interface lets the command to notify clients about changes.
- class Observer {
- public:
- virtual void OnResultsChanged() = 0;
- virtual void OnStatusChanged() = 0;
- virtual void OnProgressChanged() = 0;
- virtual void OnCommandDestroyed() = 0;
-
- protected:
- virtual ~Observer() = default;
- };
-
- // Adds an observer for this command. The observer object is not owned by this
- // class.
- virtual void AddObserver(Observer* observer) = 0;
-
- // Removes an observer for this command.
- virtual void RemoveObserver(Observer* observer) = 0;
-
// Returns the full command ID.
virtual const std::string& GetID() const = 0;
diff --git a/libweave/include/weave/device.h b/libweave/include/weave/device.h
index 289cedb..7e49202 100644
--- a/libweave/include/weave/device.h
+++ b/libweave/include/weave/device.h
@@ -52,7 +52,8 @@
virtual void AddCommandDefinitions(const base::DictionaryValue& dict) = 0;
// Callback type for AddCommandHandler.
- using CommandHandlerCallback = base::Callback<void(Command*)>;
+ using CommandHandlerCallback =
+ base::Callback<void(const std::weak_ptr<Command>& command)>;
// Sets handler for new commands added to the queue.
// |command_name| is the full command name of the command to handle. e.g.
diff --git a/libweave/include/weave/test/mock_command.h b/libweave/include/weave/test/mock_command.h
index be1b0c3..b856140 100644
--- a/libweave/include/weave/test/mock_command.h
+++ b/libweave/include/weave/test/mock_command.h
@@ -20,8 +20,6 @@
public:
~MockCommand() override = default;
- MOCK_METHOD1(AddObserver, void(Observer*));
- MOCK_METHOD1(RemoveObserver, void(Observer*));
MOCK_CONST_METHOD0(GetID, const std::string&());
MOCK_CONST_METHOD0(GetName, const std::string&());
MOCK_CONST_METHOD0(GetCategory, const std::string&());
diff --git a/libweave/src/base_api_handler.cc b/libweave/src/base_api_handler.cc
index 1a0a58d..dad443f 100644
--- a/libweave/src/base_api_handler.cc
+++ b/libweave/src/base_api_handler.cc
@@ -46,7 +46,11 @@
weak_ptr_factory_.GetWeakPtr()));
}
-void BaseApiHandler::UpdateBaseConfiguration(Command* command) {
+void BaseApiHandler::UpdateBaseConfiguration(
+ const std::weak_ptr<Command>& cmd) {
+ auto command = cmd.lock();
+ if (!command)
+ return;
CHECK(command->GetStatus() == CommandStatus::kQueued)
<< EnumToString(command->GetStatus());
command->SetProgress(base::DictionaryValue{}, nullptr);
@@ -83,7 +87,10 @@
device_->SetStateProperties(state, nullptr);
}
-void BaseApiHandler::UpdateDeviceInfo(Command* command) {
+void BaseApiHandler::UpdateDeviceInfo(const std::weak_ptr<Command>& cmd) {
+ auto command = cmd.lock();
+ if (!command)
+ return;
CHECK(command->GetStatus() == CommandStatus::kQueued)
<< EnumToString(command->GetStatus());
command->SetProgress(base::DictionaryValue{}, nullptr);
diff --git a/libweave/src/base_api_handler.h b/libweave/src/base_api_handler.h
index 7712de1..c690b03 100644
--- a/libweave/src/base_api_handler.h
+++ b/libweave/src/base_api_handler.h
@@ -28,8 +28,8 @@
BaseApiHandler(DeviceRegistrationInfo* device_info, Device* device);
private:
- void UpdateBaseConfiguration(Command* command);
- void UpdateDeviceInfo(Command* command);
+ void UpdateBaseConfiguration(const std::weak_ptr<Command>& command);
+ void UpdateDeviceInfo(const std::weak_ptr<Command>& command);
bool UpdateState(const std::string& anonymous_access_role,
bool discovery_enabled,
bool pairing_enabled);
diff --git a/libweave/src/commands/cloud_command_proxy.h b/libweave/src/commands/cloud_command_proxy.h
index c5be02e..711befa 100644
--- a/libweave/src/commands/cloud_command_proxy.h
+++ b/libweave/src/commands/cloud_command_proxy.h
@@ -17,6 +17,7 @@
#include "src/backoff_entry.h"
#include "src/commands/cloud_command_update_interface.h"
+#include "src/commands/command_instance.h"
#include "src/states/state_change_queue_interface.h"
namespace weave {
@@ -28,7 +29,7 @@
}
// Command proxy which publishes command updates to the cloud.
-class CloudCommandProxy final : public Command::Observer {
+class CloudCommandProxy final : public CommandInstance::Observer {
public:
CloudCommandProxy(CommandInstance* command_instance,
CloudCommandUpdateInterface* cloud_command_updater,
@@ -93,7 +94,7 @@
// successfully.
UpdateID last_state_update_id_{0};
- ScopedObserver<Command, Command::Observer> observer_{this};
+ ScopedObserver<CommandInstance, CommandInstance::Observer> observer_{this};
base::WeakPtrFactory<CloudCommandProxy> backoff_weak_ptr_factory_{this};
base::WeakPtrFactory<CloudCommandProxy> weak_ptr_factory_{this};
diff --git a/libweave/src/commands/command_instance.cc b/libweave/src/commands/command_instance.cc
index a8891f7..305a255 100644
--- a/libweave/src/commands/command_instance.cc
+++ b/libweave/src/commands/command_instance.cc
@@ -37,6 +37,12 @@
{CommandOrigin::kCloud, "cloud"},
};
+void ReportDestroyedError(ErrorPtr* error) {
+ Error::AddTo(error, FROM_HERE, errors::commands::kDomain,
+ errors::commands::kCommandDestroyed,
+ "Command has been destroyed");
+}
+
} // namespace
template <>
@@ -92,6 +98,10 @@
bool CommandInstance::SetProgress(const base::DictionaryValue& progress,
ErrorPtr* error) {
+ if (!command_definition_) {
+ ReportDestroyedError(error);
+ return false;
+ }
ObjectPropType obj_prop_type;
obj_prop_type.SetObjectSchema(command_definition_->GetProgress()->Clone());
@@ -110,6 +120,10 @@
bool CommandInstance::SetResults(const base::DictionaryValue& results,
ErrorPtr* error) {
+ if (!command_definition_) {
+ ReportDestroyedError(error);
+ return false;
+ }
ObjectPropType obj_prop_type;
obj_prop_type.SetObjectSchema(command_definition_->GetResults()->Clone());
diff --git a/libweave/src/commands/command_instance.h b/libweave/src/commands/command_instance.h
index 8a54b05..20e1daa 100644
--- a/libweave/src/commands/command_instance.h
+++ b/libweave/src/commands/command_instance.h
@@ -31,6 +31,17 @@
class CommandInstance final : public Command {
public:
+ class Observer {
+ public:
+ virtual void OnResultsChanged() = 0;
+ virtual void OnStatusChanged() = 0;
+ virtual void OnProgressChanged() = 0;
+ virtual void OnCommandDestroyed() = 0;
+
+ protected:
+ virtual ~Observer() = default;
+ };
+
// Construct a command instance given the full command |name| which must
// be in format "<package_name>.<command_name>", a command |category| and
// a list of parameters and their values specified in |parameters|.
@@ -42,8 +53,6 @@
// Command overrides.
std::unique_ptr<base::DictionaryValue> ToJson() const override;
- void AddObserver(Observer* observer) override;
- void RemoveObserver(Observer* observer) override;
const std::string& GetID() const override;
const std::string& GetName() const override;
CommandStatus GetStatus() const override;
@@ -82,8 +91,17 @@
// Sets the command ID (normally done by CommandQueue when the command
// instance is added to it).
void SetID(const std::string& id) { id_ = id; }
+
+ void AddObserver(Observer* observer);
+ void RemoveObserver(Observer* observer);
+
// Sets the pointer to queue this command is part of.
- void SetCommandQueue(CommandQueue* queue) { queue_ = queue; }
+ void AttachToQueue(CommandQueue* queue) { queue_ = queue; }
+ void DetachFromQueue() {
+ observers_.Clear();
+ queue_ = nullptr;
+ command_definition_ = nullptr;
+ }
private:
// Helper function to update the command status.
@@ -101,7 +119,7 @@
// The origin of the command, either "local" or "cloud".
CommandOrigin origin_ = CommandOrigin::kLocal;
// Command definition.
- const CommandDefinition* command_definition_;
+ const CommandDefinition* command_definition_{nullptr};
// Command parameters and their values.
ValueMap parameters_;
// Current command execution progress.
diff --git a/libweave/src/commands/command_manager.cc b/libweave/src/commands/command_manager.cc
index efca5ff..22d00dc 100644
--- a/libweave/src/commands/command_manager.cc
+++ b/libweave/src/commands/command_manager.cc
@@ -182,7 +182,7 @@
void CommandManager::AddCommandHandler(
const std::string& command_name,
- const CommandQueue::CommandCallback& callback) {
+ const Device::CommandHandlerCallback& callback) {
CHECK(command_name.empty() || base_dictionary_.FindCommand(command_name))
<< "Command undefined: " << command_name;
command_queue_.AddCommandHandler(command_name, callback);
diff --git a/libweave/src/commands/command_manager.h b/libweave/src/commands/command_manager.h
index f34a907..fe3cb71 100644
--- a/libweave/src/commands/command_manager.h
+++ b/libweave/src/commands/command_manager.h
@@ -36,7 +36,7 @@
void AddCommandAddedCallback(const CommandQueue::CommandCallback& callback);
void AddCommandRemovedCallback(const CommandQueue::CommandCallback& callback);
void AddCommandHandler(const std::string& command_name,
- const CommandQueue::CommandCallback& callback);
+ const Device::CommandHandlerCallback& callback);
// Sets callback which is called when command definitions is changed.
void AddCommandDefChanged(const base::Closure& callback);
diff --git a/libweave/src/commands/command_queue.cc b/libweave/src/commands/command_queue.cc
index df86d77..efdcfdc 100644
--- a/libweave/src/commands/command_queue.cc
+++ b/libweave/src/commands/command_queue.cc
@@ -24,8 +24,9 @@
on_command_removed_.push_back(callback);
}
-void CommandQueue::AddCommandHandler(const std::string& command_name,
- const CommandCallback& callback) {
+void CommandQueue::AddCommandHandler(
+ const std::string& command_name,
+ const Device::CommandHandlerCallback& callback) {
if (!command_name.empty()) {
CHECK(default_command_callback_.is_null())
<< "Commands specific handler are not allowed after default one";
@@ -33,7 +34,7 @@
for (const auto& command : map_) {
if (command.second->GetStatus() == CommandStatus::kQueued &&
command.second->GetName() == command_name) {
- callback.Run(command.second.get());
+ callback.Run(command.second);
}
}
@@ -45,7 +46,7 @@
if (command.second->GetStatus() == CommandStatus::kQueued &&
command_callbacks_.find(command.second->GetName()) ==
command_callbacks_.end()) {
- callback.Run(command.second.get());
+ callback.Run(command.second);
}
}
@@ -57,7 +58,7 @@
void CommandQueue::Add(std::unique_ptr<CommandInstance> instance) {
std::string id = instance->GetID();
LOG_IF(FATAL, id.empty()) << "Command has no ID";
- instance->SetCommandQueue(this);
+ instance->AttachToQueue(this);
auto pair = map_.insert(std::make_pair(id, std::move(instance)));
LOG_IF(FATAL, !pair.second) << "Command with ID '" << id
<< "' is already in the queue";
@@ -67,9 +68,9 @@
auto it_handler = command_callbacks_.find(pair.first->second->GetName());
if (it_handler != command_callbacks_.end())
- it_handler->second.Run(pair.first->second.get());
+ it_handler->second.Run(pair.first->second);
else if (!default_command_callback_.is_null())
- default_command_callback_.Run(pair.first->second.get());
+ default_command_callback_.Run(pair.first->second);
Cleanup();
}
@@ -88,8 +89,8 @@
auto p = map_.find(id);
if (p == map_.end())
return false;
- std::unique_ptr<CommandInstance> instance{std::move(p->second)};
- instance->SetCommandQueue(nullptr);
+ std::shared_ptr<CommandInstance> instance = p->second;
+ instance->DetachFromQueue();
map_.erase(p);
for (const auto& cb : on_command_removed_)
cb.Run(instance.get());
diff --git a/libweave/src/commands/command_queue.h b/libweave/src/commands/command_queue.h
index 888c04f..99da0b2 100644
--- a/libweave/src/commands/command_queue.h
+++ b/libweave/src/commands/command_queue.h
@@ -25,7 +25,8 @@
public:
CommandQueue() = default;
- using CommandCallback = Device::CommandHandlerCallback;
+ // TODO: Remove AddCommandAddedCallback and AddCommandRemovedCallback.
+ using CommandCallback = base::Callback<void(Command* command)>;
// Adds notifications callback for a new command is added to the queue.
void AddCommandAddedCallback(const CommandCallback& callback);
@@ -34,7 +35,7 @@
void AddCommandRemovedCallback(const CommandCallback& callback);
void AddCommandHandler(const std::string& command_name,
- const CommandCallback& callback);
+ const Device::CommandHandlerCallback& callback);
// Checks if the command queue is empty.
bool IsEmpty() const { return map_.empty(); }
@@ -75,7 +76,7 @@
base::Time test_now_;
// ID-to-CommandInstance map.
- std::map<std::string, std::unique_ptr<CommandInstance>> map_;
+ std::map<std::string, std::shared_ptr<CommandInstance>> map_;
// Queue of commands to be removed.
std::queue<std::pair<base::Time, std::string>> remove_queue_;
@@ -83,8 +84,8 @@
using CallbackList = std::vector<CommandCallback>;
CallbackList on_command_added_;
CallbackList on_command_removed_;
- std::map<std::string, CommandCallback> command_callbacks_;
- CommandCallback default_command_callback_;
+ std::map<std::string, Device::CommandHandlerCallback> command_callbacks_;
+ Device::CommandHandlerCallback default_command_callback_;
DISALLOW_COPY_AND_ASSIGN(CommandQueue);
};
diff --git a/libweave/src/commands/schema_constants.cc b/libweave/src/commands/schema_constants.cc
index 9affe73..02999f7 100644
--- a/libweave/src/commands/schema_constants.cc
+++ b/libweave/src/commands/schema_constants.cc
@@ -25,6 +25,7 @@
const char kCommandFailed[] = "command_failed";
const char kInvalidCommandVisibility[] = "invalid_command_visibility";
const char kInvalidMinimalRole[] = "invalid_minimal_role";
+const char kCommandDestroyed[] = "command_destroyed";
} // namespace commands
} // namespace errors
diff --git a/libweave/src/commands/schema_constants.h b/libweave/src/commands/schema_constants.h
index 2d4ae07..9eb8051 100644
--- a/libweave/src/commands/schema_constants.h
+++ b/libweave/src/commands/schema_constants.h
@@ -28,6 +28,7 @@
extern const char kCommandFailed[];
extern const char kInvalidCommandVisibility[];
extern const char kInvalidMinimalRole[];
+extern const char kCommandDestroyed[];
} // namespace commands
} // namespace errors
