Fix invariant violation if we get a message without piggyback ack while expecting an ack. #23282
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
…le expecting an ack. Such messages are not allowed per spec, so we should just ignore them. Fixes project-chip#22854
pullapprove
bot
requested review from
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
lazarkov,
LuDuda,
mlepage-google,
msandstedt,
mspang and
rgoliver
pullapprove
bot
requested review from
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yunhanw-google
bzbarsky-apple
changed the title
|
PR #23282: Size comparison from d7cd5aa to a5a4b20 Increases (17 builds for bl602, bl702, efr32, esp32, linux, psoc6, telink)
Decreases (3 builds for cyw30739, k32w, qpg)
Full report (38 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
andy31415
reviewed
Oct 20, 2022
andy31415
approved these changes
Oct 20, 2022
jmartinez-silabs
approved these changes
Oct 20, 2022
|
The test failures are somewhat real in the sense that we have tests that manually remove (some but not enough) retrans table entries and then send two messages in a row on the same exchange without having gotten a reply to the first one (which gets dropped in the test). Those tests need adjusting to deal with this behavior, because they are in fact relying on these messages getting delivered to the other side and the other side responding. @turon I tried just clearing all the retrans entries, but then that drops the exchange on the server entirely, and we actually get different behavior because an injected unexpected Status Response message is handled differently depending on whether it matches a closed but still alive exchange or does not match any exchange at all. In the latter case, @yunhanw-google do you have time to sort out how these tests (TestReadInteraction, TestWriteInteraction, TestCommandInteraction) should handle this? |
bzbarsky-apple
force-pushed
the
fix-missing-ack-crash
branch
from
3da0c51 to
683f467
Compare
|
PR #23282: Size comparison from 0998742 to 683f467 Increases (46 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (49 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
I decided to fix the tests to make it look like a response from the server had in fact been received and just ignored at the data model layer instead of dropped at the message layer. That means:
|
bzbarsky-apple
added a commit
to bzbarsky-apple/connectedhomeip
that referenced
this pull request
Oct 25, 2022
…le expecting an ack. (project-chip#23282) * Fix invariant violation if we get a message without piggyback ack while expecting an ack. Such messages are not allowed per spec, so we should just ignore them. Fixes project-chip#22854 * Fix tests. * Address review comment.
bzbarsky-apple
added a commit
to bzbarsky-apple/connectedhomeip
that referenced
this pull request
Oct 25, 2022
…le expecting an ack. (project-chip#23282) * Fix invariant violation if we get a message without piggyback ack while expecting an ack. Such messages are not allowed per spec, so we should just ignore them. Fixes project-chip#22854 * Fix tests. * Address review comment.
adbridge
pushed a commit
to ARM-software/connectedhomeip
that referenced
this pull request
Nov 18, 2022
…le expecting an ack. (project-chip#23282) * Fix invariant violation if we get a message without piggyback ack while expecting an ack. Such messages are not allowed per spec, so we should just ignore them. Fixes project-chip#22854 * Fix tests. * Address review comment.
adbridge
pushed a commit
to ARM-software/connectedhomeip
that referenced
this pull request
Nov 18, 2022
…le expecting an ack. (project-chip#23282) * Fix invariant violation if we get a message without piggyback ack while expecting an ack. Such messages are not allowed per spec, so we should just ignore them. Fixes project-chip#22854 * Fix tests. * Address review comment.
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.
Such messages are not allowed per spec, so we should just ignore them.
Fixes #22854