diff --git a/internal/platform/implementation/windows/ble_gatt_server.cc b/internal/platform/implementation/windows/ble_gatt_server.cc index 84c0d63f..89f5c4e9 100644 --- a/internal/platform/implementation/windows/ble_gatt_server.cc +++ b/internal/platform/implementation/windows/ble_gatt_server.cc @@ -26,6 +26,7 @@ #include #include "absl/container/flat_hash_map.h" +#include "absl/functional/any_invocable.h" #include "absl/log/check.h" #include "absl/status/status.h" #include "absl/strings/escaping.h" @@ -116,7 +117,9 @@ BleGattServer::BleGattServer(api::BluetoothAdapter* adapter, api::ble_v2::ServerGattConnectionCallback callback) : adapter_(dynamic_cast(adapter)), peripheral_(adapter_->GetMacAddress()), - gatt_connection_callback_(std::move(callback)) {} + gatt_connection_callback_(std::move(callback)) { + DCHECK(adapter_ != nullptr); +} absl::optional BleGattServer::CreateCharacteristic( @@ -204,28 +207,35 @@ absl::Status BleGattServer::NotifyCharacteristicChanged( } void BleGattServer::Stop() { - absl::MutexLock lock(&mutex_); - NEARBY_LOGS(VERBOSE) << __func__ << ": Start to stop GATT server."; - try { - if (gatt_service_provider_ == nullptr) { - NEARBY_LOGS(WARNING) << __func__ << ": GATT server already stopped."; - return; - } + absl::AnyInvocable close_notifier = nullptr; + { + absl::MutexLock lock(&mutex_); + NEARBY_LOGS(VERBOSE) << __func__ << ": Start to stop GATT server."; + if (gatt_service_provider_ != nullptr) { + try { + if (is_advertising_) { + gatt_service_provider_.StopAdvertising(); + } - if (is_advertising_) { - gatt_service_provider_.StopAdvertising(); + gatt_characteristic_datas_.clear(); + service_uuid_ = Uuid(); + gatt_service_provider_ = nullptr; + } catch (std::exception exception) { + NEARBY_LOGS(ERROR) << __func__ << ": Exception: " << exception.what(); + } catch (const winrt::hresult_error& error) { + NEARBY_LOGS(ERROR) << __func__ << ": WinRT exception: " << error.code() + << ": " << winrt::to_string(error.message()); + } catch (...) { + NEARBY_LOGS(ERROR) << __func__ << ": Unknown exception."; + } + } else { + NEARBY_LOGS(WARNING) << __func__ << ": no GATT server is running."; } + close_notifier = std::move(close_notifier_); + } - gatt_characteristic_datas_.clear(); - service_uuid_ = Uuid(); - gatt_service_provider_ = nullptr; - } catch (std::exception exception) { - NEARBY_LOGS(ERROR) << __func__ << ": Exception: " << exception.what(); - } catch (const winrt::hresult_error& error) { - NEARBY_LOGS(ERROR) << __func__ << ": WinRT exception: " << error.code() - << ": " << winrt::to_string(error.message()); - } catch (...) { - NEARBY_LOGS(ERROR) << __func__ << ": Unknown exception."; + if (close_notifier != nullptr) { + close_notifier(); } } @@ -252,6 +262,14 @@ bool BleGattServer::InitializeGattServer() { return false; } + if (!adapter_->IsPeripheralRoleSupported()) { + NEARBY_LOGS(ERROR) + << __func__ + << ": Bluetooth Hardware does not support Peripheral Role, which is " + "required to start GATT server."; + return false; + } + winrt::guid service_uuid = nearby_uuid_to_winrt_guid(service_uuid_); GattServiceProviderResult service_provider_result = GattServiceProvider::CreateAsync(service_uuid).get(); @@ -416,14 +434,6 @@ bool BleGattServer::StartAdvertisement(const ByteArray& service_data, return false; } - if (!adapter_->IsPeripheralRoleSupported()) { - NEARBY_LOGS(ERROR) - << __func__ - << ": Bluetooth Hardware does not support Peripheral Role, which is " - "required to start GATT server."; - return false; - } - // Start the GATT server advertising GattServiceProviderAdvertisingParameters advertisement_parameters; advertisement_parameters.IsConnectable(is_connectable); @@ -514,6 +524,11 @@ bool BleGattServer::StopAdvertisement() { return false; } +void BleGattServer::SetCloseNotifier(absl::AnyInvocable notifier) { + absl::MutexLock lock(&mutex_); + close_notifier_ = std::move(notifier); +} + ::winrt::fire_and_forget BleGattServer::Characteristic_ReadRequestedAsync( ::winrt::Windows::Devices::Bluetooth::GenericAttributeProfile:: GattLocalCharacteristic const& gatt_local_characteristic, diff --git a/internal/platform/implementation/windows/ble_gatt_server.h b/internal/platform/implementation/windows/ble_gatt_server.h index 324fa57f..89bafde3 100644 --- a/internal/platform/implementation/windows/ble_gatt_server.h +++ b/internal/platform/implementation/windows/ble_gatt_server.h @@ -22,6 +22,7 @@ #include "absl/base/thread_annotations.h" #include "absl/container/flat_hash_map.h" +#include "absl/functional/any_invocable.h" #include "absl/status/status.h" #include "absl/strings/string_view.h" #include "absl/synchronization/mutex.h" @@ -43,6 +44,7 @@ namespace windows { class BleGattServer : public api::ble_v2::GattServer { public: + // Make sure the adapter parameter is not null. BleGattServer(api::BluetoothAdapter* adapter, api::ble_v2::ServerGattConnectionCallback callback); ~BleGattServer() override = default; @@ -66,6 +68,9 @@ class BleGattServer : public api::ble_v2::GattServer { ABSL_LOCKS_EXCLUDED(mutex_); bool StopAdvertisement() ABSL_LOCKS_EXCLUDED(mutex_); + void SetCloseNotifier(absl::AnyInvocable notifier) + ABSL_LOCKS_EXCLUDED(mutex_); + api::ble_v2::BlePeripheral& GetBlePeripheral() override { return peripheral_; } @@ -86,53 +91,65 @@ class BleGattServer : public api::ble_v2::GattServer { ::winrt::event_token subscribed_clients_changed_token{}; }; - bool InitializeGattServer(); + bool InitializeGattServer() ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); void NotifyValueChanged( - const api::ble_v2::GattCharacteristic& gatt_characteristic); + const api::ble_v2::GattCharacteristic& gatt_characteristic) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); GattCharacteristicData* FindGattCharacteristicData( const ::winrt::Windows::Devices::Bluetooth::GenericAttributeProfile:: - GattLocalCharacteristic& gatt_local_characteristic); + GattLocalCharacteristic& gatt_local_characteristic) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); GattCharacteristicData* FindGattCharacteristicData( - const api::ble_v2::GattCharacteristic& gatt_characteristic); + const api::ble_v2::GattCharacteristic& gatt_characteristic) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); ::winrt::fire_and_forget Characteristic_ReadRequestedAsync( ::winrt::Windows::Devices::Bluetooth::GenericAttributeProfile:: GattLocalCharacteristic const& gatt_local_characteristic, ::winrt::Windows::Devices::Bluetooth::GenericAttributeProfile:: - GattReadRequestedEventArgs args); + GattReadRequestedEventArgs args) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); ::winrt::fire_and_forget Characteristic_WriteRequestedAsync( ::winrt::Windows::Devices::Bluetooth::GenericAttributeProfile:: GattLocalCharacteristic const& gatt_local_characteristic, ::winrt::Windows::Devices::Bluetooth::GenericAttributeProfile:: - GattWriteRequestedEventArgs args); + GattWriteRequestedEventArgs args) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); void Characteristic_SubscribedClientsChanged( ::winrt::Windows::Devices::Bluetooth::GenericAttributeProfile:: GattLocalCharacteristic const& gatt_local_characteristic, - ::winrt::Windows::Foundation::IInspectable const& args); + ::winrt::Windows::Foundation::IInspectable const& args) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); void ServiceProvider_AdvertisementStatusChanged( ::winrt::Windows::Devices::Bluetooth::GenericAttributeProfile:: GattServiceProvider const& sender, ::winrt::Windows::Devices::Bluetooth::GenericAttributeProfile:: - GattServiceProviderAdvertisementStatusChangedEventArgs const& args); + GattServiceProviderAdvertisementStatusChangedEventArgs const& args) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); absl::Mutex mutex_; - BluetoothAdapter* adapter_ = nullptr; + BluetoothAdapter* const adapter_ = nullptr; BleV2Peripheral peripheral_; - - ::winrt::Windows::Devices::Bluetooth::GenericAttributeProfile:: - GattServiceProvider gatt_service_provider_ = nullptr; - - Uuid service_uuid_; - std::vector gatt_characteristic_datas_; - api::ble_v2::ServerGattConnectionCallback gatt_connection_callback_{}; - ::winrt::event_token service_provider_advertisement_changed_token_{}; - bool is_advertising_ = false; - bool is_gatt_server_inited_ = false; + ::winrt::Windows::Devices::Bluetooth::GenericAttributeProfile:: + GattServiceProvider gatt_service_provider_ ABSL_GUARDED_BY(mutex_) = + nullptr; + + absl::AnyInvocable close_notifier_ ABSL_GUARDED_BY(mutex_) = nullptr; + + Uuid service_uuid_ ABSL_GUARDED_BY(mutex_); + std::vector gatt_characteristic_datas_ + ABSL_GUARDED_BY(mutex_); + + bool is_advertising_ ABSL_GUARDED_BY(mutex_) = false; + bool is_gatt_server_inited_ ABSL_GUARDED_BY(mutex_) = false; + + ::winrt::event_token service_provider_advertisement_changed_token_ + ABSL_GUARDED_BY(mutex_) = {}; }; } // namespace windows diff --git a/internal/platform/implementation/windows/ble_gatt_server_test.cc b/internal/platform/implementation/windows/ble_gatt_server_test.cc index 406b283b..b374ef09 100644 --- a/internal/platform/implementation/windows/ble_gatt_server_test.cc +++ b/internal/platform/implementation/windows/ble_gatt_server_test.cc @@ -14,10 +14,13 @@ #include "internal/platform/implementation/windows/ble_gatt_server.h" +#include #include +#include #include "gtest/gtest.h" #include "absl/synchronization/notification.h" +#include "absl/time/time.h" #include "internal/platform/implementation/ble_v2.h" #include "internal/platform/implementation/windows/bluetooth_adapter.h" @@ -44,6 +47,23 @@ TEST(BleV2GattServer, DISABLED_Stop) { blev2_gatt_server.Stop(); } +TEST(BleV2GattServer, DISABLED_StopNotifierIsCalled) { + BluetoothAdapter bluetoothAdapter; + BleGattServer blev2_gatt_server(&bluetoothAdapter, {}); + bool is_close_notifier_called = false; + absl::Notification notification; + std::function notifier = [&is_close_notifier_called, + ¬ification]() { + is_close_notifier_called = true; + notification.Notify(); + }; + blev2_gatt_server.SetCloseNotifier(std::move(notifier)); + + blev2_gatt_server.Stop(); + notification.WaitForNotificationWithTimeout(absl::Seconds(1)); + EXPECT_TRUE(is_close_notifier_called); +} + TEST(BleV2GattServer, DISABLED_CreateCharacteristic) { BluetoothAdapter bluetoothAdapter; BleGattServer blev2_gatt_server(&bluetoothAdapter, {}); diff --git a/internal/platform/implementation/windows/ble_v2.cc b/internal/platform/implementation/windows/ble_v2.cc index 73258997..be24f1a5 100644 --- a/internal/platform/implementation/windows/ble_v2.cc +++ b/internal/platform/implementation/windows/ble_v2.cc @@ -16,6 +16,7 @@ #include #include +#include #include #include #include @@ -29,6 +30,7 @@ #include "absl/strings/numbers.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_format.h" +#include "absl/strings/string_view.h" #include "absl/synchronization/mutex.h" #include "absl/time/clock.h" #include "absl/time/time.h" @@ -320,6 +322,7 @@ bool BleV2Medium::StartScanning(const Uuid& service_uuid, std::unique_ptr BleV2Medium::StartScanning( const Uuid& service_uuid, TxPowerLevel tx_power_level, BleV2Medium::ScanningCallback callback) { + absl::MutexLock lock(&mutex_); NEARBY_LOGS(INFO) << __func__ << ": service UUID: " << std::string(service_uuid) << ", TxPowerLevel: " << TxPowerLevelToName(tx_power_level); @@ -371,16 +374,12 @@ std::unique_ptr BleV2Medium::StartScanning( } uint64_t session_id = GenerateSessionId(); - // Save session id, service id and callback for this scan session. - { - absl::MutexLock lock(&map_mutex_); - auto iter = service_uuid_to_session_map_.find(service_uuid); - if (iter == service_uuid_to_session_map_.end()) { - service_uuid_to_session_map_[service_uuid].insert( - {session_id, std::move(callback)}); - } else { - iter->second.insert({session_id, std::move(callback)}); - } + auto iter = service_uuid_to_session_map_.find(service_uuid); + if (iter == service_uuid_to_session_map_.end()) { + service_uuid_to_session_map_[service_uuid].insert( + {session_id, std::move(callback)}); + } else { + iter->second.insert({session_id, std::move(callback)}); } // Generate and return ScanningSession. @@ -390,7 +389,7 @@ std::unique_ptr BleV2Medium::StartScanning( [this, session_id, service_uuid]() { size_t num_erased_from_service_and_session_map = 0u; { - absl::MutexLock lock(&map_mutex_); + absl::MutexLock lock(&mutex_); auto iter = service_uuid_to_session_map_.find(service_uuid); if (iter != service_uuid_to_session_map_.end()) { num_erased_from_service_and_session_map = @@ -462,6 +461,14 @@ std::unique_ptr BleV2Medium::StartGattServer( std::make_unique(adapter_, std::move(callback)); ble_gatt_server_ = gatt_server.get(); + ble_gatt_server_->SetCloseNotifier([this]() { + // In avoid to create a new thread to close the gatt server, we don't + // acquire the mutex here. The calling flow may cause deadlock due to + // StartGattAdvertising may run into the codes. It is not ideal, but it is + // hard to run in thread issues. + NEARBY_LOGS(INFO) << __func__ << ": GATT server is closed."; + ble_gatt_server_ = nullptr; + }); return gatt_server; } @@ -937,13 +944,13 @@ void BleV2Medium::PublisherHandler( case BluetoothLEAdvertisementPublisherStatus::Aborted: switch (args.Error()) { case BluetoothError::Success: - if (publisher_.Status() == + if (publisher.Status() == BluetoothLEAdvertisementPublisherStatus::Started) { NEARBY_LOGS(ERROR) << "Nearby BLE Medium start advertising operation was " "successfully completed or serviced."; } - if (publisher_.Status() == + if (publisher.Status() == BluetoothLEAdvertisementPublisherStatus::Stopped) { NEARBY_LOGS(ERROR) << "Nearby BLE Medium stop advertising operation was " @@ -1088,12 +1095,15 @@ void BleV2Medium::AdvertisementReceivedHandler( std::string bluetooth_address = uint64_to_mac_address_string(args.BluetoothAddress()); - BleV2Peripheral* peripheral_ptr = - GetOrCreatePeripheral(bluetooth_address); - if (peripheral_ptr == nullptr) { - NEARBY_LOGS(ERROR) << "No BLE peripheral with address: " - << bluetooth_address; - return; + BleV2Peripheral* peripheral_ptr = nullptr; + { + absl::MutexLock lock(&mutex_); + peripheral_ptr = GetOrCreatePeripheral(bluetooth_address); + if (peripheral_ptr == nullptr) { + NEARBY_LOGS(ERROR) + << "No BLE peripheral with address: " << bluetooth_address; + return; + } } NEARBY_LOGS(INFO) << "BLE peripheral with address: " << bluetooth_address; @@ -1123,7 +1133,7 @@ void BleV2Medium::AdvertisementFoundHandler( std::vector service_uuid_list; bool found_matching_service_uuid = false; { - absl::MutexLock lock(&map_mutex_); + absl::MutexLock lock(&mutex_); for (auto windows_service_uuid : advertisement.ServiceUuids()) { auto nearby_service_uuid = winrt_guid_to_nearby_uuid(windows_service_uuid); @@ -1176,18 +1186,22 @@ void BleV2Medium::AdvertisementFoundHandler( // Save the BleV2Peripheral. std::string bluetooth_address = uint64_to_mac_address_string(args.BluetoothAddress()); - BleV2Peripheral* peripheral_ptr = GetOrCreatePeripheral(bluetooth_address); - if (peripheral_ptr == nullptr) { - NEARBY_LOGS(ERROR) << "No BLE peripheral with address: " - << bluetooth_address; - return; + BleV2Peripheral* peripheral_ptr = nullptr; + { + absl::MutexLock lock(&mutex_); + peripheral_ptr = GetOrCreatePeripheral(bluetooth_address); + if (peripheral_ptr == nullptr) { + NEARBY_LOGS(ERROR) << "No BLE peripheral with address: " + << bluetooth_address; + return; + } } NEARBY_LOGS(INFO) << "BLE peripheral with address: " << bluetooth_address; // Invokes callbacks that matches the UUID. for (auto service_uuid : service_uuid_list) { { - absl::MutexLock lock(&map_mutex_); + absl::MutexLock lock(&mutex_); if (service_uuid_to_session_map_.find(service_uuid) != service_uuid_to_session_map_.end()) { for (auto& id_session_pair : @@ -1202,6 +1216,7 @@ void BleV2Medium::AdvertisementFoundHandler( bool BleV2Medium::GetRemotePeripheral(const std::string& mac_address, GetRemotePeripheralCallback callback) { + absl::MutexLock lock(&mutex_); BleV2Peripheral* peripheral = GetOrCreatePeripheral(mac_address); if (peripheral != nullptr && peripheral->Ok()) { callback(*peripheral); @@ -1212,6 +1227,7 @@ bool BleV2Medium::GetRemotePeripheral(const std::string& mac_address, bool BleV2Medium::GetRemotePeripheral(api::ble_v2::BlePeripheral::UniqueId id, GetRemotePeripheralCallback callback) { + absl::MutexLock lock(&mutex_); BleV2Peripheral* peripheral = GetPeripheral(id); if (peripheral == nullptr) { NEARBY_LOGS(WARNING) << __func__ << ": No matched peripheral device."; @@ -1222,7 +1238,6 @@ bool BleV2Medium::GetRemotePeripheral(api::ble_v2::BlePeripheral::UniqueId id, } uint64_t BleV2Medium::GenerateSessionId() { - absl::MutexLock lock(&map_mutex_); for (int i = 0; i < kGenerateSessionIdRetryLimit; i++) { uint64_t session_id = Prng().NextInt64(); if (session_id == kFailedGenerateSessionId) continue; @@ -1235,7 +1250,6 @@ uint64_t BleV2Medium::GenerateSessionId() { } BleV2Peripheral* BleV2Medium::GetOrCreatePeripheral(absl::string_view address) { - absl::MutexLock lock(&peripheral_map_mutex_); auto it = std::find_if( peripheral_map_.begin(), peripheral_map_.end(), [&](const auto& item) { return item.second.peripheral->GetAddress() == address; @@ -1261,7 +1275,6 @@ BleV2Peripheral* BleV2Medium::GetOrCreatePeripheral(absl::string_view address) { } BleV2Peripheral* BleV2Medium::GetPeripheral(BleV2Peripheral::UniqueId id) { - absl::MutexLock lock(&peripheral_map_mutex_); auto it = peripheral_map_.find(id); if (it == peripheral_map_.end()) { return nullptr; diff --git a/internal/platform/implementation/windows/ble_v2.h b/internal/platform/implementation/windows/ble_v2.h index dae7646d..1a8ee5f5 100644 --- a/internal/platform/implementation/windows/ble_v2.h +++ b/internal/platform/implementation/windows/ble_v2.h @@ -55,7 +55,7 @@ class BleV2Medium : public api::ble_v2::BleMedium { std::unique_ptr StartAdvertising( const api::ble_v2::BleAdvertisementData& advertising_data, api::ble_v2::AdvertiseParameters advertise_set_parameters, - AdvertisingCallback callback) override; + AdvertisingCallback callback) override ABSL_LOCKS_EXCLUDED(mutex_); bool StartScanning(const Uuid& service_uuid, api::ble_v2::TxPowerLevel tx_power_level, @@ -125,52 +125,53 @@ class BleV2Medium : public api::ble_v2::BleMedium { winrt::Windows::Devices::Bluetooth::Advertisement:: BluetoothLEAdvertisementWatcherStoppedEventArgs args); - uint64_t GenerateSessionId(); + uint64_t GenerateSessionId() ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); // Returns nullptr if `address` is invalid. - BleV2Peripheral* GetOrCreatePeripheral(absl::string_view address); + BleV2Peripheral* GetOrCreatePeripheral(absl::string_view address) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); // Returns nullptr if `id` does not match a known peripheral. - BleV2Peripheral* GetPeripheral(BleV2Peripheral::UniqueId id); + BleV2Peripheral* GetPeripheral(BleV2Peripheral::UniqueId id) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); - void RemoveExpiredPeripherals() - ABSL_EXCLUSIVE_LOCKS_REQUIRED(peripheral_map_mutex_); + void RemoveExpiredPeripherals() ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); absl::Mutex mutex_; - BluetoothAdapter* adapter_; + BluetoothAdapter* const adapter_; Uuid service_uuid_; api::ble_v2::TxPowerLevel tx_power_level_; ScanCallback scan_callback_; - absl::Mutex map_mutex_; // std::map> absl::flat_hash_map> - service_uuid_to_session_map_ ABSL_GUARDED_BY(map_mutex_); + service_uuid_to_session_map_ ABSL_GUARDED_BY(mutex_); // WinRT objects ::winrt::Windows::Devices::Bluetooth::Advertisement:: - BluetoothLEAdvertisementPublisher publisher_ = nullptr; + BluetoothLEAdvertisementPublisher publisher_ ABSL_GUARDED_BY(mutex_) = + nullptr; ::winrt::Windows::Devices::Bluetooth::Advertisement:: - BluetoothLEAdvertisementWatcher watcher_ = nullptr; + BluetoothLEAdvertisementWatcher watcher_ ABSL_GUARDED_BY(mutex_) = + nullptr; - bool is_ble_publisher_started_ = false; - bool is_gatt_publisher_started_ = false; - bool is_watcher_started_ = false; + bool is_ble_publisher_started_ ABSL_GUARDED_BY(mutex_) = false; + bool is_gatt_publisher_started_ ABSL_GUARDED_BY(mutex_) = false; + bool is_watcher_started_ ABSL_GUARDED_BY(mutex_) = false; - ::winrt::event_token publisher_token_; - ::winrt::event_token watcher_token_; - ::winrt::event_token advertisement_received_token_; + ::winrt::event_token publisher_token_ ABSL_GUARDED_BY(mutex_); + ::winrt::event_token watcher_token_ ABSL_GUARDED_BY(mutex_); + ::winrt::event_token advertisement_received_token_ ABSL_GUARDED_BY(mutex_); BleGattServer* ble_gatt_server_ = nullptr; // Map to protect the pointer for BlePeripheral because // DiscoveredPeripheralCallback only keeps the pointer to the object - absl::Mutex peripheral_map_mutex_; struct PeripheralInfo { absl::Time last_access_time; std::unique_ptr peripheral; }; absl::flat_hash_map peripheral_map_ - ABSL_GUARDED_BY(peripheral_map_mutex_); - absl::Time cleanup_time_ ABSL_GUARDED_BY(peripheral_map_mutex_) = absl::Now(); + ABSL_GUARDED_BY(mutex_); + absl::Time cleanup_time_ ABSL_GUARDED_BY(mutex_) = absl::Now(); }; } // namespace windows