Fix ACL Create Connection Cancel (798ed5e0) · Commits · e / os / android_packages_modules_Bluetooth

system/gd/hci/Android.bp

+1 −0

Original line number	Diff line number	Diff line
		@@ -38,6 +38,7 @@ filegroup {
		"acl_manager/classic_acl_connection_test.cc",
		"acl_builder_test.cc",
		"acl_manager_unittest.cc",
		"acl_manager/acl_scheduler_test.cc",
		"address_unittest.cc",
		"address_with_type_test.cc",
		"class_of_device_unittest.cc",

system/gd/hci/acl_manager/acl_scheduler.cc

+21 −4

Original line number	Diff line number	Diff line
		@@ -31,7 +31,7 @@ struct AclCreateConnectionQueueEntry {

		struct AclScheduler::impl {
		void EnqueueOutgoingAclConnection(Address address, common::ContextualOnceCallback<void()> start_connection) {
		pending_outgoing_connections_.push({address, std::move(start_connection)});
		pending_outgoing_connections_.push_back({address, std::move(start_connection)});
		try_dequeue_next_connection();
		}

		@@ -69,7 +69,24 @@ struct AclScheduler::impl {
		Address address,
		common::ContextualOnceCallback<void()> cancel_connection,
		common::ContextualOnceCallback<void()> cancel_connection_completed) {
		// Check if relevant connection is currently outgoing
		if (outgoing_connecting_address_ == address) {
		cancel_connection.Invoke();
		// as per method contract, we don't clear it from the queue
		return;
		}

		// Otherwise, clear from the queue
		auto it =
		std::find_if(pending_outgoing_connections_.begin(), pending_outgoing_connections_.end(), [&](auto& entry) {
		return entry.address == address;
		});
		if (it == pending_outgoing_connections_.end()) {
		LOG_ERROR("Attempted to cancel connection to %s that does not exist", address.ToString().c_str());
		return;
		}
		pending_outgoing_connections_.erase(it);
		cancel_connection_completed.Invoke();
		}

		void Stop() {
		@@ -85,7 +102,7 @@ struct AclScheduler::impl {
		!pending_outgoing_connections_.empty()) {
		LOG_INFO("Pending connections is not empty; so sending next connection");
		auto entry = std::move(pending_outgoing_connections_.front());
		pending_outgoing_connections_.pop();
		pending_outgoing_connections_.pop_front();
		outgoing_connecting_address_ = entry.address;
		entry.callback.Invoke();
		}
		@@ -98,7 +115,7 @@ struct AclScheduler::impl {
		}

		Address outgoing_connecting_address_;
		std::queue<AclCreateConnectionQueueEntry> pending_outgoing_connections_;
		std::deque<AclCreateConnectionQueueEntry> pending_outgoing_connections_;
		std::unordered_set<Address> incoming_connecting_address_set_;
		bool stopped_ = false;
		};

system/gd/hci/acl_manager/acl_scheduler_test.cc

0 → 100644

+365 −0

Original line number	Diff line number	Diff line
		/*
		* Copyright 2022 The Android Open Source Project
		*
		* Licensed under the Apache License, Version 2.0 (the "License");
		* you may not use this file except in compliance with the License.
		* You may obtain a copy of the License at
		*
		* http://www.apache.org/licenses/LICENSE-2.0
		*
		* Unless required by applicable law or agreed to in writing, software
		* distributed under the License is distributed on an "AS IS" BASIS,
		* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
		* See the License for the specific language governing permissions and
		* limitations under the License.
		*/

		#include "hci/acl_manager/acl_scheduler.h"

		#include <gmock/gmock.h>
		#include <gtest/gtest.h>

		#include <algorithm>
		#include <chrono>
		#include <future>
		#include <map>

		#include "common/bind.h"
		#include "common/init_flags.h"
		#include "hci/address.h"
		#include "os/thread.h"

		namespace bluetooth {
		namespace hci {
		namespace acl_manager {
		namespace {

		const auto address1 = Address::FromString("A1:A2:A3:A4:A5:A6").value();
		const auto address2 = Address::FromString("B1:B2:B3:B4:B5:B6").value();
		const auto address3 = Address::FromString("C1:C2:C3:C4:C5:C6").value();

		const auto timeout = std::chrono::milliseconds(100);

		MATCHER(IsSet, "Future is set") {
		if (arg.wait_for(timeout) != std::future_status::ready) {
		return false;
		}
		const_cast<std::future<void>&>(arg).get();
		return true;
		}

		class AclSchedulerTest : public ::testing::Test {
		protected:
		void SetUp() override {
		fake_registry_.Start<AclScheduler>(&thread_);
		ASSERT_TRUE(fake_registry_.IsStarted<AclScheduler>());

		client_handler_ = fake_registry_.GetTestModuleHandler(&AclScheduler::Factory);
		ASSERT_NE(client_handler_, nullptr);

		acl_scheduler_ = static_cast<AclScheduler*>(fake_registry_.GetModuleUnderTest(&AclScheduler::Factory));

		::testing::FLAGS_gtest_death_test_style = "threadsafe";
		}

		void TearDown() override {
		fake_registry_.SynchronizeModuleHandler(&AclScheduler::Factory, timeout);
		fake_registry_.StopAll();
		}

		common::ContextualOnceCallback<void(std::string)> impossibleCallbackTakingString() {
		return client_handler_->BindOnce([](std::string _) { ADD_FAILURE(); });
		}

		common::ContextualOnceCallback<void(std::string)> emptyCallbackTakingString() {
		return client_handler_->BindOnce([](std::string _) {});
		}

		common::ContextualOnceCallback<void(std::string)> promiseCallbackTakingString(std::promise<void> promise) {
		return client_handler_->BindOnce(
		[](std::promise<void> promise, std::string _) { promise.set_value(); }, std::move(promise));
		}

		common::ContextualOnceCallback<void()> impossibleCallback() {
		return client_handler_->BindOnce([] { ADD_FAILURE(); });
		}

		common::ContextualOnceCallback<void()> emptyCallback() {
		return client_handler_->BindOnce([] {});
		}

		common::ContextualOnceCallback<void()> promiseCallback(std::promise<void> promise) {
		return client_handler_->BindOnce([](std::promise<void> promise) { promise.set_value(); }, std::move(promise));
		}

		TestModuleRegistry fake_registry_;
		os::Thread& thread_ = fake_registry_.GetTestThread();
		AclScheduler* acl_scheduler_ = nullptr;
		os::Handler* client_handler_ = nullptr;
		};

		TEST_F(AclSchedulerTest, SingleConnectionImmediatelyExecuted) {
		auto promise = std::promise<void>{};
		auto future = promise.get_future();

		// start connection, which should immediately execute
		acl_scheduler_->EnqueueOutgoingAclConnection(address1, promiseCallback(std::move(promise)));

		// it has started
		EXPECT_THAT(future, IsSet());
		}

		TEST_F(AclSchedulerTest, ThreeConnectionsQueue) {
		auto promise1 = std::promise<void>{};
		auto future1 = promise1.get_future();
		auto promise2 = std::promise<void>{};
		auto future2 = promise2.get_future();

		// start first connection, which immediately runs
		acl_scheduler_->EnqueueOutgoingAclConnection(address1, emptyCallback());
		// start second connection
		acl_scheduler_->EnqueueOutgoingAclConnection(address2, (promiseCallback(std::move(promise1))));
		// start third connection
		acl_scheduler_->EnqueueOutgoingAclConnection(address3, (promiseCallback(std::move(promise2))));

		// the second and third connections are currently queued
		EXPECT_THAT(future1.wait_for(timeout), std::future_status::timeout);

		// first connection fails, so next one should start
		acl_scheduler_->ReportOutgoingAclConnectionFailure();

		// the second connection has started, the third one is queued
		EXPECT_THAT(future1, IsSet());
		EXPECT_THAT(future2.wait_for(timeout), std::future_status::timeout);

		// second connection fails, so third one should start
		acl_scheduler_->ReportOutgoingAclConnectionFailure();

		// the third connection has started
		EXPECT_THAT(future2, IsSet());
		}

		TEST_F(AclSchedulerTest, SingleConnectionCompletionCallback) {
		auto promise = std::promise<void>{};
		auto future = promise.get_future();

		// start connection, which immediately runs
		acl_scheduler_->EnqueueOutgoingAclConnection(address1, emptyCallback());

		// the outgoing connection completes
		acl_scheduler_->ReportAclConnectionCompletion(
		address1, promiseCallback(std::move(promise)), impossibleCallback(), impossibleCallbackTakingString());

		// the outgoing_connection callback should have executed
		EXPECT_THAT(future, IsSet());
		}

		TEST_F(AclSchedulerTest, SingleConnectionCompletionDequeueNext) {
		auto promise = std::promise<void>{};
		auto future = promise.get_future();

		// start connection, which immediately runs
		acl_scheduler_->EnqueueOutgoingAclConnection(address1, emptyCallback());
		// start second connection which should queue
		acl_scheduler_->EnqueueOutgoingAclConnection(address2, promiseCallback(std::move(promise)));

		// complete the first connection
		acl_scheduler_->ReportAclConnectionCompletion(
		address1, emptyCallback(), impossibleCallback(), impossibleCallbackTakingString());

		// the next connection should dequeue now
		EXPECT_THAT(future, IsSet());
		}

		TEST_F(AclSchedulerTest, IncomingConnectionCallback) {
		auto promise = std::promise<void>{};
		auto future = promise.get_future();

		// an incoming connection arrives
		acl_scheduler_->RegisterPendingIncomingConnection(address1);

		// and completes
		acl_scheduler_->ReportAclConnectionCompletion(
		address1, impossibleCallback(), promiseCallback(std::move(promise)), impossibleCallbackTakingString());

		// the incoming_connection callback should have executed
		EXPECT_THAT(future, IsSet());
		}

		TEST_F(AclSchedulerTest, UnknownConnectionCallback) {
		auto promise = std::promise<void>{};
		auto future = promise.get_future();

		// start outgoing connection
		acl_scheduler_->EnqueueOutgoingAclConnection(address1, emptyCallback());

		// an incoming connection arrives
		acl_scheduler_->RegisterPendingIncomingConnection(address2);

		// then an unknown connection completes
		acl_scheduler_->ReportAclConnectionCompletion(
		address3, impossibleCallback(), impossibleCallback(), (promiseCallbackTakingString(std::move(promise))));

		// the unknown_connection callback should have executed
		EXPECT_THAT(future, IsSet());
		}

		TEST_F(AclSchedulerTest, TiebreakForOutgoingConnection) {
		auto promise = std::promise<void>{};
		auto future = promise.get_future();

		// start outgoing connection
		acl_scheduler_->EnqueueOutgoingAclConnection(address1, emptyCallback());

		// an incoming connection arrives from the same address
		acl_scheduler_->RegisterPendingIncomingConnection(address1);

		// then the connection to that address completes
		acl_scheduler_->ReportAclConnectionCompletion(
		address1, promiseCallback(std::move(promise)), impossibleCallback(), impossibleCallbackTakingString());

		// the outgoing_connection callback should have executed, NOT the incoming_connection one
		// this preserves working behavior, it is not based on any principled decision (so if you need to break this test,
		// go for it)
		EXPECT_THAT(future, IsSet());
		}

		TEST_F(AclSchedulerTest, QueueWhileIncomingConnectionsPending) {
		auto promise = std::promise<void>{};
		auto future = promise.get_future();

		// start outgoing connection
		acl_scheduler_->EnqueueOutgoingAclConnection(address1, emptyCallback());
		// queue a second outgoing connection
		acl_scheduler_->EnqueueOutgoingAclConnection(address2, promiseCallback(std::move(promise)));

		// an incoming connection arrives
		acl_scheduler_->RegisterPendingIncomingConnection(address3);

		// then the first outgoing connection completes
		acl_scheduler_->ReportAclConnectionCompletion(
		address1, emptyCallback(), impossibleCallback(), impossibleCallbackTakingString());

		// the outgoing_connection callback should not have executed yet
		EXPECT_THAT(future.wait_for(timeout), std::future_status::timeout);

		// now the incoming connection completes
		acl_scheduler_->ReportAclConnectionCompletion(
		address3, impossibleCallback(), emptyCallback(), impossibleCallbackTakingString());

		// only now does the next outgoing connection start
		EXPECT_THAT(future, IsSet());
		}

		TEST_F(AclSchedulerTest, DoNothingWhileIncomingConnectionsExist) {
		auto promise = std::promise<void>{};
		auto future = promise.get_future();

		// an incoming connection arrives
		acl_scheduler_->RegisterPendingIncomingConnection(address1);

		// try to start an outgoing connection
		acl_scheduler_->EnqueueOutgoingAclConnection(address2, promiseCallback(std::move(promise)));

		// the outgoing_connection callback should not have executed yet
		EXPECT_THAT(future.wait_for(timeout), std::future_status::timeout);

		// a second incoming connection arrives
		acl_scheduler_->RegisterPendingIncomingConnection(address3);

		// the first incoming connection completes
		acl_scheduler_->ReportAclConnectionCompletion(
		address1, impossibleCallback(), emptyCallback(), impossibleCallbackTakingString());

		// the outgoing_connection callback should still not have executed yet
		EXPECT_THAT(future.wait_for(timeout), std::future_status::timeout);

		// the second incoming connection completes, so none are left
		acl_scheduler_->ReportAclConnectionCompletion(
		address3, impossibleCallback(), emptyCallback(), impossibleCallbackTakingString());

		// only now does the outgoing connection start
		EXPECT_THAT(future, IsSet());
		}

		TEST_F(AclSchedulerTest, CancelOutgoingConnection) {
		auto promise = std::promise<void>{};
		auto future = promise.get_future();

		// start an outgoing connection
		acl_scheduler_->EnqueueOutgoingAclConnection(address1, emptyCallback());
		// enqueue a second connection
		acl_scheduler_->EnqueueOutgoingAclConnection(address2, promiseCallback(std::move(promise)));

		// cancel the outgoing connection
		acl_scheduler_->CancelAclConnection(address1, emptyCallback(), impossibleCallback());

		// we expect the second connection to stay queued until the cancel completes
		EXPECT_THAT(future.wait_for(timeout), std::future_status::timeout);

		// now the cancel completes (with a failed status, in reality, but the scheduler doesn't care)
		acl_scheduler_->ReportAclConnectionCompletion(
		address1, emptyCallback(), impossibleCallback(), impossibleCallbackTakingString());

		// so only now do we advance the queue
		EXPECT_THAT(future, IsSet());
		}

		TEST_F(AclSchedulerTest, CancelOutgoingConnectionCallback) {
		auto promise = std::promise<void>{};
		auto future = promise.get_future();

		// start an outgoing connection
		acl_scheduler_->EnqueueOutgoingAclConnection(address1, emptyCallback());

		// cancel the outgoing connection
		acl_scheduler_->CancelAclConnection(address1, promiseCallback(std::move(promise)), impossibleCallback());

		// we expect the cancel_connection callback to be invoked since we are cancelling an actually active connection
		EXPECT_THAT(future, IsSet());
		}

		TEST_F(AclSchedulerTest, CancelQueuedConnectionRemoveFromQueue) {
		auto promise = std::promise<void>{};
		auto future = promise.get_future();

		// start an outgoing connection
		acl_scheduler_->EnqueueOutgoingAclConnection(address1, emptyCallback());
		// start another connection that will queue
		acl_scheduler_->EnqueueOutgoingAclConnection(address2, impossibleCallback());
		// start a third connection that will queue
		acl_scheduler_->EnqueueOutgoingAclConnection(address3, promiseCallback(std::move(promise)));

		// cancel the first queued connection
		acl_scheduler_->CancelAclConnection(address2, impossibleCallback(), emptyCallback());

		// the second queued connection should remain enqueued, since another connection is in progress
		EXPECT_THAT(future.wait_for(timeout), std::future_status::timeout);

		// complete the outgoing connection
		acl_scheduler_->ReportOutgoingAclConnectionFailure();

		// only now can we dequeue the second queued connection
		EXPECT_THAT(future, IsSet());
		}

		TEST_F(AclSchedulerTest, CancelQueuedConnectionCallback) {
		auto promise = std::promise<void>{};
		auto future = promise.get_future();

		// start an outgoing connection
		acl_scheduler_->EnqueueOutgoingAclConnection(address1, emptyCallback());
		// start another connection that will queue
		acl_scheduler_->EnqueueOutgoingAclConnection(address2, emptyCallback());

		// cancel the queued connection
		acl_scheduler_->CancelAclConnection(address2, impossibleCallback(), promiseCallback(std::move(promise)));

		// we expect the cancel_connection_completed callback to be invoked since we are cancelling a connection in the queue
		EXPECT_THAT(future, IsSet());
		}

		} // namespace
		} // namespace acl_manager
		} // namespace hci
		} // namespace bluetooth

system/gd/hci/acl_manager/classic_impl.h

+4 −6

Original line number	Diff line number	Diff line
		@@ -398,15 +398,13 @@ struct classic_impl : public security::ISecurityManagerListener {

		void cancel_connect(Address address) {
		acl_scheduler_->CancelAclConnection(
		address, handler_->BindOnceOn(this, &classic_impl::actually_cancel_connect, address), {});
		address,
		handler_->BindOnceOn(this, &classic_impl::actually_cancel_connect, address),
		client_handler_->BindOnceOn(
		client_callbacks_, &ConnectionCallbacks::OnConnectFail, address, ErrorCode::UNKNOWN_CONNECTION));
		}

		void actually_cancel_connect(Address address) {
		// NOTE: this will be fixed in a followup CL, commenting out to avoid build failure
		// if (outgoing_connecting_address_ == address) {
		// LOG_INFO("Cannot cancel non-existent connection to %s", address.ToString().c_str());
		// return;
		// }
		std::unique_ptr<CreateConnectionCancelBuilder> packet = CreateConnectionCancelBuilder::Create(address);
		acl_connection_interface_->EnqueueCommand(
		std::move(packet), handler_->BindOnce(&check_command_complete<CreateConnectionCancelCompleteView>));