Add invoke command test for invoke boundary conditions on reply too large
#30643
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
PR #30643: Size comparison from b5dfe72 to a62917b Full report (5 builds for cc32xx, mbed, qpg)
|
|
PR #30643: Size comparison from b5dfe72 to ce74753 Full report (5 builds for cc32xx, mbed, qpg)
|
tehampson
reviewed
Nov 24, 2023
| continue; // Unclear why these fail | ||
| } | ||
|
|
||
| if ((replySize >= 1162) && (replySize <= 1165)) { |
There was a problem hiding this comment.
If I understand this correctly it has to do with the Rollback/Checkpoint stuff. We fail in just the right way that we rollback to a state where the array closed, but the InvokeReponseMessage fails and then it proceeds to rollback
There was a problem hiding this comment.
This is what the test intends to validate - I am surprised we have 2 failure modes though. For very large buffers we obviously eventually succeed.
There was a problem hiding this comment.
Chatted about this with Andrei. But will post here for folks reading.
The second issue Crash due to MessageBuilder::EncodeInteractionModelRevision() encoding failure is likely because it errors out on ReturnErrorOnFailure(mInvokeResponseBuilder.EndOfInvokeResponseMessage()); here while in TryAddResponseData, this cause a rollback where mInvokeResponseBuilder.GetInvokeResponses(). is in the wrong state and no longer has an outer container. When it gets call a second time in the FinishStatus here you get the error.
The first error is likely some other weirdness of rollback interacting in a weird way where we end some of the containers
There was a problem hiding this comment.
It looks like OpenIOT and NRF have different limits here (guessing maybe LWIP buffer allocation ranges maybe).
I am unsure if we want to try to have this test in and then improve or just use it as a template to fix the bugs we have and then commit some version of it only once it consistently passes ...
pullapprove
bot
requested review from
andyg-apple,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy and
joonhaengHeo
pullapprove
bot
requested review from
pidarped,
plauric,
rcasallas-silabs,
robszewczyk,
saurabhst,
selissia,
sharadb-amazon,
tcarmelveilleux,
tecimovic,
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
|
PR #30643: Size comparison from b5dfe72 to 0f3624b Full report (15 builds for cc13x4_26x4, cc32xx, k32w, mbed, qpg)
|
|
PR #30643: Size comparison from b5dfe72 to a360adf Full report (5 builds for cc32xx, mbed, qpg)
|
|
PR #30643: Size comparison from b5dfe72 to eaaf8c7 Full report (5 builds for cc32xx, mbed, qpg)
|
|
PR #30643: Size comparison from b5dfe72 to 732fded Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, psoc6, qpg, telink)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Expectation for the codebase is that replies either go through or we manage to send an error reply (so that exchanges are not stuck).
This creates a draft test that is intended to go over all boundery conditions and validate our rollback capabilities.
Also fixes test sequencing issues where global state variables are not resetted properly. I switched for them to be scoped/changed within each test only.
NOTE - this shows bugs
This test is supposed to pass always, but it does NOT. See ranges and commends of reply sizes:
For even larger buffers, our error handling is ok. We should have followup changes to fix this logic.