Skip to content

Commit

Permalink
Don't mark session defunct on response timeout if the message was acked.
Browse files Browse the repository at this point in the history
If we got an ack, the session is fine and there is no reason to mark it defunct.

Fixes #29165
  • Loading branch information
bzbarsky-apple committed Sep 11, 2023
1 parent 5ae3c65 commit a3bcd83
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 6 deletions.
13 changes: 10 additions & 3 deletions src/messaging/ExchangeContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -500,11 +500,18 @@ void ExchangeContext::NotifyResponseTimeout(bool aCloseIfNeeded)
// evicted.
if (mSession)
{
if (mSession->IsSecureSession() && mSession->AsSecureSession()->IsCASESession())
// If we timed out _after_ getting an ack for the message, that means
// the session is probably fine (since our message and the ack got
// through), so don't mark the session defunct unless we have an
// un-acked message here.
if (IsMessageNotAcked())
{
mSession->AsSecureSession()->MarkAsDefunct();
if (mSession->IsSecureSession() && mSession->AsSecureSession()->IsCASESession())
{
mSession->AsSecureSession()->MarkAsDefunct();
}
mSession->DispatchSessionEvent(&SessionDelegate::OnSessionHang);
}
mSession->DispatchSessionEvent(&SessionDelegate::OnSessionHang);
}

ExchangeDelegate * delegate = GetDelegate();
Expand Down
16 changes: 13 additions & 3 deletions src/messaging/ReliableMessageMgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -131,19 +131,22 @@ void ReliableMessageMgr::ExecuteActions()

if (sendCount == CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS)
{
// Make sure our exchange stays alive until we are done working with it.
ExchangeHandle ec(entry->ec);

ChipLogError(ExchangeManager,
"Failed to Send CHIP MessageCounter:" ChipLogFormatMessageCounter " on exchange " ChipLogFormatExchange
" sendCount: %u max retries: %d",
messageCounter, ChipLogValueExchange(&entry->ec.Get()), sendCount, CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS);
messageCounter, ChipLogValueExchange(&ec.Get()), sendCount, CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS);

// Don't check whether the session in the exchange is valid, because when the session is released, the retrans entry is
// cleared inside ExchangeContext::OnSessionReleased, so the session must be valid if the entry exists.
SessionHandle session = entry->ec->GetSessionHandle();
SessionHandle session = ec->GetSessionHandle();

// If the exchange is expecting a response, it will handle sending
// this notification once it detects that it has not gotten a
// response. Otherwise, we need to do it.
if (!entry->ec->IsResponseExpected())
if (!ec->IsResponseExpected())
{
if (session->IsSecureSession() && session->AsSecureSession()->IsCASESession())
{
Expand All @@ -154,6 +157,13 @@ void ReliableMessageMgr::ExecuteActions()

// Do not StartTimer, we will schedule the timer at the end of the timer handler.
mRetransTable.ReleaseObject(entry);

// That marked the exchange as not having an un-acked message... but
// of course it _does_ have an un-acked message and we have just
// given up on waiting for the ack.

ec->GetReliableMessageContext()->SetMessageNotAcked(true);

return Loop::Continue;
}

Expand Down

0 comments on commit a3bcd83

Please sign in to comment.