Close exchanges automatically after a message send if no response is …

…expected (project-chip#8459)

examples/shell/shell_common/cmd_send.cpp

@@ -40,8 +40,6 @@ using namespace Logging;
 
 namespace {
 
-Messaging::ExchangeContext * gExchangeCtx = nullptr;
-
 class SendArguments
 {
 public:
@@ -110,16 +108,13 @@ class MockAppDelegate : public Messaging::ExchangeDelegate
         streamer_printf(sout, "Response received: len=%u time=%.3fms\n", buffer->DataLength(),
                         static_cast<double>(transitTime) / 1000);
 
-        gExchangeCtx = nullptr;
         return CHIP_NO_ERROR;
     }
 
     void OnResponseTimeout(Messaging::ExchangeContext * ec) override
     {
         streamer_t * sout = streamer_get();
         streamer_printf(sout, "No response received\n");
-
-        gExchangeCtx = nullptr;
     }
 } gMockAppDelegate;
 
@@ -131,17 +126,9 @@ CHIP_ERROR SendMessage(streamer_t * stream)
     System::PacketBufferHandle payloadBuf;
     uint32_t payloadSize = gSendArguments.GetPayloadSize();
 
-    // Discard any existing exchange context. Effectively we can only have one exchange with
-    // a single node at any one time.
-    if (gExchangeCtx != nullptr)
-    {
-        gExchangeCtx->Abort();
-        gExchangeCtx = nullptr;
-    }
-
     // Create a new exchange context.
-    gExchangeCtx = gExchangeManager.NewContext({ kTestDeviceNodeId, 0, gFabricIndex }, &gMockAppDelegate);
-    VerifyOrExit(gExchangeCtx != nullptr, err = CHIP_ERROR_NO_MEMORY);
+    auto * ec = gExchangeManager.NewContext({ kTestDeviceNodeId, 0, gFabricIndex }, &gMockAppDelegate);
+    VerifyOrExit(ec != nullptr, err = CHIP_ERROR_NO_MEMORY);
 
     payloadBuf = MessagePacketBuffer::New(payloadSize);
     VerifyOrExit(!payloadBuf.IsNull(), err = CHIP_ERROR_NO_MEMORY);
@@ -158,26 +145,24 @@ CHIP_ERROR SendMessage(streamer_t * stream)
         sendFlags.Set(Messaging::SendMessageFlags::kNoAutoRequestAck);
     }
 
-    gExchangeCtx->SetResponseTimeout(kResponseTimeOut);
+    ec->SetResponseTimeout(kResponseTimeOut);
     sendFlags.Set(Messaging::SendMessageFlags::kExpectResponse);
 
     gSendArguments.SetLastSendTime(System::Clock::GetMonotonicMilliseconds());
 
     streamer_printf(stream, "\nSend CHIP message with payload size: %d bytes to Node: %" PRIu64 "\n", payloadSize,
                     kTestDeviceNodeId);
 
-    err = gExchangeCtx->SendMessage(Protocols::Id(VendorId::Common, gSendArguments.GetProtocolId()),
-                                    gSendArguments.GetMessageType(), std::move(payloadBuf), sendFlags);
-
-    if (err != CHIP_NO_ERROR)
-    {
-        gExchangeCtx->Abort();
-        gExchangeCtx = nullptr;
-    }
+    err = ec->SendMessage(Protocols::Id(VendorId::Common, gSendArguments.GetProtocolId()), gSendArguments.GetMessageType(),
+                          std::move(payloadBuf), sendFlags);
 
 exit:
     if (err != CHIP_NO_ERROR)
     {
+        if (ec != nullptr)
+        {
+            ec->Close();
+        }
         streamer_printf(stream, "Send CHIP message failed, err: %s\n", ErrorStr(err));
     }
 

src/app/ReadHandler.cpp

@@ -36,7 +36,6 @@ CHIP_ERROR ReadHandler::Init(InteractionModelDelegate * apDelegate)
     // Error if already initialized.
     VerifyOrExit(mpExchangeCtx == nullptr, err = CHIP_ERROR_INCORRECT_STATE);
     mpExchangeCtx              = nullptr;
-    mpDelegate                 = apDelegate;
     mSuppressResponse          = true;
     mpAttributeClusterInfoList = nullptr;
     mpEventClusterInfoList     = nullptr;
@@ -52,25 +51,13 @@ void ReadHandler::Shutdown()
 {
     InteractionModelEngine::GetInstance()->ReleaseClusterInfoList(mpAttributeClusterInfoList);
     InteractionModelEngine::GetInstance()->ReleaseClusterInfoList(mpEventClusterInfoList);
-    AbortExistingExchangeContext();
+    mpExchangeCtx = nullptr;
     MoveToState(HandlerState::Uninitialized);
-    mpDelegate                 = nullptr;
     mpAttributeClusterInfoList = nullptr;
     mpEventClusterInfoList     = nullptr;
     mCurrentPriority           = PriorityLevel::Invalid;
 }
 
-CHIP_ERROR ReadHandler::AbortExistingExchangeContext()
-{
-    if (mpExchangeCtx != nullptr)
-    {
-        mpExchangeCtx->Abort();
-        mpExchangeCtx = nullptr;
-    }
-
-    return CHIP_NO_ERROR;
-}
-
 CHIP_ERROR ReadHandler::OnReadRequest(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
@@ -82,8 +69,6 @@ CHIP_ERROR ReadHandler::OnReadRequest(Messaging::ExchangeContext * apExchangeCon
     if (err != CHIP_NO_ERROR)
     {
         ChipLogFunctError(err);
-        // Keep Shutdown() from double-closing our exchange.
-        mpExchangeCtx = nullptr;
         Shutdown();
     }
 
@@ -96,6 +81,12 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload)
     VerifyOrExit(mpExchangeCtx != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
 
     err = mpExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::ReportData, std::move(aPayload));
+
+    if (err != CHIP_NO_ERROR)
+    {
+        mpExchangeCtx->Close();
+    }
+
 exit:
     ChipLogFunctError(err);
     Shutdown();

src/app/ReadHandler.h

@@ -129,10 +129,8 @@ class ReadHandler
     void MoveToState(const HandlerState aTargetState);
 
     const char * GetStateStr() const;
-    CHIP_ERROR AbortExistingExchangeContext();
 
     Messaging::ExchangeContext * mpExchangeCtx = nullptr;
-    InteractionModelDelegate * mpDelegate      = nullptr;
 
     // Don't need the response for report data if true
     bool mSuppressResponse = false;

src/app/WriteHandler.cpp

@@ -29,8 +29,6 @@ CHIP_ERROR WriteHandler::Init(InteractionModelDelegate * apDelegate)
 {
     VerifyOrReturnError(apDelegate != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(mpExchangeCtx == nullptr, CHIP_ERROR_INCORRECT_STATE);
-    mpExchangeCtx = nullptr;
-    mpDelegate    = apDelegate;
 
     System::PacketBufferHandle packet = System::PacketBufferHandle::New(chip::app::kMaxSecureSduLengthBytes);
     VerifyOrReturnError(!packet.IsNull(), CHIP_ERROR_NO_MEMORY);
@@ -50,20 +48,10 @@ void WriteHandler::Shutdown()
 {
     VerifyOrReturn(mState != State::Uninitialized);
     mMessageWriter.Reset();
-    ClearExistingExchangeContext();
-    mpDelegate = nullptr;
+    mpExchangeCtx = nullptr;
     ClearState();
 }
 
-void WriteHandler::ClearExistingExchangeContext()
-{
-    if (mpExchangeCtx != nullptr)
-    {
-        mpExchangeCtx->Close();
-        mpExchangeCtx = nullptr;
-    }
-}
-
 CHIP_ERROR WriteHandler::OnWriteRequest(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
@@ -75,8 +63,6 @@ CHIP_ERROR WriteHandler::OnWriteRequest(Messaging::ExchangeContext * apExchangeC
 
 exit:
     ChipLogFunctError(err);
-    // Keep Shutdown() from double-closing our exchange.
-    mpExchangeCtx = nullptr;
     Shutdown();
     return err;
 }

src/app/WriteHandler.h

@@ -99,10 +99,8 @@ class WriteHandler
     void MoveToState(const State aTargetState);
     void ClearState();
     const char * GetStateStr() const;
-    void ClearExistingExchangeContext();
 
     Messaging::ExchangeContext * mpExchangeCtx = nullptr;
-    InteractionModelDelegate * mpDelegate      = nullptr;
     WriteResponse::Builder mWriteResponseBuilder;
     System::PacketBufferTLVWriter mMessageWriter;
     State mState = State::Uninitialized;

src/app/tests/TestWriteInteraction.cpp

@@ -246,10 +246,6 @@ void TestWriteInteraction::TestWriteHandler(nlTestSuite * apSuite, void * apCont
     Messaging::ExchangeContext * exchange = ctx.NewExchangeToLocal(&delegate);
     err                                   = writeHandler.OnWriteRequest(exchange, std::move(buf));
     NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
-    // Manually close the exchange, because we're bypassing the normal "you
-    // received a message" flow for the exchange, so the automatic closing is
-    // not going to happen.
-    exchange->Close();
 
     Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr();
     NL_TEST_ASSERT(apSuite, rm->TestGetCountRetransTable() == 0);

src/app/util/chip-message-send.cpp

@@ -130,8 +130,12 @@ EmberStatus chipSendUnicast(NodeId destination, EmberApsFrame * apsFrame, uint16
 
     EmberStatus err = chipSendUnicast(exchange, apsFrame, messageLength, message, sendFlags);
 
-    // Make sure we always close the temporary exchange we just created.
-    exchange->Close();
+    // Make sure we always close the temporary exchange we just created, unless
+    // we sent a message successfully.
+    if (err != EMBER_SUCCESS)
+    {
+        exchange->Close();
+    }
 
     return err;
 }

src/channel/tests/TestChannel.cpp

@@ -111,11 +111,14 @@ void CheckExchangeChannels(nlTestSuite * inSuite, void * inContext)
     ec1->SendMessage(0x0001, 0x0002, System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize));
     NL_TEST_ASSERT(inSuite, !mockUnsolicitedAppDelegate.IsOnMessageReceivedCalled);
 
+    // Need to sort out what this test should really be testing and how; sending
+    // two messages in a row on an exchange is not something that really
+    // happens.
+
     // send a good packet
     ec1->SendMessage(0x0001, 0x0001, System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize));
     NL_TEST_ASSERT(inSuite, mockUnsolicitedAppDelegate.IsOnMessageReceivedCalled);
 
-    ec1->Close();
     channelHandle.Release();
 #endif
 }

src/controller/CHIPCluster.cpp

@@ -52,6 +52,7 @@ CHIP_ERROR ClusterBase::SendCommand(uint8_t seqNum, chip::System::PacketBufferHa
                                     Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
+    Messaging::SendFlags sendFlags;
 
     VerifyOrExit(mDevice != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
     VerifyOrExit(!payload.IsNull(), err = CHIP_ERROR_INTERNAL);
@@ -61,7 +62,12 @@ CHIP_ERROR ClusterBase::SendCommand(uint8_t seqNum, chip::System::PacketBufferHa
         mDevice->AddResponseHandler(seqNum, onSuccessCallback, onFailureCallback);
     }
 
-    err = mDevice->SendMessage(Protocols::TempZCL::MsgType::TempZCLRequest, std::move(payload));
+    if (onSuccessCallback != nullptr || onFailureCallback != nullptr)
+    {
+        sendFlags.Set(Messaging::SendMessageFlags::kExpectResponse);
+    }
+
+    err = mDevice->SendMessage(Protocols::TempZCL::MsgType::TempZCLRequest, sendFlags, std::move(payload));
     SuccessOrExit(err);
 
 exit:

src/controller/CHIPDevice.cpp

@@ -60,11 +60,11 @@ using namespace chip::Callback;
 
 namespace chip {
 namespace Controller {
-CHIP_ERROR Device::SendMessage(Protocols::Id protocolId, uint8_t msgType, System::PacketBufferHandle && buffer)
+CHIP_ERROR Device::SendMessage(Protocols::Id protocolId, uint8_t msgType, Messaging::SendFlags sendFlags,
+                               System::PacketBufferHandle && buffer)
 {
     System::PacketBufferHandle resend;
     bool loadedSecureSession = false;
-    Messaging::SendFlags sendFlags;
 
     VerifyOrReturnError(!buffer.IsNull(), CHIP_ERROR_INVALID_ARGUMENT);
 
@@ -372,7 +372,8 @@ CHIP_ERROR Device::OpenPairingWindow(uint32_t timeout, PairingWindowOption optio
     System::PacketBufferHandle outBuffer;
     ReturnErrorOnFailure(writer.Finalize(&outBuffer));
 
-    ReturnErrorOnFailure(SendMessage(Protocols::ServiceProvisioning::MsgType::ServiceProvisioningRequest, std::move(outBuffer)));
+    ReturnErrorOnFailure(SendMessage(Protocols::ServiceProvisioning::MsgType::ServiceProvisioningRequest,
+                                     Messaging::SendMessageFlags::kNone, std::move(outBuffer)));
 
     setupPayload.version               = 0;
     setupPayload.rendezvousInformation = RendezvousInformationFlags(RendezvousInformationFlag::kBLE);

src/controller/CHIPDevice.h

@@ -36,6 +36,7 @@
 #include <messaging/ExchangeContext.h>
 #include <messaging/ExchangeDelegate.h>
 #include <messaging/ExchangeMgr.h>
+#include <messaging/Flags.h>
 #include <protocols/secure_channel/CASESession.h>
 #include <protocols/secure_channel/PASESession.h>
 #include <protocols/secure_channel/SessionIDAllocator.h>
@@ -124,19 +125,22 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta
      *
      * @param[in] protocolId  The protocol identifier of the CHIP message to be sent.
      * @param[in] msgType     The message type of the message to be sent.  Must be a valid message type for protocolId.
+     * @param [in] sendFlags  SendMessageFlags::kExpectResponse or SendMessageFlags::kNone
      * @param[in] message     The message payload to be sent.
      *
      * @return CHIP_ERROR   CHIP_NO_ERROR on success, or corresponding error
      */
-    CHIP_ERROR SendMessage(Protocols::Id protocolId, uint8_t msgType, System::PacketBufferHandle && message);
+    CHIP_ERROR SendMessage(Protocols::Id protocolId, uint8_t msgType, Messaging::SendFlags sendFlags,
+                           System::PacketBufferHandle && message);
 
     /**
      * A strongly-message-typed version of SendMessage.
      */
     template <typename MessageType, typename = std::enable_if_t<std::is_enum<MessageType>::value>>
-    CHIP_ERROR SendMessage(MessageType msgType, System::PacketBufferHandle && message)
+    CHIP_ERROR SendMessage(MessageType msgType, Messaging::SendFlags sendFlags, System::PacketBufferHandle && message)
     {
-        return SendMessage(Protocols::MessageTypeTraits<MessageType>::ProtocolId(), to_underlying(msgType), std::move(message));
+        return SendMessage(Protocols::MessageTypeTraits<MessageType>::ProtocolId(), to_underlying(msgType), sendFlags,
+                           std::move(message));
     }
 
     CHIP_ERROR SendReadAttributeRequest(app::AttributePathParams aPath, Callback::Cancelable * onSuccessCallback,

src/controller/java/CHIPDeviceController-JNI.cpp

@@ -486,7 +486,9 @@ JNI_METHOD(void, sendMessage)(JNIEnv * env, jobject self, jlong handle, jlong de
         }
         else
         {
-            err = chipDevice->SendMessage(Protocols::TempZCL::MsgType::TempZCLRequest, std::move(buffer));
+            // We don't install a response handler, so aren't waiting for a response
+            err = chipDevice->SendMessage(Protocols::TempZCL::MsgType::TempZCLRequest, Messaging::SendMessageFlags::kNone,
+                                          std::move(buffer));
         }
     }
 

src/messaging/ExchangeContext.cpp

@@ -81,7 +81,9 @@ void ExchangeContext::SetResponseTimeout(Timeout timeout)
 CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgType, PacketBufferHandle && msgBuf,
                                         const SendFlags & sendFlags)
 {
-    if (protocolId != Protocols::SecureChannel::Id || msgType != to_underlying(Protocols::SecureChannel::MsgType::StandaloneAck))
+    bool isStandaloneAck =
+        (protocolId == Protocols::SecureChannel::Id) && msgType == to_underlying(Protocols::SecureChannel::MsgType::StandaloneAck);
+    if (!isStandaloneAck)
     {
         // If we were waiting for a message send, this is it.  Standalone acks
         // are not application-level sends, which is why we don't allow those to
@@ -148,6 +150,12 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp
             SetResponseExpected(false);
         }
 
+        // Standalone acks are not application-level message sends.
+        if (err == CHIP_NO_ERROR && !isStandaloneAck)
+        {
+            MessageHandled();
+        }
+
         return err;
     }
 }

src/messaging/ExchangeContext.h

@@ -80,6 +80,12 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public Referen
     /**
      *  Send a CHIP message on this exchange.
      *
+     *  If SendMessage returns success and the message was not expecting a
+     *  response, the exchange will close itself before returning, unless the
+     *  message being sent is a standalone ack.  If SendMessage returns failure,
+     *  the caller is responsible for deciding what to do (e.g. closing the
+     *  exchange, trying to re-establish a secure session, etc).
+     *
      *  @param[in]    protocolId    The protocol identifier of the CHIP message to be sent.
      *
      *  @param[in]    msgType       The message type of the corresponding protocol.
@@ -177,7 +183,7 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public Referen
     void SetResponseTimeout(Timeout timeout);
 
 private:
-    Timeout mResponseTimeout; // Maximum time to wait for response (in milliseconds); 0 disables response timeout.
+    Timeout mResponseTimeout       = 0; // Maximum time to wait for response (in milliseconds); 0 disables response timeout.
     ExchangeDelegate * mDelegate   = nullptr;
     ExchangeManager * mExchangeMgr = nullptr;
     ExchangeACL * mExchangeACL     = nullptr;