-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Send invalid action status when receiving unexpected message during invoke/read/subscribe/write #19356
Send invalid action status when receiving unexpected message during invoke/read/subscribe/write #19356
Conversation
PR #19356: Size comparison from 57cb679 to c079ae4 Increases (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Decreases (5 builds for cc13x2_26x2)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
c079ae4
to
a79623e
Compare
a79623e
to
3f74629
Compare
PR #19356: Size comparison from 1d88b32 to 3f74629 Increases (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
3f74629
to
33e1be6
Compare
33e1be6
to
1235b2e
Compare
PR #19356: Size comparison from 2987329 to 1235b2e Increases (40 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, linux, mbed, nrfconnect, p6, telink)
Decreases (14 builds for cc13x2_26x2, linux)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
1235b2e
to
7450f57
Compare
@@ -186,7 +181,7 @@ void CommandHandler::DecrementHoldOff() | |||
return; | |||
} | |||
|
|||
if (!mExchangeCtx->IsGroupExchangeContext() && !mSentStatusResponse) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this now going to cause us to send two messages: the IM engine sending a status response (see this), and this here sending a response?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, given the bugs in the code and the bugs in the tests (e.g. the message-dropping code) and the pain of reviewing this huge thing every time (I just needed a 2-hour solid chunk to go through it, and I have zero confidence I found all the bugs), I would like to suggest the following break-up of this into multiple PRs:
- A PR to change NetworkTestHelpers to allow the behavior we want. That behavior is "allow through N messages, then block M messages", right? I think that the right way to do that is to have a "number of messages we have let through before we start blocking" counter that counts down to 0, instead of tying it to the unrelated "total messages we have ever sent" counter. I'm happy to do this PR if you want.
- A PR to change the signature of StatusResponse parsing, so we are not conflating the mechanical changes there with the substantive changes in this PR.
- Separate PRs for commands/reads/writes, or better yet for each client/server bit (so 3 or 6 PRs). That will hopefully lead to PRs that are not too big, test changes that are not too big, and the whole thing can be reviewed sanely and actually land in a finite amount of time.... Importantly, we can parallelize and do reviews on one while another is being updated, etc.
|
||
if (mExchangeCtx->IsGroupExchangeContext()) | ||
{ | ||
ReturnErrorOnFailure(ProcessGroupCommandDataIB(commandData)); | ||
VerifyOrReturnError(ProcessGroupCommandDataIB(commandData) == CHIP_NO_ERROR, Status::Failure); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this (and ProcessCommandData
) Status::Failure? Please file a followup to have those return a useful status for parsing errors in the payloads....
@@ -186,7 +181,7 @@ void CommandHandler::DecrementHoldOff() | |||
return; | |||
} | |||
|
|||
if (!mExchangeCtx->IsGroupExchangeContext() && !mSentStatusResponse) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few problems here:
- The IM engine will not in fact be able to send a status response, as far as I can tell. That's because we're going to
Close
at the bottom of this function, which will destroy this object, but we grabbed the exchange into our exchange holder already and it's expecting a send because we calledWillSendMessage
, so we're going to abort the exchange. That looks like a bug that got introduced with the ExchangeHolder PR @mrjerryjohns . :( - If we are returning failure to the IM engine so it will send a status response, we do not want to send anything here.
It seems like we might want to:
- Not call
WillSendMessage
on the exchange until we return successfully fromProcessInvokeRequest
. - If
ProcessInvokeRequest
fails, Release our mExchangeCtx so we don't do any message sending here.
or something like that.
It also seems like we should break this apart into three (if not 6) separate PRs so we don't end up needing a huge time-sink of a review very time and don't block fixes to some of these objects because of problems with others. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not call WillSendMessage on the exchange until we return successfully from ProcessInvokeRequest.
If ProcessInvokeRequest fails, Release our mExchangeCtx so we don't do any message sending here.
Part of the reason why this bug is happening and is being exposed is the jarring mismatch between the expectation that the IM engine still be involved in sending out some messages on an exchange (i.e this final status response) vs. the CommandHandler
taking over management of that exchange.
I have posted on this PR earlier, but I think that model is dangerous. Once you transfer ownership and management of the EC to a more suited protocol object, it should be the one responsible for sending back status responses and the like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To that end, I don't think either solution suggested would be favorable for me. I'd rather CommandHandler (and I guess by extension), all the other IM handler objects manage sending their own StatusResponses. That is the cleanest, and safest.
{ | ||
System::PacketBufferTLVReader reader; | ||
reader.Init(aPayload.Retain()); | ||
|
||
ReportDataMessage::Parser report; | ||
ReturnErrorOnFailure(report.Init(reader)); | ||
VerifyOrReturnError(report.Init(reader) == CHIP_NO_ERROR, Status::InvalidSubscription); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be InvalidAction
? Same for the other parse failures here....
StatusResponse::Send(Protocols::InteractionModel::Status::InvalidAction, apExchangeContext, false /*aExpectResponse*/); | ||
if (!aSuppressErrorStatusResponse) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make any sense. Why is aSuppressErrorStatusResponse
affecting calls to mpCallback
but not affecting the sending of the status response? And how are tests possibly passing with this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure to point me to the tests that would have failed due to this bug in the updates.
StatusResponse::Send(Protocols::InteractionModel::Status::InvalidAction, apExchangeContext, false /*aExpectResponse*/); | ||
if (mpCallback != nullptr) | ||
{ | ||
mpCallback->OnError(this, aError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we not making our OnError call if aSuppressErrorStatusResponse
is true? That's broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, there should be tests covering this, and the fact that there are not is really confusing to me.
@@ -102,19 +102,37 @@ class LoopbackTransport : public Transport::Base | |||
{ | |||
ReturnErrorOnFailure(mMessageSendError); | |||
mSentMessageCount++; | |||
|
|||
if (mNumMessagesToDrop == 0) | |||
if (mNumMessagesToDropSinceIndex == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this get to be 0 if it's ever set nonzero.
PR #19356: Size comparison from 6cd4676 to 7450f57 Increases (31 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Decreases (7 builds for cc13x2_26x2)
Full report (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
Problem
Based upon IM spec,
we need to send invalid action status when receiving unexpected message during invoke/read/subscribe/write. and process this status in other side accordingly.
Fix: #8030
Change overview
see above
Testing
The existing tests covers the happy paths.
Add new tests covers the negative paths.