Skip to content

Commit

Permalink
Fix management of the mNumReportsInFlight count in reporting engine.
Browse files Browse the repository at this point in the history
If a ReadHandler failed out of SendReportData (e.g. because the session it's on
had been marked as defunct), we would increment mNumReportsInFlight and never
decrement it.  After this happened CHIP_IM_MAX_REPORTS_IN_FLIGHT times (4 by
default), we would stop being able to send out any more data reports.

This situation is pretty easy to trigger as follows:

1. Use chip-tool to commission a device with node id 17.
2. Start chip-tool interactive mode.
3. Run the following commands in interactive mode:

   onoff subscribe on-off 0 60 17 1 --keepSubscriptions true
   onoff subscribe on-off 0 60 17 1 --keepSubscriptions true
   onoff subscribe on-off 0 60 17 1 --keepSubscriptions true
   onoff subscribe on-off 0 60 17 1 --keepSubscriptions true
   onoff subscribe on-off 0 2 17 1 --keepSubscriptions true

4. quit interactive mode (Ctrl-C or quit() command).
5. Wait 60 seconds for all the subscriptions to error out.

After this the device will no longer respond with data reports to any
read or subscribe requests.
  • Loading branch information
bzbarsky-apple committed Dec 15, 2022
1 parent 127deca commit 9541715
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 15 deletions.
35 changes: 20 additions & 15 deletions src/app/ReadHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ CHIP_ERROR ReadHandler::SendStatusReport(Protocols::InteractionModel::Status aSt
CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, bool aMoreChunks)
{
VerifyOrReturnLogError(IsReportable(), CHIP_ERROR_INCORRECT_STATE);
VerifyOrDie(!IsAwaitingReportResponse()); // Should not be reportable!
if (IsPriming() || IsChunkedReport())
{
mSessionHandle.Grab(mExchangeCtx->GetSessionHandle());
Expand All @@ -230,27 +231,31 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b
mCurrentReportsBeginGeneration = InteractionModelEngine::GetInstance()->GetReportingEngine().GetDirtySetGeneration();
}
SetStateFlag(ReadHandlerFlags::ChunkedReport, aMoreChunks);
bool noResponseExpected = IsType(InteractionType::Read) && !aMoreChunks;
if (!noResponseExpected)
{
MoveToState(HandlerState::AwaitingReportResponse);
}
bool responseExpected = IsType(InteractionType::Subscribe) || aMoreChunks;

mExchangeCtx->UseSuggestedResponseTimeout(app::kExpectedIMProcessingTime);
CHIP_ERROR err =
mExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::ReportData, std::move(aPayload),
Messaging::SendFlags(noResponseExpected ? Messaging::SendMessageFlags::kNone
: Messaging::SendMessageFlags::kExpectResponse));
if (err == CHIP_NO_ERROR && noResponseExpected)
{
InteractionModelEngine::GetInstance()->GetReportingEngine().OnReportConfirm();
}

CHIP_ERROR err = mExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::ReportData, std::move(aPayload),
responseExpected ? Messaging::SendMessageFlags::kExpectResponse
: Messaging::SendMessageFlags::kNone);
if (err == CHIP_NO_ERROR)
{
if (responseExpected)
{
MoveToState(HandlerState::AwaitingReportResponse);
}
else
{
// Make sure we're not treated as an in-flight report waiting for a
// response by the reporting engine.
InteractionModelEngine::GetInstance()->GetReportingEngine().OnReportConfirm();
}

if (IsType(InteractionType::Subscribe) && !IsPriming())
{
err = RefreshSubscribeSyncTimer();
// Ignore the error from RefreshSubscribeSyncTimer. If we've
// successfully sent the message, we need to return success from
// this method.
RefreshSubscribeSyncTimer();
}
}
if (!aMoreChunks)
Expand Down
2 changes: 2 additions & 0 deletions src/app/ReadHandler.h
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,8 @@ class ReadHandler : public Messaging::ExchangeDelegate
* @retval #Others If fails to send report data
* @retval #CHIP_NO_ERROR On success.
*
* If an error is returned, the ReadHandler guarantees that it is not in
* a state where it's waiting for a response.
*/
CHIP_ERROR SendReportData(System::PacketBufferHandle && aPayload, bool aMoreChunks);

Expand Down
4 changes: 4 additions & 0 deletions src/app/reporting/Engine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -848,6 +848,10 @@ CHIP_ERROR Engine::SendReport(ReadHandler * apReadHandler, System::PacketBufferH
// We can only have 1 report in flight for any given read - increment and break out.
mNumReportsInFlight++;
err = apReadHandler->SendReportData(std::move(aPayload), aHasMoreChunks);
if (err != CHIP_NO_ERROR)
{
--mNumReportsInFlight;
}
return err;
}

Expand Down

0 comments on commit 9541715

Please sign in to comment.