Skip to content

Commit

Permalink
ExchangeContext: Implement a safer verion Abort/Close
Browse files Browse the repository at this point in the history
  • Loading branch information
@kghost
kghost committed Jun 20, 2022
1 parent 97e9cb2 commit 432255d
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 91 deletions.
141 changes: 69 additions & 72 deletions src/messaging/ExchangeContext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,16 +228,38 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp
}
}

void ExchangeContext::DoClose(bool clearRetransTable)
void ExchangeContext::DoClose(bool clearRetransTable, bool shouldCallResponseTimout)
{
// Hold a ref to ourselves so we can make calls into our delegate that might
// decrease our refcount without worrying about use-after-free.
ExchangeHandle ref(*this);

VerifyOrDie(!mFlags.Has(Flags::kFlagClosed));
mFlags.Set(Flags::kFlagClosed);
// This releases refcount assigned in constructor. The initial refcount is associated with kFlagClosed. The DoClose function is
// not re-entrant, it is triggered once and only once when kFlagClosed is changing from false to ture.
Release();

// Clear protocol callbacks
if (mDelegate != nullptr)
bool callResponseTimout = false;

if (IsResponseExpected())
{
mDelegate->OnExchangeClosing(this);
// The application has closed the exchange, call its response timeout callback.
CancelResponseTimer();
SetResponseExpected(false);

// If we trigger the callback here and the user calls Close or Abort in the callback, it becomes too complicated to handle.
// Defer OnResponseTimeout callback until we have committed our state.
callResponseTimout = true;
}
else
{
// Either we're expecting a send or we are in our "just allocated, first send has not happened yet" state.
if (IsSendExpected())
{
mFlags.Clear(Flags::kFlagWillSendMessage);
}
}
mDelegate = nullptr;

// Closure of an exchange context is based on ref counting. The Protocol, when it calls DoClose(), indicates that
// it is done with the exchange context and the message layer sets all callbacks to NULL and does not send anything
Expand All @@ -252,42 +274,51 @@ void ExchangeContext::DoClose(bool clearRetransTable)
mExchangeMgr->GetReliableMessageMgr()->ClearRetransTable(this);
}

// Cancel the response timer.
CancelResponseTimer();
// Call deferred OnResponseTimeout
if (mDelegate != nullptr && callResponseTimout && shouldCallResponseTimout)
mDelegate->OnResponseTimeout(this);

// Check for mDelegate again, because mDelegate can be cleared during OnResponseTimeout event
if (mDelegate != nullptr)
mDelegate->OnExchangeClosing(this);

mDelegate = nullptr;
}

/**
* Gracefully close an exchange context. This call decrements the
* reference count and releases the exchange when the reference
* count goes to zero.
*
*/
void ExchangeContext::Close()
{
VerifyOrDie(mExchangeMgr != nullptr && GetReferenceCount() > 0);
if (mFlags.Has(Flags::kFlagClosed))
{
return;
}

#if defined(CHIP_EXCHANGE_CONTEXT_DETAIL_LOGGING)
ChipLogDetail(ExchangeManager, "ec - close[" ChipLogFormatExchange "], %s", ChipLogValueExchange(this), __func__);
#endif
if (IsResponseExpected())
{
// Close should not be called on a exchange which is waiting for a response. In normal procedure, close can only be called
// when an exchange is finishing its work. There is no chance that an exchange is finishing its work while waiting for a
// response.
//
// This is caused by a faulty implementation of an application, leave a error message for the application to fix it.
ChipLogError(ExchangeManager, "Close called when waiting for response on exchange " ChipLogFormatExchange,
ChipLogValueExchange(this));
}

DoClose(false);
Release();
// The graceful shutdown should take care of retransmitting the last message using MRP.
// Do not trigger OnResponseTimeout when user is closing the exchange.
DoClose(false /* clearRetransTable */, false /* shouldCallResponseTimout */);
}
/**
* Abort the Exchange context immediately and release all
* references to it.
*
*/

void ExchangeContext::Abort()
{
VerifyOrDie(mExchangeMgr != nullptr && GetReferenceCount() > 0);

#if defined(CHIP_EXCHANGE_CONTEXT_DETAIL_LOGGING)
ChipLogDetail(ExchangeManager, "ec - abort[" ChipLogFormatExchange "], %s", ChipLogValueExchange(this), __func__);
#endif
if (mFlags.Has(Flags::kFlagClosed))
{
// Exchange is already being closed. It may occur when Abort is called on a closed exchange.
mExchangeMgr->GetReliableMessageMgr()->ClearRetransTable(this);
return;
}

DoClose(true);
Release();
// Do not trigger OnResponseTimeout when user is aborting the exchange.
DoClose(true /* clearRetransTable */, false /* shouldCallResponseTimout */);
}

void ExchangeContextDeletor::Release(ExchangeContext * ec)
Expand All @@ -307,6 +338,8 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, cons
mSession.Grab(session);
mFlags.Set(Flags::kFlagInitiator, Initiator);
mFlags.Set(Flags::kFlagEphemeralExchange, isEphemeralExchange);
if (!isEphemeralExchange && Initiator)
WillSendMessage();
mDelegate = delegate;

SetAckPending(false);
Expand All @@ -333,11 +366,6 @@ ExchangeContext::~ExchangeContext()
UpdateSEDIntervalMode(false);
#endif

// Ideally, in this scenario, the retransmit table should
// be clear of any outstanding messages for this context and
// the boolean parameter passed to DoClose() should not matter.

DoClose(false);
mExchangeMgr = nullptr;

#if defined(CHIP_EXCHANGE_CONTEXT_DETAIL_LOGGING)
Expand Down Expand Up @@ -373,40 +401,12 @@ void ExchangeContext::OnSessionReleased()
{
if (mFlags.Has(Flags::kFlagClosed))
{
// Exchange is already being closed. It may occur when closing an exchange after sending
// RemoveFabric response which triggers removal of all sessions for the given fabric.
// Exchange is already closed.
mExchangeMgr->GetReliableMessageMgr()->ClearRetransTable(this);
return;
}

// Hold a ref to ourselves so we can make calls into our delegate that might
// decrease our refcount without worrying about use-after-free.
ExchangeHandle ref(*this);

if (IsResponseExpected())
{
// If we're waiting on a response, we now know it's never going to show up
// and we should notify our delegate accordingly.
CancelResponseTimer();
// We want to Abort, not just Close, so that RMP bits are cleared, so
// don't let NotifyResponseTimeout close us.
NotifyResponseTimeout(/* aCloseIfNeeded = */ false);
Abort();
}
else
{
// Either we're expecting a send or we are in our "just allocated, first
// send has not happened yet" state.
//
// Just mark ourselves as closed. The consumer is responsible for
// releasing us. See documentation for
// ExchangeDelegate::OnExchangeClosing.
if (IsSendExpected())
{
mFlags.Clear(Flags::kFlagWillSendMessage);
}
DoClose(true /* clearRetransTable */);
}
DoClose(true /* clearRetransTable */, true /* shouldCallResponseTimout */);
}

CHIP_ERROR ExchangeContext::StartResponseTimer()
Expand Down Expand Up @@ -440,10 +440,10 @@ void ExchangeContext::HandleResponseTimeout(System::Layer * aSystemLayer, void *
if (ec == nullptr)
return;

ec->NotifyResponseTimeout(/* aCloseIfNeeded = */ true);
ec->NotifyResponseTimeout();
}

void ExchangeContext::NotifyResponseTimeout(bool aCloseIfNeeded)
void ExchangeContext::NotifyResponseTimeout()
{
SetResponseExpected(false);

Expand All @@ -455,10 +455,7 @@ void ExchangeContext::NotifyResponseTimeout(bool aCloseIfNeeded)
delegate->OnResponseTimeout(this);
}

if (aCloseIfNeeded)
{
MessageHandled();
}
MessageHandled();
}

CHIP_ERROR ExchangeContext::HandleMessage(uint32_t messageCounter, const PayloadHeader & payloadHeader, MessageFlags msgFlags,
Expand Down
12 changes: 5 additions & 7 deletions src/messaging/ExchangeContext.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,12 +165,10 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,

uint16_t GetExchangeId() const { return mExchangeId; }

/*
* In order to use reference counting (see refCount below) we use a hold/free paradigm where users of the exchange
* can hold onto it while it's out of their direct control to make sure it isn't closed before everyone's ready.
* A customized version of reference counting is used since there are some extra stuff to do within Release.
*/
/// @brief Gracefully close an exchange context. This function can be call at any time on a valid exchange.
void Close();

/// @brief Abort the Exchange context immediately. This function can be call at any time on a valid exchange.
void Abort();

// Applies a suggested response timeout value based on the session type and the given upper layer processing time for
Expand Down Expand Up @@ -239,14 +237,14 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
* response. If aCloseIfNeeded is true, check whether the exchange needs to
* be closed.
*/
void NotifyResponseTimeout(bool aCloseIfNeeded);
void NotifyResponseTimeout();

CHIP_ERROR StartResponseTimer();

void CancelResponseTimer();
static void HandleResponseTimeout(System::Layer * aSystemLayer, void * aAppState);

void DoClose(bool clearRetransTable);
void DoClose(bool clearRetransTable, bool shouldCallResponseTimout);

/**
* We have handled an application-level message in some way and should
Expand Down
24 changes: 13 additions & 11 deletions src/messaging/ExchangeDelegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,17 +90,19 @@ class DLL_EXPORT ExchangeDelegate

/**
* @brief
* This function is the protocol callback to invoke when the associated
* exchange context is being closed.
*
* If the exchange was in a state where it was expecting a message to be
* sent due to an earlier WillSendMessage call or because the exchange has
* just been created as an initiator, the consumer is holding a reference
* to the exchange and it's the consumer's responsibility to call
* Release() on the exchange at some point. The usual way this happens is
* that the consumer tries to send its message, that fails, and the
* consumer calls Close() on the exchange. Calling Close() after an
* OnExchangeClosing() notification is allowed in this situation.
* This function is the protocol callback to invoke when the associated exchange context is being closed.
*
* The exchange is getting released soon after this callback. If there is no message pending in the retrans queue in MRP, the
* exchange will be released immediately after this callback, otherwise the exchange is released after the retrans entry is
* removed, by either receiving an acknowledgment or give up. There is no way to prevent the exchange from releasing at this
* stage.
*
* The user do not need to call Close or Abort to release the reference when this callback is triggered, the reference is
* already released. The user can call Abort if it want to clear the retrans entry of the last message, and speed up releasing
* the exchange.
*
* The user MUST release all pointers to the exchange in the callback, because the exchange is getting release soon, or else
* they become dangling pointer.
*
* @param[in] ec A pointer to the ExchangeContext object.
*/
Expand Down
3 changes: 2 additions & 1 deletion src/messaging/ReliableMessageContext.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,8 @@ class ReliableMessageContext
/// When set, signifies that this exchange is waiting for a call to SendMessage.
kFlagWillSendMessage = (1u << 6),

/// When set, we have had Close() or Abort() called on us already.
/// When set, signifies that the exchange is closed. All applications have already lost their refs to the exchange, and the
/// only possible ref is the retrans entry in MPR.
kFlagClosed = (1u << 7),

/// When set, signifies that the exchange is requesting Sleepy End Device active mode.
Expand Down

0 comments on commit 432255d

Please sign in to comment.