From a88f22eb3481403edc829f64e6a6664d878942fa Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 31 Mar 2023 12:07:55 +0200 Subject: [PATCH 1/6] Run functions on GLib event loop in a sync way --- src/platform/Linux/PlatformManagerImpl.cpp | 95 +++++++------------ src/platform/Linux/PlatformManagerImpl.h | 53 ++++------- .../Linux/bluez/ChipDeviceScanner.cpp | 12 +-- src/platform/Linux/bluez/ChipDeviceScanner.h | 4 +- src/platform/Linux/bluez/Helper.cpp | 72 +++++++------- 5 files changed, 96 insertions(+), 140 deletions(-) diff --git a/src/platform/Linux/PlatformManagerImpl.cpp b/src/platform/Linux/PlatformManagerImpl.cpp index ff753e40fab6ba..a3fe8b81c05452 100644 --- a/src/platform/Linux/PlatformManagerImpl.cpp +++ b/src/platform/Linux/PlatformManagerImpl.cpp @@ -56,11 +56,24 @@ PlatformManagerImpl PlatformManagerImpl::sInstance; namespace { #if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP + +struct GLibMatterContextInvokeData +{ + CHIP_ERROR (*mFunc)(void *); + void * mFuncUserData; + CHIP_ERROR mFuncResult; + // Sync primitives to wait for the function to be executed + std::mutex mDoneMutex; + std::condition_variable mDoneCond; + bool mDone = false; +}; + void * GLibMainLoopThread(void * loop) { g_main_loop_run(static_cast(loop)); return nullptr; } + #endif #if CHIP_DEVICE_CONFIG_ENABLE_WIFI @@ -193,12 +206,8 @@ CHIP_ERROR PlatformManagerImpl::_InitChipStack() mGLibMainLoop = g_main_loop_new(nullptr, FALSE); mGLibMainLoopThread = g_thread_new("gmain-matter", GLibMainLoopThread, mGLibMainLoop); - { - std::unique_lock lock(mGLibMainLoopCallbackIndirectionMutex); - CallbackIndirection startedInd([](void *) { return G_SOURCE_REMOVE; }, nullptr); - g_idle_add(G_SOURCE_FUNC(&CallbackIndirection::Callback), &startedInd); - startedInd.Wait(lock); - } + // Wait for the GLib main loop to start. + ReturnErrorOnFailure(GLibMatterContextInvokeSync([](void *) { return CHIP_NO_ERROR; }, nullptr)); #endif @@ -254,62 +263,28 @@ void PlatformManagerImpl::_Shutdown() } #if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP - -void PlatformManagerImpl::CallbackIndirection::Wait(std::unique_lock & lock) +CHIP_ERROR PlatformManagerImpl::_GLibMatterContextInvokeSync(CHIP_ERROR (*func)(void *), void * userData) { - mDoneCond.wait(lock, [this]() { return mDone; }); -} - -gboolean PlatformManagerImpl::CallbackIndirection::Callback(CallbackIndirection * self) -{ - // We can not access "self" before acquiring the lock, because TSAN will complain that - // there is a race condition between the thread that created the object and the thread - // that is executing the callback. - std::unique_lock lock(PlatformMgrImpl().mGLibMainLoopCallbackIndirectionMutex); - - auto callback = self->mCallback; - auto userData = self->mUserData; - - lock.unlock(); - auto result = callback(userData); - lock.lock(); - - self->mDone = true; - self->mDoneCond.notify_all(); - - return result; + GLibMatterContextInvokeData invokeData{ func, userData }; + + g_main_context_invoke_full( + g_main_loop_get_context(mGLibMainLoop), G_PRIORITY_HIGH_IDLE, + [](void * userData_) { + auto * data = reinterpret_cast(userData_); + data->mFuncResult = data->mFunc(data->mFuncUserData); + data->mDoneMutex.lock(); + data->mDone = true; + data->mDoneMutex.unlock(); + data->mDoneCond.notify_one(); + return G_SOURCE_REMOVE; + }, + &invokeData, nullptr); + + std::unique_lock lock(invokeData.mDoneMutex); + invokeData.mDoneCond.wait(lock, [&invokeData]() { return invokeData.mDone; }); + + return invokeData.mFuncResult; } - -#if CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE -CHIP_ERROR PlatformManagerImpl::RunOnGLibMainLoopThread(GSourceFunc callback, void * userData, bool wait) -{ - - GMainContext * context = g_main_loop_get_context(mGLibMainLoop); - VerifyOrReturnError(context != nullptr, CHIP_ERROR_INTERNAL, - ChipLogDetail(DeviceLayer, "Failed to get GLib main loop context")); - - // If we've been called from the GLib main loop thread itself, there is no reason to wait - // for the callback, as it will be executed immediately by the g_main_context_invoke() call - // below. Using a callback indirection in this case would cause a deadlock. - if (g_main_context_is_owner(context)) - { - wait = false; - } - - if (wait) - { - std::unique_lock lock(mGLibMainLoopCallbackIndirectionMutex); - CallbackIndirection indirection(callback, userData); - g_main_context_invoke(context, G_SOURCE_FUNC(&CallbackIndirection::Callback), &indirection); - indirection.Wait(lock); - return CHIP_NO_ERROR; - } - - g_main_context_invoke(context, callback, userData); - return CHIP_NO_ERROR; -} -#endif // CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE - #endif // CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP } // namespace DeviceLayer diff --git a/src/platform/Linux/PlatformManagerImpl.h b/src/platform/Linux/PlatformManagerImpl.h index 2b1bad0ea7fd2c..75d24b77dabaa8 100644 --- a/src/platform/Linux/PlatformManagerImpl.h +++ b/src/platform/Linux/PlatformManagerImpl.h @@ -57,22 +57,19 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener #if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP && CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE /** - * @brief Executes a callback in the GLib main loop thread. + * @brief Invoke a function on the Matter GLib context. * - * @param[in] callback The callback to execute. - * @param[in] userData User data to pass to the callback. - * @param[in] wait If true, the function will block until the callback has been executed. - * @returns CHIP_NO_ERROR if the callback was successfully executed. - */ - CHIP_ERROR RunOnGLibMainLoopThread(GSourceFunc callback, void * userData, bool wait = false); - - /** - * @brief Convenience method to require less casts to void pointers. + * If execution of the function will have to be scheduled on other thread, + * this call will block the current thread until the function is executed. + * + * @param[in] function The function to call. + * @param[in] userData User data to pass to the function. + * @returns The result of the function. */ - template - CHIP_ERROR ScheduleOnGLibMainLoopThread(gboolean (*callback)(T *), T * userData, bool wait = false) + template + CHIP_ERROR GLibMatterContextInvokeSync(CHIP_ERROR (*func)(T *), T * userData) { - return RunOnGLibMainLoopThread(G_SOURCE_FUNC(callback), userData, wait); + return _GLibMatterContextInvokeSync((CHIP_ERROR(*)(void *)) func, (void *) userData); } #endif @@ -97,29 +94,13 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener #if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP - class CallbackIndirection - { - public: - CallbackIndirection(GSourceFunc callback, void * userData) : mCallback(callback), mUserData(userData) {} - void Wait(std::unique_lock & lock); - static gboolean Callback(CallbackIndirection * self); - - private: - GSourceFunc mCallback; - void * mUserData; - // Sync primitives to wait for the callback to be executed. - std::condition_variable mDoneCond; - bool mDone = false; - }; - - // XXX: Mutex for guarding access to glib main event loop callback indirection - // synchronization primitives. This is a workaround to suppress TSAN warnings. - // TSAN does not know that from the thread synchronization perspective the - // g_source_attach() function should be treated as pthread_create(). Memory - // access to shared data before the call to g_source_attach() without mutex - // is not a race condition - the callback will not be executed on glib main - // event loop thread before the call to g_source_attach(). - std::mutex mGLibMainLoopCallbackIndirectionMutex; + /** + * @brief Invoke a function on the Matter GLib context. + * + * @note This function does not provide type safety for the user data. Please, + * use the GLibMatterContextInvokeSync() template function instead. + */ + CHIP_ERROR _GLibMatterContextInvokeSync(CHIP_ERROR (*func)(void *), void * userData); GMainLoop * mGLibMainLoop; GThread * mGLibMainLoopThread; diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.cpp b/src/platform/Linux/bluez/ChipDeviceScanner.cpp index 04c522ba34aa66..0172c820c5a0cc 100644 --- a/src/platform/Linux/bluez/ChipDeviceScanner.cpp +++ b/src/platform/Linux/bluez/ChipDeviceScanner.cpp @@ -123,7 +123,7 @@ CHIP_ERROR ChipDeviceScanner::StartScan(System::Clock::Timeout timeout) ReturnErrorCodeIf(mIsScanning, CHIP_ERROR_INCORRECT_STATE); mIsScanning = true; // optimistic, to allow all callbacks to check this - if (PlatformMgrImpl().ScheduleOnGLibMainLoopThread(MainLoopStartScan, this, true) != CHIP_NO_ERROR) + if (PlatformMgrImpl().GLibMatterContextInvokeSync(MainLoopStartScan, this) != CHIP_NO_ERROR) { ChipLogError(Ble, "Failed to schedule BLE scan start."); mIsScanning = false; @@ -174,7 +174,7 @@ CHIP_ERROR ChipDeviceScanner::StopScan() mInterfaceChangedSignal = 0; } - if (PlatformMgrImpl().ScheduleOnGLibMainLoopThread(MainLoopStopScan, this, true) != CHIP_NO_ERROR) + if (PlatformMgrImpl().GLibMatterContextInvokeSync(MainLoopStopScan, this) != CHIP_NO_ERROR) { ChipLogError(Ble, "Failed to schedule BLE scan stop."); return CHIP_ERROR_INTERNAL; @@ -183,7 +183,7 @@ CHIP_ERROR ChipDeviceScanner::StopScan() return CHIP_NO_ERROR; } -int ChipDeviceScanner::MainLoopStopScan(ChipDeviceScanner * self) +CHIP_ERROR ChipDeviceScanner::MainLoopStopScan(ChipDeviceScanner * self) { GError * error = nullptr; @@ -199,7 +199,7 @@ int ChipDeviceScanner::MainLoopStopScan(ChipDeviceScanner * self) // references to 'self' here) delegate->OnScanComplete(); - return 0; + return CHIP_NO_ERROR; } void ChipDeviceScanner::SignalObjectAdded(GDBusObjectManager * manager, GDBusObject * object, ChipDeviceScanner * self) @@ -266,7 +266,7 @@ void ChipDeviceScanner::RemoveDevice(BluezDevice1 * device) } } -int ChipDeviceScanner::MainLoopStartScan(ChipDeviceScanner * self) +CHIP_ERROR ChipDeviceScanner::MainLoopStartScan(ChipDeviceScanner * self) { GError * error = nullptr; @@ -308,7 +308,7 @@ int ChipDeviceScanner::MainLoopStartScan(ChipDeviceScanner * self) self->mDelegate->OnScanComplete(); } - return 0; + return CHIP_NO_ERROR; } } // namespace Internal diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.h b/src/platform/Linux/bluez/ChipDeviceScanner.h index f81f923dfb3b47..ad12ba9b31c8d8 100644 --- a/src/platform/Linux/bluez/ChipDeviceScanner.h +++ b/src/platform/Linux/bluez/ChipDeviceScanner.h @@ -79,8 +79,8 @@ class ChipDeviceScanner private: static void TimerExpiredCallback(chip::System::Layer * layer, void * appState); - static int MainLoopStartScan(ChipDeviceScanner * self); - static int MainLoopStopScan(ChipDeviceScanner * self); + static CHIP_ERROR MainLoopStartScan(ChipDeviceScanner * self); + static CHIP_ERROR MainLoopStopScan(ChipDeviceScanner * self); static void SignalObjectAdded(GDBusObjectManager * manager, GDBusObject * object, ChipDeviceScanner * self); static void SignalInterfaceChanged(GDBusObjectManagerClient * manager, GDBusObjectProxy * object, GDBusProxy * aInterface, GVariant * aChangedProperties, const gchar * const * aInvalidatedProps, diff --git a/src/platform/Linux/bluez/Helper.cpp b/src/platform/Linux/bluez/Helper.cpp index ff0c73d1214c8c..f73d4ccb392ba9 100644 --- a/src/platform/Linux/bluez/Helper.cpp +++ b/src/platform/Linux/bluez/Helper.cpp @@ -251,7 +251,7 @@ static void BluezAdvStopDone(GObject * aObject, GAsyncResult * aResult, gpointer g_error_free(error); } -static gboolean BluezAdvSetup(BluezEndpoint * endpoint) +static CHIP_ERROR BluezAdvSetup(BluezEndpoint * endpoint) { BluezLEAdvertisement1 * adv; @@ -263,10 +263,10 @@ static gboolean BluezAdvSetup(BluezEndpoint * endpoint) VerifyOrExit(adv != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL adv in %s", __func__)); exit: - return G_SOURCE_REMOVE; + return CHIP_NO_ERROR; } -static gboolean BluezAdvStart(BluezEndpoint * endpoint) +static CHIP_ERROR BluezAdvStart(BluezEndpoint * endpoint) { GDBusObject * adapter; BluezLEAdvertisingManager1 * advMgr = nullptr; @@ -291,10 +291,10 @@ static gboolean BluezAdvStart(BluezEndpoint * endpoint) endpoint); exit: - return G_SOURCE_REMOVE; + return CHIP_NO_ERROR; } -static gboolean BluezAdvStop(BluezEndpoint * endpoint) +static CHIP_ERROR BluezAdvStop(BluezEndpoint * endpoint) { GDBusObject * adapter; BluezLEAdvertisingManager1 * advMgr = nullptr; @@ -313,7 +313,7 @@ static gboolean BluezAdvStop(BluezEndpoint * endpoint) bluez_leadvertising_manager1_call_unregister_advertisement(advMgr, endpoint->mpAdvPath, nullptr, BluezAdvStopDone, endpoint); exit: - return G_SOURCE_REMOVE; + return CHIP_NO_ERROR; } static gboolean BluezCharacteristicReadValue(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, @@ -843,7 +843,7 @@ static void BluezPeripheralRegisterAppDone(GObject * aObject, GAsyncResult * aRe } } -gboolean BluezPeripheralRegisterApp(BluezEndpoint * endpoint) +static CHIP_ERROR BluezPeripheralRegisterApp(BluezEndpoint * endpoint) { GDBusObject * adapter; BluezGattManager1 * gattMgr; @@ -865,7 +865,7 @@ gboolean BluezPeripheralRegisterApp(BluezEndpoint * endpoint) nullptr); exit: - return G_SOURCE_REMOVE; + return CHIP_NO_ERROR; } /// Update the table of open BLE connections whevener a new device is spotted or its attributes have changed. @@ -1343,7 +1343,7 @@ static void BluezOnNameLost(GDBusConnection * aConn, const gchar * aName, gpoint } #endif -static int StartupEndpointBindings(BluezEndpoint * endpoint) +static CHIP_ERROR StartupEndpointBindings(BluezEndpoint * endpoint) { GDBusObjectManager * manager; GError * error = nullptr; @@ -1378,10 +1378,10 @@ static int StartupEndpointBindings(BluezEndpoint * endpoint) if (error != nullptr) g_error_free(error); - return 0; + return CHIP_NO_ERROR; } -static gboolean BluezC2Indicate(ConnectionDataBundle * closure) +static CHIP_ERROR BluezC2Indicate(ConnectionDataBundle * closure) { BluezConnection * conn = nullptr; GError * error = nullptr; @@ -1424,7 +1424,8 @@ static gboolean BluezC2Indicate(ConnectionDataBundle * closure) if (error != nullptr) g_error_free(error); - return G_SOURCE_REMOVE; + + return CHIP_NO_ERROR; } static ConnectionDataBundle * MakeConnectionDataBundle(BLE_CONNECTION_OBJECT apConn, const chip::System::PacketBufferHandle & apBuf) @@ -1439,10 +1440,10 @@ static ConnectionDataBundle * MakeConnectionDataBundle(BLE_CONNECTION_OBJECT apC CHIP_ERROR SendBluezIndication(BLE_CONNECTION_OBJECT apConn, chip::System::PacketBufferHandle apBuf) { VerifyOrReturnError(!apBuf.IsNull(), CHIP_ERROR_INVALID_ARGUMENT, ChipLogError(DeviceLayer, "apBuf is NULL in %s", __func__)); - return PlatformMgrImpl().ScheduleOnGLibMainLoopThread(BluezC2Indicate, MakeConnectionDataBundle(apConn, apBuf)); + return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezC2Indicate, MakeConnectionDataBundle(apConn, apBuf)); } -static gboolean BluezDisconnect(void * apClosure) +static CHIP_ERROR BluezDisconnect(void * apClosure) { BluezConnection * conn = static_cast(apClosure); GError * error = nullptr; @@ -1459,23 +1460,22 @@ static gboolean BluezDisconnect(void * apClosure) exit: if (error != nullptr) g_error_free(error); - return G_SOURCE_REMOVE; + return CHIP_NO_ERROR; } -static int CloseBleconnectionCB(void * apAppState) +static CHIP_ERROR CloseBleconnectionCB(void * apAppState) { - BluezDisconnect(apAppState); - return G_SOURCE_REMOVE; + return BluezDisconnect(apAppState); } CHIP_ERROR CloseBluezConnection(BLE_CONNECTION_OBJECT apConn) { - return PlatformMgrImpl().ScheduleOnGLibMainLoopThread(CloseBleconnectionCB, apConn); + return PlatformMgrImpl().GLibMatterContextInvokeSync(CloseBleconnectionCB, apConn); } CHIP_ERROR StartBluezAdv(BluezEndpoint * apEndpoint) { - CHIP_ERROR err = PlatformMgrImpl().ScheduleOnGLibMainLoopThread(BluezAdvStart, apEndpoint); + CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync(BluezAdvStart, apEndpoint); VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_ERROR_INCORRECT_STATE, ChipLogError(Ble, "Failed to schedule BluezAdvStart() on CHIPoBluez thread")); return err; @@ -1483,7 +1483,7 @@ CHIP_ERROR StartBluezAdv(BluezEndpoint * apEndpoint) CHIP_ERROR StopBluezAdv(BluezEndpoint * apEndpoint) { - CHIP_ERROR err = PlatformMgrImpl().ScheduleOnGLibMainLoopThread(BluezAdvStop, apEndpoint); + CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync(BluezAdvStop, apEndpoint); VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_ERROR_INCORRECT_STATE, ChipLogError(Ble, "Failed to schedule BluezAdvStop() on CHIPoBluez thread")); return err; @@ -1491,7 +1491,7 @@ CHIP_ERROR StopBluezAdv(BluezEndpoint * apEndpoint) CHIP_ERROR BluezAdvertisementSetup(BluezEndpoint * apEndpoint) { - CHIP_ERROR err = PlatformMgrImpl().ScheduleOnGLibMainLoopThread(BluezAdvSetup, apEndpoint); + CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync(BluezAdvSetup, apEndpoint); VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_ERROR_INCORRECT_STATE, ChipLogError(Ble, "Failed to schedule BluezAdvSetup() on CHIPoBluez thread")); return err; @@ -1499,7 +1499,7 @@ CHIP_ERROR BluezAdvertisementSetup(BluezEndpoint * apEndpoint) CHIP_ERROR BluezGattsAppRegister(BluezEndpoint * apEndpoint) { - CHIP_ERROR err = PlatformMgrImpl().ScheduleOnGLibMainLoopThread(BluezPeripheralRegisterApp, apEndpoint); + CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync(BluezPeripheralRegisterApp, apEndpoint); VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_ERROR_INCORRECT_STATE, ChipLogError(Ble, "Failed to schedule BluezPeripheralRegisterApp() on CHIPoBluez thread")); return err; @@ -1564,7 +1564,7 @@ CHIP_ERROR InitBluezBleLayer(bool aIsCentral, char * apBleAddr, BLEAdvConfig & a endpoint->mpConnectCancellable = g_cancellable_new(); } - err = PlatformMgrImpl().ScheduleOnGLibMainLoopThread(StartupEndpointBindings, endpoint, true); + err = PlatformMgrImpl().GLibMatterContextInvokeSync(StartupEndpointBindings, endpoint); VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(DeviceLayer, "Failed to schedule endpoint initialization")); retval = TRUE; @@ -1607,7 +1607,7 @@ static void SendWriteRequestDone(GObject * aObject, GAsyncResult * aResult, gpoi g_error_free(error); } -static gboolean SendWriteRequestImpl(ConnectionDataBundle * data) +static CHIP_ERROR SendWriteRequestImpl(ConnectionDataBundle * data) { GVariant * options = nullptr; GVariantBuilder optionsBuilder; @@ -1625,13 +1625,13 @@ static gboolean SendWriteRequestImpl(ConnectionDataBundle * data) exit: g_free(data); - return G_SOURCE_REMOVE; + return CHIP_NO_ERROR; } CHIP_ERROR BluezSendWriteRequest(BLE_CONNECTION_OBJECT apConn, chip::System::PacketBufferHandle apBuf) { VerifyOrReturnError(!apBuf.IsNull(), CHIP_ERROR_INVALID_ARGUMENT, ChipLogError(DeviceLayer, "apBuf is NULL in %s", __func__)); - return PlatformMgrImpl().ScheduleOnGLibMainLoopThread(SendWriteRequestImpl, MakeConnectionDataBundle(apConn, apBuf)); + return PlatformMgrImpl().GLibMatterContextInvokeSync(SendWriteRequestImpl, MakeConnectionDataBundle(apConn, apBuf)); } // BluezSubscribeCharacteristic callbacks @@ -1665,7 +1665,7 @@ static void SubscribeCharacteristicDone(GObject * aObject, GAsyncResult * aResul g_error_free(error); } -static gboolean SubscribeCharacteristicImpl(BluezConnection * connection) +static CHIP_ERROR SubscribeCharacteristicImpl(BluezConnection * connection) { BluezGattCharacteristic1 * c2 = nullptr; VerifyOrExit(connection != nullptr, ChipLogError(DeviceLayer, "BluezConnection is NULL in %s", __func__)); @@ -1677,12 +1677,12 @@ static gboolean SubscribeCharacteristicImpl(BluezConnection * connection) bluez_gatt_characteristic1_call_start_notify(connection->mpC2, nullptr, SubscribeCharacteristicDone, connection); exit: - return G_SOURCE_REMOVE; + return CHIP_NO_ERROR; } CHIP_ERROR BluezSubscribeCharacteristic(BLE_CONNECTION_OBJECT apConn) { - return PlatformMgrImpl().ScheduleOnGLibMainLoopThread(SubscribeCharacteristicImpl, static_cast(apConn)); + return PlatformMgrImpl().GLibMatterContextInvokeSync(SubscribeCharacteristicImpl, static_cast(apConn)); } // BluezUnsubscribeCharacteristic callbacks @@ -1704,7 +1704,7 @@ static void UnsubscribeCharacteristicDone(GObject * aObject, GAsyncResult * aRes g_error_free(error); } -static gboolean UnsubscribeCharacteristicImpl(BluezConnection * connection) +static CHIP_ERROR UnsubscribeCharacteristicImpl(BluezConnection * connection) { VerifyOrExit(connection != nullptr, ChipLogError(DeviceLayer, "BluezConnection is NULL in %s", __func__)); VerifyOrExit(connection->mpC2 != nullptr, ChipLogError(DeviceLayer, "C2 is NULL in %s", __func__)); @@ -1712,12 +1712,12 @@ static gboolean UnsubscribeCharacteristicImpl(BluezConnection * connection) bluez_gatt_characteristic1_call_stop_notify(connection->mpC2, nullptr, UnsubscribeCharacteristicDone, connection); exit: - return G_SOURCE_REMOVE; + return CHIP_NO_ERROR; } CHIP_ERROR BluezUnsubscribeCharacteristic(BLE_CONNECTION_OBJECT apConn) { - return PlatformMgrImpl().ScheduleOnGLibMainLoopThread(UnsubscribeCharacteristicImpl, static_cast(apConn)); + return PlatformMgrImpl().GLibMatterContextInvokeSync(UnsubscribeCharacteristicImpl, static_cast(apConn)); } // ConnectDevice callbacks @@ -1772,7 +1772,7 @@ static void ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointe g_error_free(error); } -static gboolean ConnectDeviceImpl(ConnectParams * apParams) +static CHIP_ERROR ConnectDeviceImpl(ConnectParams * apParams) { BluezDevice1 * device = apParams->mDevice; BluezEndpoint * endpoint = apParams->mEndpoint; @@ -1784,7 +1784,7 @@ static gboolean ConnectDeviceImpl(ConnectParams * apParams) bluez_device1_call_connect(device, endpoint->mpConnectCancellable, ConnectDeviceDone, apParams); g_object_unref(device); - return G_SOURCE_REMOVE; + return CHIP_NO_ERROR; } CHIP_ERROR ConnectDevice(BluezDevice1 * apDevice, BluezEndpoint * apEndpoint) @@ -1792,7 +1792,7 @@ CHIP_ERROR ConnectDevice(BluezDevice1 * apDevice, BluezEndpoint * apEndpoint) auto params = chip::Platform::New(apDevice, apEndpoint); g_object_ref(apDevice); - if (PlatformMgrImpl().ScheduleOnGLibMainLoopThread(ConnectDeviceImpl, params) != CHIP_NO_ERROR) + if (PlatformMgrImpl().GLibMatterContextInvokeSync(ConnectDeviceImpl, params) != CHIP_NO_ERROR) { ChipLogError(Ble, "Failed to schedule ConnectDeviceImpl() on CHIPoBluez thread"); g_object_unref(apDevice); From 40811c495d958ec4f9a0d5f088cb6aa5cc259eb3 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 31 Mar 2023 13:22:26 +0200 Subject: [PATCH 2/6] Include invoke sync template when GLib is enabled --- src/platform/Linux/PlatformManagerImpl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/Linux/PlatformManagerImpl.h b/src/platform/Linux/PlatformManagerImpl.h index 75d24b77dabaa8..4913476c129745 100644 --- a/src/platform/Linux/PlatformManagerImpl.h +++ b/src/platform/Linux/PlatformManagerImpl.h @@ -54,7 +54,7 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener public: // ===== Platform-specific members that may be accessed directly by the application. -#if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP && CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE +#if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP /** * @brief Invoke a function on the Matter GLib context. From 805437c32da487b52a3c9ab1d842198eb35fa57a Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 31 Mar 2023 15:07:30 +0200 Subject: [PATCH 3/6] Restore workaround for TSAN false positives --- src/platform/Linux/PlatformManagerImpl.cpp | 43 ++++++++++++---------- src/platform/Linux/PlatformManagerImpl.h | 19 ++++++++++ 2 files changed, 43 insertions(+), 19 deletions(-) diff --git a/src/platform/Linux/PlatformManagerImpl.cpp b/src/platform/Linux/PlatformManagerImpl.cpp index a3fe8b81c05452..d9cb11f0a0273f 100644 --- a/src/platform/Linux/PlatformManagerImpl.cpp +++ b/src/platform/Linux/PlatformManagerImpl.cpp @@ -56,24 +56,11 @@ PlatformManagerImpl PlatformManagerImpl::sInstance; namespace { #if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP - -struct GLibMatterContextInvokeData -{ - CHIP_ERROR (*mFunc)(void *); - void * mFuncUserData; - CHIP_ERROR mFuncResult; - // Sync primitives to wait for the function to be executed - std::mutex mDoneMutex; - std::condition_variable mDoneCond; - bool mDone = false; -}; - void * GLibMainLoopThread(void * loop) { g_main_loop_run(static_cast(loop)); return nullptr; } - #endif #if CHIP_DEVICE_CONFIG_ENABLE_WIFI @@ -265,22 +252,40 @@ void PlatformManagerImpl::_Shutdown() #if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP CHIP_ERROR PlatformManagerImpl::_GLibMatterContextInvokeSync(CHIP_ERROR (*func)(void *), void * userData) { + // Because of TSAN false positives, we need to use a mutex to synchronize access to all members of + // the GLibMatterContextInvokeData object (including constructor and destructor). This is a temporary + // workaround until TSAN-enabled GLib will be used in our CI. + std::unique_lock lock(mGLibMainLoopCallbackIndirectionMutex); + GLibMatterContextInvokeData invokeData{ func, userData }; + lock.unlock(); + g_main_context_invoke_full( g_main_loop_get_context(mGLibMainLoop), G_PRIORITY_HIGH_IDLE, [](void * userData_) { - auto * data = reinterpret_cast(userData_); - data->mFuncResult = data->mFunc(data->mFuncUserData); - data->mDoneMutex.lock(); - data->mDone = true; - data->mDoneMutex.unlock(); + auto * data = reinterpret_cast(userData_); + + // XXX: Temporary workaround for TSAN false positives. + std::unique_lock lock(PlatformMgrImpl().mGLibMainLoopCallbackIndirectionMutex); + + auto func = data->mFunc; + auto userData = data->mFuncUserData; + + lock.unlock(); + auto result = func(userData); + lock.lock(); + + data->mDone = true; + data->mFuncResult = result; data->mDoneCond.notify_one(); + return G_SOURCE_REMOVE; }, &invokeData, nullptr); - std::unique_lock lock(invokeData.mDoneMutex); + lock.lock(); + invokeData.mDoneCond.wait(lock, [&invokeData]() { return invokeData.mDone; }); return invokeData.mFuncResult; diff --git a/src/platform/Linux/PlatformManagerImpl.h b/src/platform/Linux/PlatformManagerImpl.h index 4913476c129745..aa50cce59e2998 100644 --- a/src/platform/Linux/PlatformManagerImpl.h +++ b/src/platform/Linux/PlatformManagerImpl.h @@ -94,6 +94,16 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener #if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP + struct GLibMatterContextInvokeData + { + CHIP_ERROR (*mFunc)(void *); + void * mFuncUserData; + CHIP_ERROR mFuncResult; + // Sync primitives to wait for the function to be executed + std::condition_variable mDoneCond; + bool mDone = false; + }; + /** * @brief Invoke a function on the Matter GLib context. * @@ -102,6 +112,15 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener */ CHIP_ERROR _GLibMatterContextInvokeSync(CHIP_ERROR (*func)(void *), void * userData); + // XXX: Mutex for guarding access to glib main event loop callback indirection + // synchronization primitives. This is a workaround to suppress TSAN warnings. + // TSAN does not know that from the thread synchronization perspective the + // g_source_attach() function should be treated as pthread_create(). Memory + // access to shared data before the call to g_source_attach() without mutex + // is not a race condition - the callback will not be executed on glib main + // event loop thread before the call to g_source_attach(). + std::mutex mGLibMainLoopCallbackIndirectionMutex; + GMainLoop * mGLibMainLoop; GThread * mGLibMainLoopThread; From 95eb3116ac94f9c7e2d88eb0f35b7a8a9789be0f Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 31 Mar 2023 15:14:24 +0200 Subject: [PATCH 4/6] Fix variable shadowing --- src/platform/Linux/PlatformManagerImpl.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/platform/Linux/PlatformManagerImpl.cpp b/src/platform/Linux/PlatformManagerImpl.cpp index d9cb11f0a0273f..1bedad57e649fa 100644 --- a/src/platform/Linux/PlatformManagerImpl.cpp +++ b/src/platform/Linux/PlatformManagerImpl.cpp @@ -267,14 +267,14 @@ CHIP_ERROR PlatformManagerImpl::_GLibMatterContextInvokeSync(CHIP_ERROR (*func)( auto * data = reinterpret_cast(userData_); // XXX: Temporary workaround for TSAN false positives. - std::unique_lock lock(PlatformMgrImpl().mGLibMainLoopCallbackIndirectionMutex); + std::unique_lock lock_(PlatformMgrImpl().mGLibMainLoopCallbackIndirectionMutex); - auto func = data->mFunc; - auto userData = data->mFuncUserData; + auto mFunc = data->mFunc; + auto mUserData = data->mFuncUserData; - lock.unlock(); - auto result = func(userData); - lock.lock(); + lock_.unlock(); + auto result = mFunc(mUserData); + lock_.lock(); data->mDone = true; data->mFuncResult = result; From 1771a9d419bad469df93caf08ee326096124c079 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 3 Apr 2023 17:52:46 +0200 Subject: [PATCH 5/6] Do not use g_main_context_invoke_full for waiting for loop to start The g_main_context_invoke_full() function checks whether the context, on which it's supposed to run callback function, is acquired. Otherwise, it runs given callback on the current thread. In our case this may lead to incorrect GLib signals bindings, so we have to use g_source_attach() when waiting for main loop. --- src/platform/Linux/PlatformManagerImpl.cpp | 26 ++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/src/platform/Linux/PlatformManagerImpl.cpp b/src/platform/Linux/PlatformManagerImpl.cpp index 1bedad57e649fa..e536ade9b094d7 100644 --- a/src/platform/Linux/PlatformManagerImpl.cpp +++ b/src/platform/Linux/PlatformManagerImpl.cpp @@ -193,8 +193,30 @@ CHIP_ERROR PlatformManagerImpl::_InitChipStack() mGLibMainLoop = g_main_loop_new(nullptr, FALSE); mGLibMainLoopThread = g_thread_new("gmain-matter", GLibMainLoopThread, mGLibMainLoop); - // Wait for the GLib main loop to start. - ReturnErrorOnFailure(GLibMatterContextInvokeSync([](void *) { return CHIP_NO_ERROR; }, nullptr)); + { + // Wait for the GLib main loop to start. It is required that the context used + // by the main loop is acquired before any other GLib functions are called. Otherwise, + // the GLibMatterContextInvokeSync() might run functions on the wrong thread. + + std::unique_lock lock(mGLibMainLoopCallbackIndirectionMutex); + GLibMatterContextInvokeData invokeData{}; + + auto * idleSource = g_idle_source_new(); + g_source_set_callback( + idleSource, + [](void * userData_) { + auto * data = reinterpret_cast(userData_); + std::unique_lock lock_(PlatformMgrImpl().mGLibMainLoopCallbackIndirectionMutex); + data->mDone = true; + data->mDoneCond.notify_one(); + return G_SOURCE_REMOVE; + }, + &invokeData, nullptr); + g_source_attach(idleSource, g_main_loop_get_context(mGLibMainLoop)); + g_source_unref(idleSource); + + invokeData.mDoneCond.wait(lock, [&invokeData]() { return invokeData.mDone; }); + } #endif From 523b369b9700ffa0ff308980ba86cf01431d769a Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Tue, 4 Apr 2023 10:56:09 +0200 Subject: [PATCH 6/6] Release context object after joining glib thread TSAN can report false-positive on context unref. By firstly joining thread and later unreferencing context we will prevent such warning. --- src/platform/Linux/PlatformManagerImpl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/Linux/PlatformManagerImpl.cpp b/src/platform/Linux/PlatformManagerImpl.cpp index e536ade9b094d7..8001ae6677d7de 100644 --- a/src/platform/Linux/PlatformManagerImpl.cpp +++ b/src/platform/Linux/PlatformManagerImpl.cpp @@ -266,8 +266,8 @@ void PlatformManagerImpl::_Shutdown() #if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP g_main_loop_quit(mGLibMainLoop); - g_main_loop_unref(mGLibMainLoop); g_thread_join(mGLibMainLoopThread); + g_main_loop_unref(mGLibMainLoop); #endif }