Skip to content

Commit

Permalink
Make it easier to catch bugs in async command handling. (#25402)
Browse files Browse the repository at this point in the history
If GetSubjectDescriptor or GetAccessingFabricIndex were called on a
CommandHandler after the command handling had gone async, it would sometimes
crash, and sometimes not, depending on whether sessions had been evicted or not.

Make that situation always crash, so that it will be caught more easily in
testing.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Dec 21, 2023
1 parent c039265 commit 1050960
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 1 deletion.
3 changes: 3 additions & 0 deletions src/app/CommandHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ void CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * ec, con
StatusResponse::Send(status, mExchangeCtx.Get(), false /*aExpectResponse*/);
mSentStatusResponse = true;
}

mGoneAsync = true;
}

Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payload, bool isTimedInvoke)
Expand Down Expand Up @@ -577,6 +579,7 @@ TLV::TLVWriter * CommandHandler::GetCommandDataIBTLVWriter()

FabricIndex CommandHandler::GetAccessingFabricIndex() const
{
VerifyOrDie(!mGoneAsync);
return mExchangeCtx->GetSessionHandle()->GetFabricIndex();
}

Expand Down
23 changes: 22 additions & 1 deletion src/app/CommandHandler.h
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,13 @@ class CommandHandler : public Messaging::ExchangeDelegate
CHIP_ERROR PrepareStatus(const ConcreteCommandPath & aCommandPath);
CHIP_ERROR FinishStatus();
TLV::TLVWriter * GetCommandDataIBTLVWriter();

/**
* GetAccessingFabricIndex() may only be called during synchronous command
* processing. Anything that runs async (while holding a
* CommandHandler::Handle or equivalent) must not call this method, because
* it will not work right if the session we're using was evicted.
*/
FabricIndex GetAccessingFabricIndex() const;

/**
Expand Down Expand Up @@ -272,7 +279,17 @@ class CommandHandler : public Messaging::ExchangeDelegate
msgContext->FlushAcks();
}

Access::SubjectDescriptor GetSubjectDescriptor() const { return mExchangeCtx->GetSessionHandle()->GetSubjectDescriptor(); }
/**
* GetSubjectDescriptor() may only be called during synchronous command
* processing. Anything that runs async (while holding a
* CommandHandler::Handle or equivalent) must not call this method, because
* it might not work right if the session we're using was evicted.
*/
Access::SubjectDescriptor GetSubjectDescriptor() const
{
VerifyOrDie(!mGoneAsync);
return mExchangeCtx->GetSessionHandle()->GetSubjectDescriptor();
}

private:
friend class TestCommandInteraction;
Expand Down Expand Up @@ -394,6 +411,10 @@ class CommandHandler : public Messaging::ExchangeDelegate
chip::System::PacketBufferTLVWriter mCommandMessageWriter;
TLV::TLVWriter mBackupWriter;
bool mBufferAllocated = false;
// If mGoneAsync is true, we have finished out initial processing of the
// incoming invoke. After this point, our session could go away at any
// time.
bool mGoneAsync = false;
};

} // namespace app
Expand Down

0 comments on commit 1050960

Please sign in to comment.