From 21ba5bbea87602ff5b4e26b2f54abe09c9846018 Mon Sep 17 00:00:00 2001
From: Abtin Keshavarzian <abtink@google.com>
Date: Tue, 18 Mar 2025 12:22:17 -0700
Subject: [PATCH] [coap] ensure correct retransmission timer scheduling
 (#11348)

This commit introduces `CoapBase::ScheduleRetransmissionTimer()`, a
new method that calculates the next retransmission timer fire time
based on all queued messages in `mPendingRequests` and then
schedules (starts or stops) the timer.

This method is now used whenever `mPendingRequests` is updated (a new
message is added or an existing message is removed). It is also used
in `HandleRetransmissionTimer()`, the timer's callback. This change
centralizes timer scheduling, simplifying the logic.

This also addresses an issue in the existing code where
`HandleRetransmissionTimer()` iterated over `mPendingRequests` to
determine expired messages and calculate the next fire time. However,
finalizing an expired message could trigger its `ResponseHandler`
callback, from which the caller may start or abort CoAP message tx
and  modify `mPendingRequests` and reschedule the timer, leading to
incorrect fire time calculations (`NextFireTime` can be incorrect
which would then improperly re-schedules the timer, stop it or
schedule it to a later time).

With this change, `HandleRetransmissionTimer()` first finalizes
expired messages (potentially invoking callbacks) and then calls
`ScheduleRetransmissionTimer()` to calculate the next fire time
based on the updated `mPendingRequests`, ensuring accurate timer
scheduling.
---
 src/core/coap/coap.cpp | 51 ++++++++++++++++++++++++++----------------
 src/core/coap/coap.hpp |  1 +
 2 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/src/core/coap/coap.cpp b/src/core/coap/coap.cpp
index b57bb6ae0..e9609d57c 100644
--- a/src/core/coap/coap.cpp
+++ b/src/core/coap/coap.cpp
@@ -448,6 +448,31 @@ exit:
     return error;
 }
 
+void CoapBase::ScheduleRetransmissionTimer(void)
+{
+    NextFireTime nextTime;
+    Metadata     metadata;
+
+    for (const Message &message : mPendingRequests)
+    {
+        metadata.ReadFrom(message);
+
+#if OPENTHREAD_CONFIG_COAP_OBSERVE_API_ENABLE
+        if (message.IsRequest() && metadata.mObserve && metadata.mAcknowledged)
+        {
+            // This is an RFC7641 subscription which is already acknowledged.
+            // We do not time it out, so skip it when determining the next
+            // fire time.
+            continue;
+        }
+#endif
+
+        nextTime.UpdateIfEarlier(metadata.mNextTimerShot);
+    }
+
+    mRetransmissionTimer.FireAt(nextTime);
+}
+
 void CoapBase::HandleRetransmissionTimer(Timer &aTimer)
 {
     static_cast<Coap *>(static_cast<TimerMilliContext &>(aTimer).GetContext())->HandleRetransmissionTimer();
@@ -455,7 +480,7 @@ void CoapBase::HandleRetransmissionTimer(Timer &aTimer)
 
 void CoapBase::HandleRetransmissionTimer(void)
 {
-    NextFireTime     nextTime;
+    TimeMilli        now = TimerMilli::GetNow();
     Metadata         metadata;
     Ip6::MessageInfo messageInfo;
 
@@ -463,7 +488,7 @@ void CoapBase::HandleRetransmissionTimer(void)
     {
         metadata.ReadFrom(message);
 
-        if (nextTime.GetNow() >= metadata.mNextTimerShot)
+        if (now >= metadata.mNextTimerShot)
         {
 #if OPENTHREAD_CONFIG_COAP_OBSERVE_API_ENABLE
             if (message.IsRequest() && metadata.mObserve && metadata.mAcknowledged)
@@ -483,7 +508,7 @@ void CoapBase::HandleRetransmissionTimer(void)
             // Increment retransmission counter and timer.
             metadata.mRetransmissionsRemaining--;
             metadata.mRetransmissionTimeout *= 2;
-            metadata.mNextTimerShot = nextTime.GetNow() + metadata.mRetransmissionTimeout;
+            metadata.mNextTimerShot = now + metadata.mRetransmissionTimeout;
             metadata.UpdateIn(message);
 
             // Retransmit
@@ -501,11 +526,9 @@ void CoapBase::HandleRetransmissionTimer(void)
                 SendCopy(message, messageInfo);
             }
         }
-
-        nextTime.UpdateIfEarlier(metadata.mNextTimerShot);
     }
 
-    mRetransmissionTimer.FireAt(nextTime);
+    ScheduleRetransmissionTimer();
 }
 
 void CoapBase::FinalizeCoapTransaction(Message                &aRequest,
@@ -550,9 +573,8 @@ Message *CoapBase::CopyAndEnqueueMessage(const Message &aMessage, uint16_t aCopy
 
     SuccessOrExit(error = aMetadata.AppendTo(*messageCopy));
 
-    mRetransmissionTimer.FireAtIfEarlier(aMetadata.mNextTimerShot);
-
     mPendingRequests.Enqueue(*messageCopy);
+    ScheduleRetransmissionTimer();
 
 exit:
     FreeAndNullMessageOnError(messageCopy, error);
@@ -561,17 +583,8 @@ exit:
 
 void CoapBase::DequeueMessage(Message &aMessage)
 {
-    mPendingRequests.Dequeue(aMessage);
-
-    if (mRetransmissionTimer.IsRunning() && (mPendingRequests.GetHead() == nullptr))
-    {
-        mRetransmissionTimer.Stop();
-    }
-
-    aMessage.Free();
-
-    // No need to worry that the earliest pending message was removed -
-    // the timer would just shoot earlier and then it'd be setup again.
+    mPendingRequests.DequeueAndFree(aMessage);
+    ScheduleRetransmissionTimer();
 }
 
 #if OPENTHREAD_CONFIG_COAP_BLOCKWISE_TRANSFER_ENABLE
diff --git a/src/core/coap/coap.hpp b/src/core/coap/coap.hpp
index e21879dc4..858e07bb4 100644
--- a/src/core/coap/coap.hpp
+++ b/src/core/coap/coap.hpp
@@ -821,6 +821,7 @@ private:
     Message *InitMessage(Message *aMessage, Type aType, Uri aUri);
     Message *InitResponse(Message *aMessage, const Message &aRequest);
 
+    void        ScheduleRetransmissionTimer(void);
     static void HandleRetransmissionTimer(Timer &aTimer);
     void        HandleRetransmissionTimer(void);