From e6218a9f921716306df06af9ec775ee8ee0118b2 Mon Sep 17 00:00:00 2001 From: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com> Date: Thu, 30 Jan 2025 10:11:45 -0800 Subject: [PATCH 1/4] Handle the output event of type TransferSession::OutputEventType::kInternalError in the ProcessOuputEvents processing loop - The transfer session state machine generates an output event of type TransferSession::OutputEventType::kInternalError in certain error scenarios which put the transfer session in a bad state and when that happens, we need to unwind the processing loop for events and clean up. --- .../bdx/AsyncTransferFacilitator.cpp | 26 +++++++++++++------ src/protocols/bdx/AsyncTransferFacilitator.h | 6 ++++- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/protocols/bdx/AsyncTransferFacilitator.cpp b/src/protocols/bdx/AsyncTransferFacilitator.cpp index 2e3a0f03794319..51bd0fd51e27ba 100644 --- a/src/protocols/bdx/AsyncTransferFacilitator.cpp +++ b/src/protocols/bdx/AsyncTransferFacilitator.cpp @@ -63,6 +63,17 @@ void AsyncTransferFacilitator::ProcessOutputEvents() mTransfer.GetNextAction(outEvent); while (outEvent.EventType != TransferSession::OutputEventType::kNone) { + + // If the transfer session state machine generates an event of type TransferSession::OutputEventType::kInternalError, + // indicating that the session is in a bad state, it will keep doing that thereafter. + // + // So stop trying to process events, and go ahead and destroy ourselves to clean up the transfer. + if (outEvent.EventType == TransferSession::OutputEventType::kInternalError) + { + mDestroySelfAfterProcessingEvents = true; + break; + } + if (outEvent.EventType == TransferSession::OutputEventType::kMsgToSend) { CHIP_ERROR err = SendMessage(outEvent.msgTypeData, outEvent.MsgData); @@ -170,21 +181,20 @@ void AsyncResponder::NotifyEventHandled(const TransferSession::OutputEventType e ChipLogDetail(BDX, "NotifyEventHandled : Event %s Error %" CHIP_ERROR_FORMAT, TransferSession::OutputEvent::TypeToString(eventType), status.Format()); - // If this is the end of the transfer (whether a clean end, or some sort of error condition), ensure that - // we destroy ourselves after processing any output events that might have been generated by - // the transfer session to handle end-of-transfer (e.g. in some error conditions it might want to send - // out a status report). + // If this is the end of the transfer (whether a clean end, or some sort of error condition), ensure + // that we destroy ourselves after unwinding the processing loop in the ProcessOutputEvents API. + // We can ignore the status for these messages since the state machine is in a bad, unrecoverable + // state and we should stop processing events and clean up. if (eventType == TransferSession::OutputEventType::kAckEOFReceived || + eventType == TransferSession::OutputEventType::kStatusReceived || eventType == TransferSession::OutputEventType::kInternalError || - eventType == TransferSession::OutputEventType::kTransferTimeout || - eventType == TransferSession::OutputEventType::kStatusReceived) + eventType == TransferSession::OutputEventType::kTransferTimeout) { mDestroySelfAfterProcessingEvents = true; } - // If there was an error handling the output event, this should notify the transfer object to abort transfer so it can send a // status report across the exchange when we call ProcessOutputEvents below. - if (status != CHIP_NO_ERROR) + else if (status != CHIP_NO_ERROR) { mTransfer.AbortTransfer(GetBdxStatusCodeFromChipError(status)); } diff --git a/src/protocols/bdx/AsyncTransferFacilitator.h b/src/protocols/bdx/AsyncTransferFacilitator.h index 1ad95d39e98d2d..39f87b741160c9 100644 --- a/src/protocols/bdx/AsyncTransferFacilitator.h +++ b/src/protocols/bdx/AsyncTransferFacilitator.h @@ -72,7 +72,11 @@ class AsyncTransferFacilitator : public Messaging::ExchangeDelegate */ virtual void DestroySelf() = 0; - // Calling ProcessOutputEvents can destroy this object before the call returns. + /** + * Calling ProcessOutputEvents can destroy this object before the call returns for certain cases + * where the state machine either receives a message indicating termination of the transfer session + * or some error cases where the state machine goes to a bad, unrecoverable state. + */ void ProcessOutputEvents(); // The transfer session corresponding to this AsyncTransferFacilitator object. From 27cd3edda4fefc48f53912941dea676f0ed479ff Mon Sep 17 00:00:00 2001 From: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com> Date: Fri, 31 Jan 2025 14:12:25 -0800 Subject: [PATCH 2/4] Update src/protocols/bdx/AsyncTransferFacilitator.cpp Co-authored-by: Boris Zbarsky --- src/protocols/bdx/AsyncTransferFacilitator.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/protocols/bdx/AsyncTransferFacilitator.cpp b/src/protocols/bdx/AsyncTransferFacilitator.cpp index 51bd0fd51e27ba..20e758efc3f591 100644 --- a/src/protocols/bdx/AsyncTransferFacilitator.cpp +++ b/src/protocols/bdx/AsyncTransferFacilitator.cpp @@ -183,8 +183,12 @@ void AsyncResponder::NotifyEventHandled(const TransferSession::OutputEventType e // If this is the end of the transfer (whether a clean end, or some sort of error condition), ensure // that we destroy ourselves after unwinding the processing loop in the ProcessOutputEvents API. - // We can ignore the status for these messages since the state machine is in a bad, unrecoverable - // state and we should stop processing events and clean up. + // We can ignore the status for output events because none of them are supposed to result in + // us sending a StatusReport, and that's all we use the status for. + // + // In particular, for kTransferTimeout, kAckEOFReceived, and kStatusReceived per spec we + // are not supposed to reply with a StatusReport. And for kInternalError the state machine + // is in an unrecoverable state of some sort, and we should stop trying to make use of it. if (eventType == TransferSession::OutputEventType::kAckEOFReceived || eventType == TransferSession::OutputEventType::kStatusReceived || eventType == TransferSession::OutputEventType::kInternalError || From 5d18e4aba124c4de8f325a1a76ab4d5768602e9e Mon Sep 17 00:00:00 2001 From: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com> Date: Fri, 31 Jan 2025 14:21:59 -0800 Subject: [PATCH 3/4] Address review comments --- src/protocols/bdx/AsyncTransferFacilitator.cpp | 8 ++++---- src/protocols/bdx/AsyncTransferFacilitator.h | 4 +--- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/protocols/bdx/AsyncTransferFacilitator.cpp b/src/protocols/bdx/AsyncTransferFacilitator.cpp index 20e758efc3f591..5093dc6cfea783 100644 --- a/src/protocols/bdx/AsyncTransferFacilitator.cpp +++ b/src/protocols/bdx/AsyncTransferFacilitator.cpp @@ -190,16 +190,16 @@ void AsyncResponder::NotifyEventHandled(const TransferSession::OutputEventType e // are not supposed to reply with a StatusReport. And for kInternalError the state machine // is in an unrecoverable state of some sort, and we should stop trying to make use of it. if (eventType == TransferSession::OutputEventType::kAckEOFReceived || - eventType == TransferSession::OutputEventType::kStatusReceived || eventType == TransferSession::OutputEventType::kInternalError || - eventType == TransferSession::OutputEventType::kTransferTimeout) + eventType == TransferSession::OutputEventType::kTransferTimeout || + eventType == TransferSession::OutputEventType::kStatusReceived) { mDestroySelfAfterProcessingEvents = true; } - // If there was an error handling the output event, this should notify the transfer object to abort transfer so it can send a - // status report across the exchange when we call ProcessOutputEvents below. else if (status != CHIP_NO_ERROR) { + // If there was an error handling the output event, this should notify the transfer object to abort transfer + // so it can send a status report across the exchange when we call ProcessOutputEvents below. mTransfer.AbortTransfer(GetBdxStatusCodeFromChipError(status)); } diff --git a/src/protocols/bdx/AsyncTransferFacilitator.h b/src/protocols/bdx/AsyncTransferFacilitator.h index 39f87b741160c9..b9d4b79bf55c5f 100644 --- a/src/protocols/bdx/AsyncTransferFacilitator.h +++ b/src/protocols/bdx/AsyncTransferFacilitator.h @@ -73,9 +73,7 @@ class AsyncTransferFacilitator : public Messaging::ExchangeDelegate virtual void DestroySelf() = 0; /** - * Calling ProcessOutputEvents can destroy this object before the call returns for certain cases - * where the state machine either receives a message indicating termination of the transfer session - * or some error cases where the state machine goes to a bad, unrecoverable state. + * Calling ProcessOutputEvents can destroy this object before the call returns. */ void ProcessOutputEvents(); From f17da93df37c6f18f488abcc42b29ddabe9c0ff1 Mon Sep 17 00:00:00 2001 From: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com> Date: Mon, 3 Feb 2025 11:21:39 -0800 Subject: [PATCH 4/4] Update src/protocols/bdx/AsyncTransferFacilitator.cpp Co-authored-by: Boris Zbarsky --- src/protocols/bdx/AsyncTransferFacilitator.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/protocols/bdx/AsyncTransferFacilitator.cpp b/src/protocols/bdx/AsyncTransferFacilitator.cpp index 5093dc6cfea783..eb3d996b1e53b8 100644 --- a/src/protocols/bdx/AsyncTransferFacilitator.cpp +++ b/src/protocols/bdx/AsyncTransferFacilitator.cpp @@ -183,7 +183,7 @@ void AsyncResponder::NotifyEventHandled(const TransferSession::OutputEventType e // If this is the end of the transfer (whether a clean end, or some sort of error condition), ensure // that we destroy ourselves after unwinding the processing loop in the ProcessOutputEvents API. - // We can ignore the status for output events because none of them are supposed to result in + // We can ignore the status for these output events because none of them are supposed to result in // us sending a StatusReport, and that's all we use the status for. // // In particular, for kTransferTimeout, kAckEOFReceived, and kStatusReceived per spec we