Tether MessageTransferOperation: Only wait for a response from a host for a...

Tether MessageTransferOperation: Only wait for a response from a host for a certain amount of time before timing out.

BUG=672263

Review-Url: https://codereview.chromium.org/2915713002
Cr-Commit-Position: refs/heads/master@{#476382}

chromeos/components/tether/disconnect_tethering_operation.cc

@@ -86,6 +86,10 @@ MessageType DisconnectTetheringOperation::GetMessageTypeForConnection() {
   return MessageType::DISCONNECT_TETHERING_REQUEST;
 }
 
+bool DisconnectTetheringOperation::ShouldWaitForResponse() {
+  return false;
+}
+
 }  // namespace tether
 
 }  // namespace chromeos

chromeos/components/tether/disconnect_tethering_operation.h

@@ -58,6 +58,7 @@ class DisconnectTetheringOperation : public MessageTransferOperation {
       const cryptauth::RemoteDevice& remote_device) override;
   void OnOperationFinished() override;
   MessageType GetMessageTypeForConnection() override;
+  bool ShouldWaitForResponse() override;
 
  private:
   friend class DisconnectTetheringOperationTest;

chromeos/components/tether/message_transfer_operation.cc

@@ -7,6 +7,7 @@
 #include <set>
 
 #include "chromeos/components/tether/message_wrapper.h"
+#include "chromeos/components/tether/timer_factory.h"
 #include "components/proximity_auth/logging/logging.h"
 
 namespace chromeos {
@@ -35,12 +36,17 @@ std::vector<cryptauth::RemoteDevice> RemoveDuplicatesFromVector(
 // static
 uint32_t MessageTransferOperation::kMaxConnectionAttemptsPerDevice = 3;
 
+// static
+uint32_t MessageTransferOperation::kDefaultResponseTimeoutSeconds = 10;
+
 MessageTransferOperation::MessageTransferOperation(
     const std::vector<cryptauth::RemoteDevice>& devices_to_connect,
     BleConnectionManager* connection_manager)
     : remote_devices_(RemoveDuplicatesFromVector(devices_to_connect)),
       connection_manager_(connection_manager),
-      initialized_(false) {}
+      timer_factory_(base::MakeUnique<TimerFactory>()),
+      initialized_(false),
+      weak_ptr_factory_(this) {}
 
 MessageTransferOperation::~MessageTransferOperation() {
   connection_manager_->RemoveObserver(this);
@@ -63,6 +69,9 @@ void MessageTransferOperation::Initialize() {
     cryptauth::SecureChannel::Status status;
     if (connection_manager_->GetStatusForDevice(remote_device, &status) &&
         status == cryptauth::SecureChannel::Status::AUTHENTICATED) {
+      if (ShouldWaitForResponse())
+        StartResponseTimerForDevice(remote_device);
+
       OnDeviceAuthenticated(remote_device);
     }
   }
@@ -81,6 +90,9 @@ void MessageTransferOperation::OnSecureChannelStatusChanged(
   }
 
   if (new_status == cryptauth::SecureChannel::Status::AUTHENTICATED) {
+    if (ShouldWaitForResponse())
+      StartResponseTimerForDevice(remote_device);
+
     OnDeviceAuthenticated(remote_device);
   } else if (old_status == cryptauth::SecureChannel::Status::AUTHENTICATING) {
     // If authentication fails, account details (e.g., BeaconSeeds) are not
@@ -139,6 +151,9 @@ void MessageTransferOperation::UnregisterDevice(
   remote_devices_.erase(std::remove(remote_devices_.begin(),
                                     remote_devices_.end(), remote_device),
                         remote_devices_.end());
+  if (ShouldWaitForResponse())
+    StopResponseTimerForDeviceIfRunning(remote_device);
+
   connection_manager_->UnregisterRemoteDevice(remote_device,
                                               GetMessageTypeForConnection());
 
@@ -154,6 +169,56 @@ void MessageTransferOperation::SendMessageToDevice(
                                    message_wrapper->ToRawMessage());
 }
 
+bool MessageTransferOperation::ShouldWaitForResponse() {
+  return true;
+}
+
+uint32_t MessageTransferOperation::GetResponseTimeoutSeconds() {
+  return MessageTransferOperation::kDefaultResponseTimeoutSeconds;
+}
+
+void MessageTransferOperation::SetTimerFactoryForTest(
+    std::unique_ptr<TimerFactory> timer_factory_for_test) {
+  timer_factory_ = std::move(timer_factory_for_test);
+}
+
+void MessageTransferOperation::StartResponseTimerForDevice(
+    const cryptauth::RemoteDevice& remote_device) {
+  DCHECK(ShouldWaitForResponse());
+
+  PA_LOG(INFO) << "Starting timer to wait for response to message type "
+               << GetMessageTypeForConnection() << " from device with ID "
+               << remote_device.GetTruncatedDeviceIdForLogs() << ".";
+
+  remote_device_to_timer_map_.emplace(remote_device,
+                                      timer_factory_->CreateOneShotTimer());
+  remote_device_to_timer_map_[remote_device]->Start(
+      FROM_HERE, base::TimeDelta::FromSeconds(GetResponseTimeoutSeconds()),
+      base::Bind(&MessageTransferOperation::OnResponseTimeout,
+                 weak_ptr_factory_.GetWeakPtr(), remote_device));
+}
+
+void MessageTransferOperation::StopResponseTimerForDeviceIfRunning(
+    const cryptauth::RemoteDevice& remote_device) {
+  DCHECK(ShouldWaitForResponse());
+
+  if (!remote_device_to_timer_map_[remote_device])
+    return;
+
+  remote_device_to_timer_map_[remote_device]->Stop();
+  remote_device_to_timer_map_.erase(remote_device);
+}
+
+void MessageTransferOperation::OnResponseTimeout(
+    const cryptauth::RemoteDevice& remote_device) {
+  PA_LOG(WARNING) << "Timed out while waiting for response to message type "
+                  << GetMessageTypeForConnection() << " from device with ID "
+                  << remote_device.GetTruncatedDeviceIdForLogs() << ".";
+
+  remote_device_to_timer_map_.erase(remote_device);
+  UnregisterDevice(remote_device);
+}
+
 }  // namespace tether
 
 }  // namespace chromeos

chromeos/components/tether/message_transfer_operation.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/timer/timer.h"
 #include "chromeos/components/tether/ble_connection_manager.h"
 
 namespace chromeos {
@@ -16,6 +17,7 @@ namespace chromeos {
 namespace tether {
 
 class MessageWrapper;
+class TimerFactory;
 
 // Abstract base class used for operations which send and/or receive messages
 // from remote devices.
@@ -70,6 +72,16 @@ class MessageTransferOperation : public BleConnectionManager::Observer {
   // Returns the type of message that this operation intends to send.
   virtual MessageType GetMessageTypeForConnection() = 0;
 
+  // Returns true if we should wait for a response from host devices. Note
+  // that if ShouldWaitForResponse() returns true and a response is not received
+  // within the amount of time returned by GetResponseTimeoutSeconds() after a
+  // host device authenticates, that host device will be unregistered.
+  virtual bool ShouldWaitForResponse();
+
+  // The number of seconds that we should wait for a response from the host. If
+  // ShouldWaitForResponse() returns false, this method is never used.
+  virtual uint32_t GetResponseTimeoutSeconds();
+
   std::vector<cryptauth::RemoteDevice>& remote_devices() {
     return remote_devices_;
   }
@@ -82,12 +94,31 @@ class MessageTransferOperation : public BleConnectionManager::Observer {
 
   static uint32_t kMaxConnectionAttemptsPerDevice;
 
+  // The default number of seconds the client should generally wait for a
+  // response from the host once an authenticated connection is established.
+  // Once this amount of time passes, the connection will be closed. Subclasses
+  // of MessageTransferOperation should override GetResponseTimeoutSeconds()
+  // if they desire a different duration.
+  static uint32_t kDefaultResponseTimeoutSeconds;
+
+  void SetTimerFactoryForTest(
+      std::unique_ptr<TimerFactory> timer_factory_for_test);
+  void StartResponseTimerForDevice(
+      const cryptauth::RemoteDevice& remote_device);
+  void StopResponseTimerForDeviceIfRunning(
+      const cryptauth::RemoteDevice& remote_device);
+  void OnResponseTimeout(const cryptauth::RemoteDevice& remote_device);
+
   std::vector<cryptauth::RemoteDevice> remote_devices_;
   BleConnectionManager* connection_manager_;
+  std::unique_ptr<TimerFactory> timer_factory_;
 
   bool initialized_;
   std::map<cryptauth::RemoteDevice, uint32_t>
       remote_device_to_num_attempts_map_;
+  std::map<cryptauth::RemoteDevice, std::unique_ptr<base::Timer>>
+      remote_device_to_timer_map_;
+  base::WeakPtrFactory<MessageTransferOperation> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(MessageTransferOperation);
 };