Make platform PlatformManager::PostEvent() return status (#9573)

* Make platform PlatformManager::PostEvent() return status #### Problem `void PlatformManager::PostEvent()` can fail on some platforms, but does not report this to callers. In the #9543 case, an intermediate layer unconditionally returned `CHIP_NO_ERROR`, and `Inet` code assumed it could actually test whether `PostEvent()` succeeded. Fixes #9543 _PacketBuffer leak_ #### Change overview Made `PlatformManager::PostEvent()` return `CHIP_ERROR`. Marked it `[[nodiscard]]` to catch all uses, and made callers propagate or at least log any error. #### Testing CI. * address review comments * restyle * replace PostEventLoggingErrors with PostEventOrDie * fix P6
project-chip · Sep 14, 2021 · 6fd3811 · 6fd3811
1 parent 0859980
commit 6fd3811
Show file tree

Hide file tree

Showing 36 changed files with 224 additions and 148 deletions.
diff --git a/src/include/platform/PlatformManager.h b/src/include/platform/PlatformManager.h
@@ -190,7 +190,8 @@ class PlatformManager
      * processing, the event might get dispatched (on the work item processing
      * thread) before PostEvent returns.
      */
-    void PostEvent(const ChipDeviceEvent * event);
+    [[nodiscard]] CHIP_ERROR PostEvent(const ChipDeviceEvent * event);
+    void PostEventOrDie(const ChipDeviceEvent * event);
     void DispatchEvent(const ChipDeviceEvent * event);
     CHIP_ERROR StartChipTimer(uint32_t durationMS);
 
@@ -354,9 +355,16 @@ inline void PlatformManager::UnlockChipStack()
     static_cast<ImplClass *>(this)->_UnlockChipStack();
 }
 
-inline void PlatformManager::PostEvent(const ChipDeviceEvent * event)
+inline CHIP_ERROR PlatformManager::PostEvent(const ChipDeviceEvent * event)
 {
-    static_cast<ImplClass *>(this)->_PostEvent(event);
+    return static_cast<ImplClass *>(this)->_PostEvent(event);
+}
+
+inline void PlatformManager::PostEventOrDie(const ChipDeviceEvent * event)
+{
+    CHIP_ERROR status = static_cast<ImplClass *>(this)->_PostEvent(event);
+    VerifyOrDieWithMsg(status == CHIP_NO_ERROR, DeviceLayer, "Failed to post event %d: %" CHIP_ERROR_FORMAT,
+                       static_cast<int>(event->Type), status.Format());
 }
 
 inline void PlatformManager::DispatchEvent(const ChipDeviceEvent * event)

diff --git a/src/include/platform/internal/GenericConfigurationManagerImpl.cpp b/src/include/platform/internal/GenericConfigurationManagerImpl.cpp
@@ -781,11 +781,14 @@ CHIP_ERROR GenericConfigurationManagerImpl<ImplClass>::_StoreServiceProvisioning
 template <class ImplClass>
 CHIP_ERROR GenericConfigurationManagerImpl<ImplClass>::_ClearServiceProvisioningData()
 {
+    CHIP_ERROR err = CHIP_NO_ERROR;
+
     Impl()->ClearConfigValue(ImplClass::kConfigKey_ServiceId);
     Impl()->ClearConfigValue(ImplClass::kConfigKey_ServiceConfig);
     Impl()->ClearConfigValue(ImplClass::kConfigKey_PairedAccountId);
 
     // TODO: Move these behaviors out of configuration manager.
+    // Also, should the flags be cleared even if the corresponding notification fails to post?
 
     // If necessary, post an event alerting other subsystems to the change in
     // the account pairing state.
@@ -794,7 +797,7 @@ CHIP_ERROR GenericConfigurationManagerImpl<ImplClass>::_ClearServiceProvisioning
         ChipDeviceEvent event;
         event.Type                                   = DeviceEventType::kAccountPairingChange;
         event.AccountPairingChange.IsPairedToAccount = false;
-        PlatformMgr().PostEvent(&event);
+        err                                          = PlatformMgr().PostEvent(&event);
     }
 
     // If necessary, post an event alerting other subsystems to the change in
@@ -805,13 +808,17 @@ CHIP_ERROR GenericConfigurationManagerImpl<ImplClass>::_ClearServiceProvisioning
         event.Type                                           = DeviceEventType::kServiceProvisioningChange;
         event.ServiceProvisioningChange.IsServiceProvisioned = false;
         event.ServiceProvisioningChange.ServiceConfigUpdated = false;
-        PlatformMgr().PostEvent(&event);
+        CHIP_ERROR postError                                 = PlatformMgr().PostEvent(&event);
+        if (err == CHIP_NO_ERROR)
+        {
+            err = postError;
+        }
     }
 
     mFlags.Clear(Flags::kIsServiceProvisioned);
     mFlags.Clear(Flags::kIsPairedToAccount);
 
-    return CHIP_NO_ERROR;
+    return err;
 }
 
 template <class ImplClass>

diff --git a/src/include/platform/internal/GenericConnectivityManagerImpl_Thread.cpp b/src/include/platform/internal/GenericConnectivityManagerImpl_Thread.cpp
@@ -109,7 +109,11 @@ void GenericConnectivityManagerImpl_Thread<ImplClass>::UpdateServiceConnectivity
             event.ServiceConnectivityChange.ViaThread.Result =
                 (haveServiceConnectivity) ? kConnectivity_Established : kConnectivity_Lost;
             event.ServiceConnectivityChange.Overall.Result = event.ServiceConnectivityChange.ViaThread.Result;
-            PlatformMgr().PostEvent(&event);
+            CHIP_ERROR status                              = PlatformMgr().PostEvent(&event);
+            if (status != CHIP_NO_ERROR)
+            {
+                ChipLogError(DeviceLayer, "Failed to post thread connectivity change: %" CHIP_ERROR_FORMAT, status.Format());
+            }
         }
     }
 }

diff --git a/src/include/platform/internal/GenericPlatformManagerImpl.cpp b/src/include/platform/internal/GenericPlatformManagerImpl.cpp
@@ -207,7 +207,11 @@ void GenericPlatformManagerImpl<ImplClass>::_ScheduleWork(AsyncWorkFunct workFun
     event.CallWorkFunct.WorkFunct = workFunct;
     event.CallWorkFunct.Arg       = arg;
 
-    Impl()->PostEvent(&event);
+    CHIP_ERROR status = Impl()->PostEvent(&event);
+    if (status != CHIP_NO_ERROR)
+    {
+        ChipLogError(DeviceLayer, "Failed to schedule work: %" CHIP_ERROR_FORMAT, status.Format());
+    }
 }
 
 template <class ImplClass>

diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp
@@ -98,15 +98,19 @@ void GenericPlatformManagerImpl_FreeRTOS<ImplClass>::_UnlockChipStack(void)
 }
 
 template <class ImplClass>
-void GenericPlatformManagerImpl_FreeRTOS<ImplClass>::_PostEvent(const ChipDeviceEvent * event)
+CHIP_ERROR GenericPlatformManagerImpl_FreeRTOS<ImplClass>::_PostEvent(const ChipDeviceEvent * event)
 {
-    if (mChipEventQueue != NULL)
+    if (mChipEventQueue == NULL)
     {
-        if (!xQueueSend(mChipEventQueue, event, 1))
-        {
-            ChipLogError(DeviceLayer, "Failed to post event to CHIP Platform event queue");
-        }
+        return CHIP_ERROR_INTERNAL;
+    }
+    BaseType_t status = xQueueSend(mChipEventQueue, event, 1);
+    if (status != pdTRUE)
+    {
+        ChipLogError(DeviceLayer, "Failed to post event to CHIP Platform event queue");
+        return CHIP_ERROR(chip::ChipError::Range::kOS, status);
     }
+    return CHIP_NO_ERROR;
 }
 
 template <class ImplClass>
@@ -218,7 +222,7 @@ CHIP_ERROR GenericPlatformManagerImpl_FreeRTOS<ImplClass>::_StartChipTimer(uint3
     {
         ChipDeviceEvent event;
         event.Type = DeviceEventType::kNoOp;
-        Impl()->PostEvent(&event);
+        ReturnErrorOnFailure(Impl()->PostEvent(&event));
     }
 
     return CHIP_NO_ERROR;

diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.h b/src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.h
@@ -68,7 +68,7 @@ class GenericPlatformManagerImpl_FreeRTOS : public GenericPlatformManagerImpl<Im
     void _LockChipStack(void);
     bool _TryLockChipStack(void);
     void _UnlockChipStack(void);
-    void _PostEvent(const ChipDeviceEvent * event);
+    CHIP_ERROR _PostEvent(const ChipDeviceEvent * event);
     void _RunEventLoop(void);
     CHIP_ERROR _StartEventLoopTask(void);
     CHIP_ERROR _StopEventLoopTask();

diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp
@@ -127,11 +127,12 @@ CHIP_ERROR GenericPlatformManagerImpl_POSIX<ImplClass>::_StartChipTimer(int64_t
 }
 
 template <class ImplClass>
-void GenericPlatformManagerImpl_POSIX<ImplClass>::_PostEvent(const ChipDeviceEvent * event)
+CHIP_ERROR GenericPlatformManagerImpl_POSIX<ImplClass>::_PostEvent(const ChipDeviceEvent * event)
 {
     mChipEventQueue.Push(*event);
 
     SystemLayerSocketsLoop().Signal(); // Trigger wake select on CHIP thread
+    return CHIP_NO_ERROR;
 }
 
 template <class ImplClass>

diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.h
@@ -89,7 +89,7 @@ class GenericPlatformManagerImpl_POSIX : public GenericPlatformManagerImpl<ImplC
     void _LockChipStack();
     bool _TryLockChipStack();
     void _UnlockChipStack();
-    void _PostEvent(const ChipDeviceEvent * event);
+    CHIP_ERROR _PostEvent(const ChipDeviceEvent * event);
     void _RunEventLoop();
     CHIP_ERROR _StartEventLoopTask();
     CHIP_ERROR _StopEventLoopTask();

diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_Zephyr.cpp b/src/include/platform/internal/GenericPlatformManagerImpl_Zephyr.cpp
@@ -32,6 +32,7 @@
 // from which the GenericPlatformManagerImpl_Zephyr<> template inherits.
 #include <platform/internal/GenericPlatformManagerImpl.cpp>
 
+#include <system/SystemError.h>
 #include <system/SystemLayer.h>
 
 #define DEFAULT_MIN_SLEEP_PERIOD (60 * 60 * 24 * 30) // Month [sec]
@@ -107,15 +108,19 @@ CHIP_ERROR GenericPlatformManagerImpl_Zephyr<ImplClass>::_Shutdown(void)
 }
 
 template <class ImplClass>
-void GenericPlatformManagerImpl_Zephyr<ImplClass>::_PostEvent(const ChipDeviceEvent * event)
+CHIP_ERROR GenericPlatformManagerImpl_Zephyr<ImplClass>::_PostEvent(const ChipDeviceEvent * event)
 {
     // For some reasons mentioned in https://github.com/zephyrproject-rtos/zephyr/issues/22301
     // k_msgq_put takes `void*` instead of `const void*`. Nonetheless, it should be safe to
     // const_cast here and there are components in Zephyr itself which do the same.
-    if (k_msgq_put(&mChipEventQueue, const_cast<ChipDeviceEvent *>(event), K_NO_WAIT) == 0)
-        SystemLayerSocketsLoop().Signal(); // Trigger wake on CHIP thread
-    else
+    int status = k_msgq_put(&mChipEventQueue, const_cast<ChipDeviceEvent *>(event), K_NO_WAIT);
+    if (status != 0)
+    {
         ChipLogError(DeviceLayer, "Failed to post event to CHIP Platform event queue");
+        return System::MapErrorZephyr(status);
+    }
+    SystemLayerSocketsLoop().Signal(); // Trigger wake on CHIP thread
+    return CHIP_NO_ERROR;
 }
 
 template <class ImplClass>

diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_Zephyr.h b/src/include/platform/internal/GenericPlatformManagerImpl_Zephyr.h
@@ -73,7 +73,7 @@ class GenericPlatformManagerImpl_Zephyr : public GenericPlatformManagerImpl<Impl
     void _LockChipStack(void);
     bool _TryLockChipStack(void);
     void _UnlockChipStack(void);
-    void _PostEvent(const ChipDeviceEvent * event);
+    CHIP_ERROR _PostEvent(const ChipDeviceEvent * event);
     void _RunEventLoop(void);
     CHIP_ERROR _StartEventLoopTask(void);
     CHIP_ERROR _StopEventLoopTask();

diff --git a/src/platform/Darwin/PlatformManagerImpl.cpp b/src/platform/Darwin/PlatformManagerImpl.cpp
@@ -114,12 +114,13 @@ CHIP_ERROR PlatformManagerImpl::_Shutdown()
     return GenericPlatformManagerImpl<ImplClass>::_Shutdown();
 }
 
-void PlatformManagerImpl::_PostEvent(const ChipDeviceEvent * event)
+CHIP_ERROR PlatformManagerImpl::_PostEvent(const ChipDeviceEvent * event)
 {
     const ChipDeviceEvent eventCopy = *event;
     dispatch_async(mWorkQueue, ^{
         Impl()->DispatchEvent(&eventCopy);
     });
+    return CHIP_NO_ERROR;
 }
 
 } // namespace DeviceLayer

diff --git a/src/platform/Darwin/PlatformManagerImpl.h b/src/platform/Darwin/PlatformManagerImpl.h
@@ -65,7 +65,7 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener
     void _LockChipStack(){};
     bool _TryLockChipStack() { return false; };
     void _UnlockChipStack(){};
-    void _PostEvent(const ChipDeviceEvent * event);
+    CHIP_ERROR _PostEvent(const ChipDeviceEvent * event);
 
 #if CHIP_STACK_LOCK_TRACKING_ENABLED
     bool _IsChipStackLockedByCurrentThread() const { return false; };

diff --git a/src/platform/DeviceControlServer.cpp b/src/platform/DeviceControlServer.cpp
@@ -46,7 +46,11 @@ void DeviceControlServer::CommissioningFailedTimerComplete()
     ChipDeviceEvent event;
     event.Type                         = DeviceEventType::kCommissioningComplete;
     event.CommissioningComplete.status = CHIP_ERROR_TIMEOUT;
-    PlatformMgr().PostEvent(&event);
+    CHIP_ERROR status                  = PlatformMgr().PostEvent(&event);
+    if (status != CHIP_NO_ERROR)
+    {
+        ChipLogError(DeviceLayer, "Failed to post commissioning complete: %" CHIP_ERROR_FORMAT, status.Format());
+    }
 }
 
 CHIP_ERROR DeviceControlServer::ArmFailSafe(uint16_t expiryLengthSeconds)
@@ -68,8 +72,7 @@ CHIP_ERROR DeviceControlServer::CommissioningComplete()
     ChipDeviceEvent event;
     event.Type                         = DeviceEventType::kCommissioningComplete;
     event.CommissioningComplete.status = CHIP_NO_ERROR;
-    PlatformMgr().PostEvent(&event);
-    return CHIP_NO_ERROR;
+    return PlatformMgr().PostEvent(&event);
 }
 
 CHIP_ERROR DeviceControlServer::SetRegulatoryConfig(uint8_t location, const char * countryCode, uint64_t breadcrumb)

diff --git a/src/platform/EFR32/BLEManagerImpl.cpp b/src/platform/EFR32/BLEManagerImpl.cpp
@@ -423,7 +423,7 @@ void BLEManagerImpl::_OnPlatformEvent(const ChipDeviceEvent * event)
         ChipLogProgress(DeviceLayer, "_OnPlatformEvent kCHIPoBLESubscribe");
         HandleSubscribeReceived(event->CHIPoBLESubscribe.ConId, &CHIP_BLE_SVC_ID, &ChipUUID_CHIPoBLEChar_TX);
         connEstEvent.Type = DeviceEventType::kCHIPoBLEConnectionEstablished;
-        PlatformMgr().PostEvent(&connEstEvent);
+        PlatformMgr().PostEventOrDie(&connEstEvent);
     }
     break;
 
@@ -523,7 +523,7 @@ bool BLEManagerImpl::SendIndication(BLE_CONNECTION_OBJECT conId, const ChipBleUU
     {
         event.Type                        = DeviceEventType::kCHIPoBLENotifyConfirm;
         event.CHIPoBLENotifyConfirm.ConId = conId;
-        PlatformMgr().PostEvent(&event);
+        err                               = PlatformMgr().PostEvent(&event);
     }
 
 exit:
@@ -881,7 +881,7 @@ void BLEManagerImpl::HandleConnectionCloseEvent(volatile sl_bt_msg_t * evt)
 
         ChipLogProgress(DeviceLayer, "BLE GATT connection closed (con %u, reason %u)", connHandle, conn_evt->reason);
 
-        PlatformMgr().PostEvent(&event);
+        PlatformMgr().PostEventOrDie(&event);
 
         // Arrange to re-enable connectable advertising in case it was disabled due to the
         // maximum connection limit being reached.
@@ -931,7 +931,7 @@ void BLEManagerImpl::HandleTXCharCCCDWrite(volatile sl_bt_msg_t * evt)
             {
                 event.Type                    = DeviceEventType::kCHIPoBLESubscribe;
                 event.CHIPoBLESubscribe.ConId = evt->data.evt_gatt_server_user_write_request.connection;
-                PlatformMgr().PostEvent(&event);
+                err                           = PlatformMgr().PostEvent(&event);
             }
         }
     }
@@ -940,7 +940,7 @@ void BLEManagerImpl::HandleTXCharCCCDWrite(volatile sl_bt_msg_t * evt)
         bleConnState->subscribed      = 0;
         event.Type                    = DeviceEventType::kCHIPoBLEUnsubscribe;
         event.CHIPoBLESubscribe.ConId = evt->data.evt_gatt_server_user_write_request.connection;
-        PlatformMgr().PostEvent(&event);
+        err                           = PlatformMgr().PostEvent(&event);
     }
 
 exit:
@@ -970,7 +970,7 @@ void BLEManagerImpl::HandleRXCharWrite(volatile sl_bt_msg_t * evt)
         event.Type                        = DeviceEventType::kCHIPoBLEWriteReceived;
         event.CHIPoBLEWriteReceived.ConId = evt->data.evt_gatt_server_user_write_request.connection;
         event.CHIPoBLEWriteReceived.Data  = std::move(buf).UnsafeRelease();
-        PlatformMgr().PostEvent(&event);
+        err                               = PlatformMgr().PostEvent(&event);
     }
 
 exit:
@@ -996,7 +996,7 @@ void BLEManagerImpl::HandleTxConfirmationEvent(BLE_CONNECTION_OBJECT conId)
 
     event.Type                          = DeviceEventType::kCHIPoBLEIndicateConfirm;
     event.CHIPoBLEIndicateConfirm.ConId = conId;
-    PlatformMgr().PostEvent(&event);
+    PlatformMgr().PostEventOrDie(&event);
 }
 
 void BLEManagerImpl::HandleSoftTimerEvent(volatile sl_bt_msg_t * evt)
@@ -1011,7 +1011,7 @@ void BLEManagerImpl::HandleSoftTimerEvent(volatile sl_bt_msg_t * evt)
         event.CHIPoBLEConnectionError.ConId                          = mIndConfId[evt->data.evt_system_soft_timer.handle];
         sInstance.mIndConfId[evt->data.evt_system_soft_timer.handle] = kUnusedIndex;
         event.CHIPoBLEConnectionError.Reason                         = BLE_ERROR_CHIPOBLE_PROTOCOL_ABORT;
-        PlatformMgr().PostEvent(&event);
+        PlatformMgr().PostEventOrDie(&event);
     }
 }
 

diff --git a/src/platform/ESP32/ConnectivityManagerImpl.cpp b/src/platform/ESP32/ConnectivityManagerImpl.cpp
@@ -667,7 +667,7 @@ void ConnectivityManagerImpl::OnStationConnected()
     ChipDeviceEvent event;
     event.Type                          = DeviceEventType::kWiFiConnectivityChange;
     event.WiFiConnectivityChange.Result = kConnectivity_Established;
-    PlatformMgr().PostEvent(&event);
+    PlatformMgr().PostEventOrDie(&event);
 
     UpdateInternetConnectivityState();
 }
@@ -680,7 +680,7 @@ void ConnectivityManagerImpl::OnStationDisconnected()
     ChipDeviceEvent event;
     event.Type                          = DeviceEventType::kWiFiConnectivityChange;
     event.WiFiConnectivityChange.Result = kConnectivity_Lost;
-    PlatformMgr().PostEvent(&event);
+    PlatformMgr().PostEventOrDie(&event);
 
     UpdateInternetConnectivityState();
 }
@@ -950,7 +950,7 @@ void ConnectivityManagerImpl::UpdateInternetConnectivityState(void)
         event.InternetConnectivityChange.IPv4 = GetConnectivityChange(hadIPv4Conn, haveIPv4Conn);
         event.InternetConnectivityChange.IPv6 = GetConnectivityChange(hadIPv6Conn, haveIPv6Conn);
         addr.ToString(event.InternetConnectivityChange.address);
-        PlatformMgr().PostEvent(&event);
+        PlatformMgr().PostEventOrDie(&event);
 
         if (haveIPv4Conn != hadIPv4Conn)
         {
@@ -981,7 +981,7 @@ void ConnectivityManagerImpl::OnStationIPv4AddressAvailable(const ip_event_got_i
     ChipDeviceEvent event;
     event.Type                           = DeviceEventType::kInterfaceIpAddressChanged;
     event.InterfaceIpAddressChanged.Type = InterfaceIpChangeType::kIpV4_Assigned;
-    PlatformMgr().PostEvent(&event);
+    PlatformMgr().PostEventOrDie(&event);
 }
 
 void ConnectivityManagerImpl::OnStationIPv4AddressLost(void)
@@ -995,7 +995,7 @@ void ConnectivityManagerImpl::OnStationIPv4AddressLost(void)
     ChipDeviceEvent event;
     event.Type                           = DeviceEventType::kInterfaceIpAddressChanged;
     event.InterfaceIpAddressChanged.Type = InterfaceIpChangeType::kIpV4_Lost;
-    PlatformMgr().PostEvent(&event);
+    PlatformMgr().PostEventOrDie(&event);
 }
 
 void ConnectivityManagerImpl::OnIPv6AddressAvailable(const ip_event_got_ip6_t & got_ip)
@@ -1014,7 +1014,7 @@ void ConnectivityManagerImpl::OnIPv6AddressAvailable(const ip_event_got_ip6_t &
     ChipDeviceEvent event;
     event.Type                           = DeviceEventType::kInterfaceIpAddressChanged;
     event.InterfaceIpAddressChanged.Type = InterfaceIpChangeType::kIpV6_Assigned;
-    PlatformMgr().PostEvent(&event);
+    PlatformMgr().PostEventOrDie(&event);
 }
 
 void ConnectivityManagerImpl::RefreshMessageLayer(void) {}

diff --git a/src/platform/ESP32/PlatformManagerImpl.cpp b/src/platform/ESP32/PlatformManagerImpl.cpp
@@ -192,7 +192,7 @@ void PlatformManagerImpl::HandleESPSystemEvent(void * arg, esp_event_base_t even
         }
     }
 
-    sInstance.PostEvent(&event);
+    sInstance.PostEventOrDie(&event);
 }
 
 } // namespace DeviceLayer
-Original file line number
+Diff line change
@@ Expand Up @@
             }
         }
-        sInstance.PostEvent(&event);
+        sInstance.PostEventOrDie(&event);
     }
     } // namespace DeviceLayer
@@ Expand Down @@