[chromecast][BLE] Queue concurrent Connect requests

The Bluetooth stack will drop a Connect request while there is a
Connect request pending. The request's callback is never called.

This CL queues concurrent Connect requests to avoid this issue.

Bug: internal 111887604
Test: cast_bluetooth_unittests
      Try concurrent Connect requests, second request returned
      successfully.

Change-Id: I187330f94f75c643c896549d49f8e00a7ff3e195
Reviewed-on: https://chromium-review.googlesource.com/1155998Reviewed-by: Bailey Forrest <bcf@chromium.org>
Commit-Queue: Tiansong Cui <tiansong@google.com>
Cr-Commit-Position: refs/heads/master@{#579436}

chromecast/device/bluetooth/le/gatt_client_manager_impl.cc

@@ -8,6 +8,7 @@
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
+#include "base/stl_util.h"
 #include "chromecast/base/bind_to_task_runner.h"
 #include "chromecast/device/bluetooth/le/remote_characteristic_impl.h"
 #include "chromecast/device/bluetooth/le/remote_descriptor_impl.h"
@@ -118,9 +119,21 @@ void GattClientManagerImpl::NotifyConnect(
   observers_->Notify(FROM_HERE, &Observer::OnConnectInitated, addr);
 }
 
+void GattClientManagerImpl::EnqueueConnectRequest(
+    const bluetooth_v2_shlib::Addr& addr) {
+  DCHECK(io_task_runner_->BelongsToCurrentThread());
+  pending_connect_requests_.push_back(addr);
+
+  // Run the request if this is the only request in the queue. Otherwise, it
+  // will be run when all previous requests complete.
+  if (pending_connect_requests_.size() == 1) {
+    RunQueuedConnectRequest();
+  }
+}
+
 void GattClientManagerImpl::EnqueueReadRemoteRssiRequest(
     const bluetooth_v2_shlib::Addr& addr) {
-  MAKE_SURE_IO_THREAD(EnqueueReadRemoteRssiRequest, addr);
+  DCHECK(io_task_runner_->BelongsToCurrentThread());
   pending_read_remote_rssi_requests_.push_back(addr);
 
   // Run the request if this is the only request in the queue. Otherwise, it
@@ -152,10 +165,14 @@ void GattClientManagerImpl::OnConnectChanged(
     connected_devices_.insert(addr);
   } else {
     connected_devices_.erase(addr);
-    pending_read_remote_rssi_requests_.erase(
-        std::remove(pending_read_remote_rssi_requests_.begin(),
-                    pending_read_remote_rssi_requests_.end(), addr),
-        pending_read_remote_rssi_requests_.end());
+    if (addr == pending_connect_requests_.front()) {
+      pending_connect_requests_.pop_front();
+      RunQueuedConnectRequest();
+    } else {
+      base::Erase(pending_connect_requests_, addr);
+    }
+
+    base::Erase(pending_read_remote_rssi_requests_, addr);
   }
 
   // We won't declare the device connected until service discovery completes.
@@ -236,16 +253,15 @@ void GattClientManagerImpl::OnReadRemoteRssi(
   CHECK_DEVICE_EXISTS_IT(it);
   it->second->OnReadRemoteRssiComplete(status, rssi);
 
-  if (pending_read_remote_rssi_requests_.empty()) {
+  if (pending_read_remote_rssi_requests_.empty() ||
+      addr != pending_read_remote_rssi_requests_.front()) {
     NOTREACHED() << "Unexpected call to " << __func__;
-  } else {
-    pending_read_remote_rssi_requests_.pop_front();
+    return;
   }
 
-  // Run the next request if there is one in the queue.
-  if (!pending_read_remote_rssi_requests_.empty()) {
+  pending_read_remote_rssi_requests_.pop_front();
+  // Try to run the next ReadRemoteRssi request
   RunQueuedReadRemoteRssiRequest();
-  }
 }
 
 void GattClientManagerImpl::OnMtuChanged(const bluetooth_v2_shlib::Addr& addr,
@@ -275,6 +291,16 @@ void GattClientManagerImpl::OnGetServices(
 
   observers_->Notify(FROM_HERE, &Observer::OnServicesUpdated, it->second,
                      it->second->GetServicesSync());
+
+  if (pending_connect_requests_.empty() ||
+      addr != pending_connect_requests_.front()) {
+    NOTREACHED() << "Unexpected call to " << __func__;
+    return;
+  }
+
+  pending_connect_requests_.pop_front();
+  // Try to run the next Connect request
+  RunQueuedConnectRequest();
 }
 
 void GattClientManagerImpl::OnServicesRemoved(
@@ -301,9 +327,37 @@ void GattClientManagerImpl::OnServicesAdded(
                      it->second->GetServicesSync());
 }
 
+void GattClientManagerImpl::RunQueuedConnectRequest() {
+  DCHECK(io_task_runner_->BelongsToCurrentThread());
+
+  if (pending_connect_requests_.empty()) {
+    return;
+  }
+
+  auto addr = pending_connect_requests_.front();
+  while (!gatt_client_->Connect(addr)) {
+    // If current request fails, run the next request
+    LOG(ERROR) << "Connect failed";
+    auto it = addr_to_device_.find(addr);
+    if (it != addr_to_device_.end()) {
+      it->second->SetConnected(false);
+    }
+    pending_connect_requests_.pop_front();
+
+    if (pending_connect_requests_.empty()) {
+      return;
+    }
+
+    addr = pending_connect_requests_.front();
+  }
+}
+
 void GattClientManagerImpl::RunQueuedReadRemoteRssiRequest() {
   DCHECK(io_task_runner_->BelongsToCurrentThread());
-  DCHECK(!pending_read_remote_rssi_requests_.empty());
+
+  if (pending_read_remote_rssi_requests_.empty()) {
+    return;
+  }
 
   auto addr = pending_read_remote_rssi_requests_.front();
   while (!gatt_client_->ReadRemoteRssi(addr)) {

chromecast/device/bluetooth/le/gatt_client_manager_impl.h

@@ -43,6 +43,9 @@ class GattClientManagerImpl
   void NotifyConnect(const bluetooth_v2_shlib::Addr& addr) override;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner() override;
 
+  // Add a Connect request to the queue. They can only be executed serially.
+  void EnqueueConnectRequest(const bluetooth_v2_shlib::Addr& addr);
+
   // Add a ReadRemoteRssi request to the queue. They can only be executed
   // serially.
   void EnqueueReadRemoteRssiRequest(const bluetooth_v2_shlib::Addr& addr);
@@ -89,6 +92,7 @@ class GattClientManagerImpl
       const bluetooth_v2_shlib::Addr& addr,
       const std::vector<bluetooth_v2_shlib::Gatt::Service>& services) override;
 
+  void RunQueuedConnectRequest();
   void RunQueuedReadRemoteRssiRequest();
 
   static void FinalizeOnIoThread(
@@ -108,6 +112,9 @@ class GattClientManagerImpl
       addr_to_device_;
   std::set<bluetooth_v2_shlib::Addr> connected_devices_;
 
+  // Queue for concurrent Connect requests.
+  std::deque<bluetooth_v2_shlib::Addr> pending_connect_requests_;
+
   // Queue for concurrent ReadRemoteRssi requests.
   std::deque<bluetooth_v2_shlib::Addr> pending_read_remote_rssi_requests_;
 

chromecast/device/bluetooth/le/gatt_client_manager_impl_test.cc

@@ -33,6 +33,8 @@ const bluetooth_v2_shlib::Addr kTestAddr2 = {
     {0x10, 0x11, 0x12, 0x13, 0x14, 0x15}};
 const bluetooth_v2_shlib::Addr kTestAddr3 = {
     {0x20, 0x21, 0x22, 0x23, 0x24, 0x25}};
+const bluetooth_v2_shlib::Addr kTestAddr4 = {
+    {0x30, 0x31, 0x32, 0x33, 0x34, 0x35}};
 
 class MockGattClientManagerObserver : public GattClientManager::Observer {
  public:
@@ -238,6 +240,66 @@ TEST_F(GattClientManagerTest, RemoteDeviceConnect) {
   base::RunLoop().RunUntilIdle();
 }
 
+TEST_F(GattClientManagerTest, RemoteDeviceConnectConcurrent) {
+  bluetooth_v2_shlib::Gatt::Client::Delegate* delegate =
+      gatt_client_->delegate();
+  scoped_refptr<RemoteDevice> device1 = GetDevice(kTestAddr1);
+  scoped_refptr<RemoteDevice> device2 = GetDevice(kTestAddr2);
+  scoped_refptr<RemoteDevice> device3 = GetDevice(kTestAddr3);
+  scoped_refptr<RemoteDevice> device4 = GetDevice(kTestAddr4);
+
+  base::MockCallback<RemoteDevice::StatusCallback> cb1;
+  base::MockCallback<RemoteDevice::StatusCallback> cb2;
+  base::MockCallback<RemoteDevice::StatusCallback> cb3;
+  base::MockCallback<RemoteDevice::StatusCallback> cb4;
+
+  // Only the 1st Connect request will be executed immediately. The rest will be
+  // queued.
+  EXPECT_CALL(*gatt_client_, Connect(kTestAddr1)).WillOnce(Return(true));
+  device1->Connect(cb1.Get());
+  device2->Connect(cb2.Get());
+  device3->Connect(cb3.Get());
+  device4->Connect(cb4.Get());
+
+  EXPECT_CALL(*gatt_client_, GetServices(kTestAddr1)).WillOnce(Return(true));
+  delegate->OnConnectChanged(kTestAddr1, true /* status */,
+                             true /* connected */);
+
+  // Queued Connect requests will not be called until we receive OnGetServices
+  // of the current Connect request if it is successful.
+  EXPECT_CALL(cb1, Run(true));
+  EXPECT_CALL(*gatt_client_, Connect(kTestAddr2)).WillOnce(Return(false));
+  EXPECT_CALL(cb2, Run(false));
+  // If the Connect request fails in the initial request (not in the callback),
+  // the next queued request will be executed immediately.
+  EXPECT_CALL(*gatt_client_, Connect(kTestAddr3)).WillOnce(Return(true));
+  delegate->OnGetServices(kTestAddr1, {});
+
+  EXPECT_CALL(cb3, Run(false));
+  EXPECT_CALL(*gatt_client_, Connect(kTestAddr4)).WillOnce(Return(true));
+  delegate->OnConnectChanged(kTestAddr3, true /* status */,
+                             false /* connected */);
+
+  EXPECT_CALL(*gatt_client_, GetServices(kTestAddr4)).WillOnce(Return(true));
+  delegate->OnConnectChanged(kTestAddr4, true /* status */,
+                             true /* connected */);
+
+  EXPECT_CALL(cb4, Run(true));
+  delegate->OnGetServices(kTestAddr4, {});
+
+  EXPECT_TRUE(device1->IsConnected());
+  EXPECT_FALSE(device2->IsConnected());
+  EXPECT_FALSE(device3->IsConnected());
+  EXPECT_TRUE(device4->IsConnected());
+
+  base::MockCallback<base::OnceCallback<void(size_t)>>
+      get_num_connected_callback;
+  EXPECT_CALL(get_num_connected_callback, Run(2));
+  gatt_client_manager_->GetNumConnected(get_num_connected_callback.Get());
+
+  base::RunLoop().RunUntilIdle();
+}
+
 TEST_F(GattClientManagerTest, RemoteDeviceReadRssi) {
   static const int kRssi = -34;
 
@@ -269,20 +331,25 @@ TEST_F(GattClientManagerTest, RemoteDeviceReadRssiConcurrent) {
   base::MockCallback<RemoteDevice::RssiCallback> rssi_cb2;
   base::MockCallback<RemoteDevice::RssiCallback> rssi_cb3;
 
+  // Only the 1st ReadRemoteRssi request will be executed immediately. The rest
+  // will be queued.
   EXPECT_CALL(*gatt_client_, ReadRemoteRssi(kTestAddr1)).WillOnce(Return(true));
-  EXPECT_CALL(*gatt_client_, ReadRemoteRssi(kTestAddr2))
-      .WillOnce(Return(false));
-  EXPECT_CALL(*gatt_client_, ReadRemoteRssi(kTestAddr3)).WillOnce(Return(true));
-
   device1->ReadRemoteRssi(rssi_cb1.Get());
   device2->ReadRemoteRssi(rssi_cb2.Get());
   device3->ReadRemoteRssi(rssi_cb3.Get());
 
+  // Queued ReadRemoteRssi requests will not be called until we receive
+  // OnGetServices of the current Connect request if it is successful.
   EXPECT_CALL(rssi_cb1, Run(true, kRssi1));
+  EXPECT_CALL(*gatt_client_, ReadRemoteRssi(kTestAddr2))
+      .WillOnce(Return(false));
   EXPECT_CALL(rssi_cb2, Run(false, _));
-  EXPECT_CALL(rssi_cb3, Run(true, kRssi3));
-
+  // If the ReadRemoteRssi request fails in the initial request (not in the
+  // callback), the next queued request will be executed immediately.
+  EXPECT_CALL(*gatt_client_, ReadRemoteRssi(kTestAddr3)).WillOnce(Return(true));
   delegate->OnReadRemoteRssi(kTestAddr1, true, kRssi1);
+
+  EXPECT_CALL(rssi_cb3, Run(true, kRssi3));
   delegate->OnReadRemoteRssi(kTestAddr3, true, kRssi3);
 }
 

chromecast/device/bluetooth/le/remote_device_impl.cc

@@ -85,11 +85,10 @@ bool RemoteDeviceImpl::ConnectSync() {
   }
 
   gatt_client_manager_->NotifyConnect(addr_);
-  if (!gatt_client_manager_->gatt_client()->Connect(addr_)) {
-    LOG(ERROR) << __func__ << " failed";
-    return false;
-  }
+
   connect_pending_ = true;
+  gatt_client_manager_->EnqueueConnectRequest(addr_);
+
   return true;
 }