support: handle invalid transaction ids instead of asserting

Author: frankist

include/srsran/support/async/protocol_transaction_manager.h

@@ -74,7 +74,8 @@ class protocol_transaction_manager
     unsigned transaction_id = next_transaction_id.fetch_add(1, std::memory_order_relaxed) % N;
     if (not transactions[transaction_id].is_set()) {
       // cancel any existing awaiter.
-      set(transaction_id, cancel_value);
+      bool ignore = set(transaction_id, cancel_value);
+      (void)ignore;
     }
     transactions[transaction_id].reset();
     return {transaction_id, transactions[transaction_id]};
@@ -90,19 +91,26 @@ class protocol_transaction_manager
       // Create a new timer if it doesn't exist yet.
       running_timers[t.id()] = timer_db.create_unique_timer();
     }
-    running_timers[t.id()].set(time_to_cancel,
-                               [this, transaction_id = t.id()](timer_id_t tid) { set(transaction_id, cancel_value); });
+    running_timers[t.id()].set(time_to_cancel, [this, transaction_id = t.id()](timer_id_t tid) {
+      if (not set(transaction_id, cancel_value)) {
+        srslog::fetch_basic_logger("ALL").warning("Transaction id={} timeout but transaction is already completed",
+                                                  transaction_id);
+      }
+    });
     running_timers[t.id()].run();
     return t;
   }
 
   /// \brief Sets the result of a managed transaction with the provided transaction_id.
   template <typename U>
-  void set(unsigned transaction_id, U&& u)
+  bool __attribute__((warn_unused_result)) set(unsigned transaction_id, U&& u)
   {
+    if (transactions[transaction_id].is_set()) {
+      return false;
+    }
     running_timers[transaction_id].stop();
-    srsran_assert(not transactions[transaction_id].is_set(), "Transaction result cannot be overwritten.");
     transactions[transaction_id].set(std::forward<U>(u));
+    return true;
   }
 
 private:

lib/e1ap/cu_cp/e1ap_cu_cp_impl.cpp

@@ -235,7 +235,10 @@ void e1ap_cu_cp_impl::handle_successful_outcome(const asn1::e1ap::successful_out
       }
 
       // Set transaction result and resume suspended procedure.
-      ev_mng.transactions.set(transaction_id.value(), outcome);
+      if (not ev_mng.transactions.set(transaction_id.value(), outcome)) {
+        logger.warning("Ignoring message. Cause: Transaction with id={} has already completed.",
+                       transaction_id.value());
+      }
   }
 }
 
@@ -259,6 +262,9 @@ void e1ap_cu_cp_impl::handle_unsuccessful_outcome(const asn1::e1ap::unsuccessful
       }
 
       // Set transaction result and resume suspended procedure.
-      ev_mng.transactions.set(transaction_id.value(), outcome);
+      if (not ev_mng.transactions.set(transaction_id.value(), outcome)) {
+        logger.warning("Ignoring message. Cause: Transaction with id={} has already completed.",
+                       transaction_id.value());
+      }
   }
 }

lib/e2/common/e2_impl.cpp

@@ -96,7 +96,9 @@ void e2_impl::handle_successful_outcome(const asn1::e2ap::successful_outcome_s&
         return;
       }
       // Set transaction result and resume suspended procedure.
-      events->transactions.set(transaction_id.value(), outcome);
+      if (not events->transactions.set(transaction_id.value(), outcome)) {
+        logger.warning("Unrecognized transaction id={}", transaction_id.value());
+      }
     } break;
     default:
       logger.error("Invalid E2AP successful outcome message type");
@@ -115,7 +117,9 @@ void e2_impl::handle_unsuccessful_outcome(const asn1::e2ap::unsuccessful_outcome
         return;
       }
       // Set transaction result and resume suspended procedure.
-      events->transactions.set(transaction_id.value(), outcome);
+      if (not events->transactions.set(transaction_id.value(), outcome)) {
+        logger.warning("Unrecognized transaction id={}", transaction_id.value());
+      }
     } break;
     default:
       logger.error("Invalid E2AP unsuccessful outcome message type");

lib/f1ap/du/f1ap_du_impl.cpp

@@ -269,19 +269,23 @@ void f1ap_du_impl::handle_successful_outcome(const asn1::f1ap::successful_outcom
   }
 
   // Set transaction result and resume suspended procedure.
-  events->transactions.set(transaction_id.value(), outcome);
+  if (not events->transactions.set(transaction_id.value(), outcome)) {
+    logger.warning("Unexpected transaction id={}", transaction_id.value());
+  }
 }
 
 void f1ap_du_impl::handle_unsuccessful_outcome(const asn1::f1ap::unsuccessful_outcome_s& outcome)
 {
   expected<uint8_t> transaction_id = get_transaction_id(outcome);
   if (transaction_id.is_error()) {
-    logger.error("Successful outcome of type {} is not supported", outcome.value.type().to_string());
+    logger.error("Unsuccessful outcome of type {} is not supported", outcome.value.type().to_string());
     return;
   }
 
   // Set transaction result and resume suspended procedure.
-  events->transactions.set(transaction_id.value(), outcome);
+  if (not events->transactions.set(transaction_id.value(), outcome)) {
+    logger.warning("Unexpected transaction id={}", transaction_id.value());
+  }
 }
 
 bool f1ap_du_impl::handle_rx_message_gnb_cu_ue_f1ap_id(f1ap_du_ue& ue, gnb_cu_ue_f1ap_id_t gnb_cu_ue_f1ap_id)

lib/f1ap/du/ue_context/f1c_du_bearer_impl.cpp

@@ -55,7 +55,9 @@ void f1c_srb0_du_bearer::handle_sdu(byte_buffer_slice_chain sdu)
   f1ap_notifier.on_new_message(msg);
 
   // Signal that the transaction has completed and the DU does not expect a response.
-  ev_manager.transactions.set(transaction.id(), f1ap_outcome{});
+  if (not ev_manager.transactions.set(transaction.id(), f1ap_outcome{})) {
+    logger.warning("Unexpected transaction id={}", transaction.id());
+  }
 
   log_ue_event(logger,
                ue_event_prefix{"UL", ue_ctxt.ue_index}.set_channel("SRB0") | ue_ctxt.rnti,

lib/rrc/ue/rrc_ue_message_handlers.cpp

@@ -157,7 +157,9 @@ void rrc_ue_impl::handle_rrc_transaction_complete(const ul_dcch_msg_s& msg, uint
   expected<uint8_t> transaction_id = transaction_id_;
 
   // Set transaction result and resume suspended procedure.
-  event_mng->transactions.set(transaction_id.value(), msg);
+  if (not event_mng->transactions.set(transaction_id.value(), msg)) {
+    logger.warning("Unexpected transaction id={}", transaction_id.value());
+  }
 }
 
 void rrc_ue_impl::handle_new_guami(const guami& msg)

tests/unittests/support/protocol_transaction_manager_test.cpp

@@ -54,7 +54,7 @@ TEST_F(protocol_transaction_test, when_transaction_is_set_after_suspend_then_awa
 
   // Test Section.
   ASSERT_FALSE(t.ready());
-  transaction_manager.set(tr.id(), 1);
+  ASSERT_TRUE(transaction_manager.set(tr.id(), 1));
   ASSERT_TRUE(t.ready());
   ASSERT_EQ(1, tr.result());
 }
@@ -66,7 +66,7 @@ TEST_F(protocol_transaction_test, when_transaction_is_set_before_suspend_then_aw
     CORO_BEGIN(ctx);
     EXPECT_FALSE(tr.complete());
     // transaction set before await.
-    this->transaction_manager.set(tr.id(), 2);
+    ASSERT_TRUE(this->transaction_manager.set(tr.id(), 2));
     CORO_AWAIT(tr);
     EXPECT_TRUE(tr.complete());
     CORO_RETURN(tr.result());
@@ -90,9 +90,9 @@ TEST_F(protocol_transaction_test,
 
   // Test Section.
   ASSERT_FALSE(t.ready());
-  transaction_manager.set(tr2.id(), 1);
+  ASSERT_TRUE(transaction_manager.set(tr2.id(), 1));
   ASSERT_FALSE(t.ready());
-  transaction_manager.set(tr.id(), 2);
+  ASSERT_TRUE(transaction_manager.set(tr.id(), 2));
   ASSERT_TRUE(t.ready());
 }