fix a bug with actions when starting up with an old message in the queue
Change-Id: I27b82fecb07d34772a283832c1293b850affd1bb
diff --git a/aos/common/actions/action_test.cc b/aos/common/actions/action_test.cc
index 423769e..eb863e3 100644
--- a/aos/common/actions/action_test.cc
+++ b/aos/common/actions/action_test.cc
@@ -1,6 +1,7 @@
#include <unistd.h>
#include <memory>
+#include <thread>
#include "gtest/gtest.h"
#include "aos/common/queue.h"
@@ -124,13 +125,48 @@
EXPECT_FALSE(action_queue_.Running());
}
+// Tests that starting with an old run message in the goal queue actually works.
+// This used to result in the client hanging, waiting for a response to its
+// cancel message.
+TEST_F(ActionTest, StartWithOldGoal) {
+ ASSERT_TRUE(actions::test_action.goal.MakeWithBuilder().run(971).Send());
+
+ TestActorNOP nop_act(&actions::test_action);
+
+ ASSERT_FALSE(actions::test_action.status.FetchLatest());
+ ::std::thread init_thread([&nop_act]() { nop_act.Initialize(); });
+ ::aos::time::SleepFor(::aos::time::Time::InSeconds(0.01));
+ ASSERT_TRUE(actions::test_action.goal.MakeWithBuilder().run(1).Send());
+ init_thread.join();
+ ASSERT_TRUE(actions::test_action.status.FetchLatest());
+ EXPECT_EQ(0u, actions::test_action.status->running);
+ EXPECT_EQ(0u, actions::test_action.status->last_running);
+
+ action_queue_.EnqueueAction(MakeTestActionNOP());
+ nop_act.WaitForActionRequest();
+
+ // We started an action and it should be running.
+ EXPECT_TRUE(action_queue_.Running());
+
+ action_queue_.CancelAllActions();
+ action_queue_.Tick();
+
+ EXPECT_TRUE(action_queue_.Running());
+
+ // Run the action so it can signal completion.
+ nop_act.RunIteration();
+ action_queue_.Tick();
+
+ // Make sure it stopped.
+ EXPECT_FALSE(action_queue_.Running());
+}
+
// Tests that the queues are properly configured for testing. Tests that queues
// work exactly as used in the tests.
TEST_F(ActionTest, QueueCheck) {
actions::TestActionQueueGroup *send_side = &actions::test_action;
actions::TestActionQueueGroup *recv_side = &actions::test_action;
- send_side->goal.MakeMessage();
send_side->goal.MakeWithBuilder().run(1).Send();
EXPECT_TRUE(recv_side->goal.FetchLatest());
@@ -141,7 +177,6 @@
EXPECT_TRUE(recv_side->goal.FetchLatest());
EXPECT_FALSE(recv_side->goal->run);
- send_side->status.MakeMessage();
send_side->status.MakeWithBuilder().running(5).last_running(6).Send();
EXPECT_TRUE(recv_side->status.FetchLatest());
diff --git a/aos/common/actions/actions.h b/aos/common/actions/actions.h
index d35cd20..c729b3f 100644
--- a/aos/common/actions/actions.h
+++ b/aos/common/actions/actions.h
@@ -280,8 +280,9 @@
has_started_ = true;
LOG(DEBUG, "Action %" PRIx32 " on queue %s has been started\n",
run_value_, queue_group_->goal.name());
- } else if (queue_group_->status->running == old_run_value_) {
- // It's still running an old instance (or still doing nothing).
+ } else if (old_run_value_ != 0 &&
+ queue_group_->status->running == old_run_value_) {
+ LOG(DEBUG, "still running old instance %" PRIx32 "\n", old_run_value_);
} else {
LOG(WARNING, "Action %" PRIx32 " on queue %s interrupted by %" PRIx32
" before starting\n",
diff --git a/aos/common/actions/actor.h b/aos/common/actions/actor.h
index 0747646..a3287db 100644
--- a/aos/common/actions/actor.h
+++ b/aos/common/actions/actor.h
@@ -32,6 +32,9 @@
// Runs action while enabled.
void Run();
+ // Gets ready to run actions.
+ void Initialize();
+
// Checks if an action was initially running when the thread started.
bool CheckInitialRunning();
@@ -152,19 +155,7 @@
template <class T>
void ActorBase<T>::Run() {
-
- // Make sure the last job is done and we have a signal.
- if (CheckInitialRunning()) {
- LOG(DEBUG, "action %" PRIx32 " was stopped\n", action_q_->goal->run);
- }
-
- if (!action_q_->status.MakeWithBuilder()
- .running(0)
- .last_running(0)
- .success(!abort_)
- .Send()) {
- LOG(ERROR, "Failed to send the status.\n");
- }
+ Initialize();
while (true) {
// Wait for a request to come in before starting.
@@ -184,6 +175,22 @@
}
template <class T>
+void ActorBase<T>::Initialize() {
+ // Make sure the last job is done and we have a signal.
+ if (CheckInitialRunning()) {
+ LOG(DEBUG, "action %" PRIx32 " was stopped\n", action_q_->goal->run);
+ }
+
+ if (!action_q_->status.MakeWithBuilder()
+ .running(0)
+ .last_running(0)
+ .success(!abort_)
+ .Send()) {
+ LOG(ERROR, "Failed to send the status.\n");
+ }
+}
+
+template <class T>
bool ActorBase<T>::WaitUntil(::std::function<bool(void)> done_condition,
const ::aos::time::Time& end_time) {
while (!done_condition()) {