battor agent: Make timeout per-action rather than per-command

Actions are internal implementation details of the BattOrAgent: reading
a serial data frame is an action.

Commands make up the interface of the BattOrAgent: StopTracing is a
command.

The problem with having a per-command timeout is that StopTracing can
take a really long time to run if the trace has been collecting data
for a while. StopTracing always takes 1/6 of the trace runtime to
download, so any timeout we could add would be violated by running a
trace for >6x that long.

Now, as long as the BattOrAgent is making progress towards the
completion of a command, it won't time out.

BUG=585532,585533

Review URL: https://codereview.chromium.org/1815323005

Cr-Commit-Position: refs/heads/master@{#382328}

4 files changed, 50 insertions, 13 deletions

diff --git a/tools/battor_agent/battor_agent.cc b/tools/battor_agent/battor_agent.cc
index 05f47873..0195b81 100644
--- a/tools/battor_agent/battor_agent.cc
+++ b/tools/battor_agent/battor_agent.cc
@@ -30,6 +30,9 @@ const uint8_t kReadRetryDelayMilliseconds = 1;
 // order to ensure that the sample we synced to doesn't get thrown out.
 const uint8_t kStopTracingClockSyncDelayMilliseconds = 100;
 
+// The number of seconds allowed for a given action before timing out.
+const uint8_t kBattOrTimeoutSeconds = 10;
+
 // Returns true if the specified vector of bytes decodes to a message that is an
 // ack for the specified control message type.
 bool IsAckOfControlCommand(BattOrMessageType message_type,
@@ -159,6 +162,11 @@ void BattOrAgent::BeginConnect() {
 }
 
 void BattOrAgent::OnConnectionOpened(bool success) {
+  // Return immediately if opening the connection already timed out.
+  if (timeout_callback_.IsCancelled())
+    return;
+  timeout_callback_.Cancel();
+
   if (!success) {
     CompleteCommand(BATTOR_ERROR_CONNECTION_FAILED);
     return;
@@ -186,6 +194,12 @@ void BattOrAgent::OnConnectionOpened(bool success) {
 void BattOrAgent::OnBytesSent(bool success) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  // Return immediately if whatever action we were trying to perform already
+  // timed out.
+  if (timeout_callback_.IsCancelled())
+    return;
+  timeout_callback_.Cancel();
+
   if (!success) {
     CompleteCommand(BATTOR_ERROR_SEND_ERROR);
     return;
@@ -231,6 +245,12 @@ void BattOrAgent::OnBytesSent(bool success) {
 void BattOrAgent::OnMessageRead(bool success,
                                 BattOrMessageType type,
                                 scoped_ptr<vector<char>> bytes) {
+  // Return immediately if whatever action we were trying to perform already
+  // timed out.
+  if (timeout_callback_.IsCancelled())
+    return;
+  timeout_callback_.Cancel();
+
   if (!success) {
     switch (last_action_) {
       case Action::READ_EEPROM:
@@ -366,6 +386,12 @@ void BattOrAgent::OnMessageRead(bool success,
 void BattOrAgent::PerformAction(Action action) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  timeout_callback_.Reset(
+      base::Bind(&BattOrAgent::OnActionTimeout, AsWeakPtr()));
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE, timeout_callback_.callback(),
+      base::TimeDelta::FromSeconds(kBattOrTimeoutSeconds));
+
   last_action_ = action;
 
   switch (action) {
@@ -456,6 +482,11 @@ void BattOrAgent::PerformDelayedAction(Action action, base::TimeDelta delay) {
       delay);
 }
 
+void BattOrAgent::OnActionTimeout() {
+  CompleteCommand(BATTOR_ERROR_TIMEOUT);
+  timeout_callback_.Cancel();
+}
+
 void BattOrAgent::SendControlMessage(BattOrControlMessageType type,
                                      uint16_t param1,
                                      uint16_t param2) {
diff --git a/tools/battor_agent/battor_agent.h b/tools/battor_agent/battor_agent.h
index b31d5e4..524b5a8 100644
--- a/tools/battor_agent/battor_agent.h
+++ b/tools/battor_agent/battor_agent.h
@@ -7,6 +7,7 @@
 
 #include <map>
 
+#include "base/cancelable_callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
@@ -72,6 +73,12 @@ class BattOrAgent : public BattOrConnection::Listener,
   // fake in testing.
   scoped_ptr<BattOrConnection> connection_;
 
+  // Timeout for when an action isn't completed within the allotted time. This
+  // is virtual and protected so that timeouts can be disabled in testing. The
+  // testing task runner that runs delayed tasks immediately deals poorly with
+  // timeouts posted as future tasks.
+  virtual void OnActionTimeout();
+
  private:
   enum class Command {
     INVALID,
@@ -112,6 +119,8 @@ class BattOrAgent : public BattOrConnection::Listener,
   // Performs an action after a delay.
   void PerformDelayedAction(Action action, base::TimeDelta delay);
 
+
+
   // Requests a connection to the BattOr.
   void BeginConnect();
 
@@ -167,6 +176,9 @@ class BattOrAgent : public BattOrConnection::Listener,
   // The number of times that we've attempted to read the last message.
   uint8_t num_read_attempts_;
 
+  // The timeout that's run when an action times out.
+  base::CancelableClosure timeout_callback_;
+
   DISALLOW_COPY_AND_ASSIGN(BattOrAgent);
 };
 
diff --git a/tools/battor_agent/battor_agent_bin.cc b/tools/battor_agent/battor_agent_bin.cc
index 1c00b47..50f4e34 100644
--- a/tools/battor_agent/battor_agent_bin.cc
+++ b/tools/battor_agent/battor_agent_bin.cc
@@ -57,7 +57,6 @@ namespace {
 const char kIoThreadName[] = "BattOr IO Thread";
 const char kFileThreadName[] = "BattOr File Thread";
 const char kUiThreadName[] = "BattOr UI Thread";
-const int32_t kBattOrCommandTimeoutSeconds = 10;
 
 const char kUsage[] =
     "Start the battor_agent shell with:\n"
@@ -168,7 +167,7 @@ class BattOrAgentBin : public BattOrAgent::Listener {
     io_thread_.task_runner()->PostTask(
         FROM_HERE,
         base::Bind(&BattOrAgentBin::CreateAgent, base::Unretained(this), path));
-    AwaitResult();
+    done_.Wait();
   }
 
   // Performs any cleanup necessary after the BattOr binary is done running.
@@ -176,14 +175,14 @@ class BattOrAgentBin : public BattOrAgent::Listener {
     io_thread_.task_runner()->PostTask(
         FROM_HERE,
         base::Bind(&BattOrAgentBin::DeleteAgent, base::Unretained(this)));
-    AwaitResult();
+    done_.Wait();
   }
 
   void StartTracing() {
     io_thread_.task_runner()->PostTask(
         FROM_HERE,
         base::Bind(&BattOrAgent::StartTracing, base::Unretained(agent_.get())));
-    AwaitResult();
+    done_.Wait();
   }
 
   void OnStartTracingComplete(BattOrError error) override {
@@ -199,7 +198,7 @@ class BattOrAgentBin : public BattOrAgent::Listener {
     io_thread_.task_runner()->PostTask(
         FROM_HERE,
         base::Bind(&BattOrAgent::StopTracing, base::Unretained(agent_.get())));
-    AwaitResult();
+    done_.Wait();
   }
 
   void OnStopTracingComplete(const std::string& trace,
@@ -218,7 +217,7 @@ class BattOrAgentBin : public BattOrAgent::Listener {
     io_thread_.task_runner()->PostTask(
         FROM_HERE, base::Bind(&BattOrAgent::RecordClockSyncMarker,
                               base::Unretained(agent_.get()), marker));
-    AwaitResult();
+    done_.Wait();
   }
 
   void OnRecordClockSyncMarkerComplete(BattOrError error) override {
@@ -258,13 +257,6 @@ class BattOrAgentBin : public BattOrAgent::Listener {
     done_.Signal();
   }
 
-  // Waits until the previously executed command has finished executing.
-  void AwaitResult() {
-    if (!done_.TimedWait(
-            base::TimeDelta::FromSeconds(kBattOrCommandTimeoutSeconds)))
-      HandleError(BATTOR_ERROR_TIMEOUT);
-  }
-
  private:
   // Event signaled when an async task has finished executing.
   base::WaitableEvent done_;
diff --git a/tools/battor_agent/battor_agent_unittest.cc b/tools/battor_agent/battor_agent_unittest.cc
index eaf4c4a..99433eb 100644
--- a/tools/battor_agent/battor_agent_unittest.cc
+++ b/tools/battor_agent/battor_agent_unittest.cc
@@ -86,6 +86,8 @@ class TestableBattOrAgent : public BattOrAgent {
   MockBattOrConnection* GetConnection() {
     return static_cast<MockBattOrConnection*>(connection_.get());
   }
+
+  void OnActionTimeout() override {}
 };
 
 // BattOrAgentTest provides a BattOrAgent and captures the results of its