Don't mark session defunct on response timeout if the message was acked. #29173
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
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
gjc13,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kkasperczyk-no,
ksperling-apple,
lazarkov,
lpbeliveau-silabs,
LuDuda,
mhazley and
mkardous-silabs
pullapprove
bot
requested review from
tecimovic,
tehampson,
tima-q,
tobiasgraf,
turon,
vivien-apple,
woody-apple,
younghak-hwang and
yufengwangca
tcarmelveilleux
approved these changes
Sep 11, 2023
|
PR #29173: Size comparison from 8d0761d to a3bcd83 Increases (65 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (4 builds for psoc6)
Full report (65 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
yunhanw-google
approved these changes
Sep 12, 2023
|
PR #29173: Size comparison from 8d0761d to 0e6fdb3 Increases (65 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (4 builds for psoc6)
Full report (65 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
saurabhst
approved these changes
Sep 12, 2023
If we got an ack, the session is fine and there is no reason to mark it defunct. Fixes project-chip#29165
bzbarsky-apple
force-pushed
the
no-defunct-unless-unacked
branch
from
0e6fdb3
to
b512b3e
Compare
|
PR #29173: Size comparison from b02c984 to b512b3e Increases (65 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (4 builds for psoc6)
Full report (65 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
bzbarsky-apple
added a commit
to bzbarsky-apple/connectedhomeip
that referenced
this pull request
Oct 6, 2023
After project-chip#29173 we can get into the following situation: 1. A message is sent. 2. Before we get an ack or response, all MRP retries happen, MRP gives up, but the exchange response timer has not been hit yet. 3. We get an actual response. 4. Because our exchange is marked as having an un-acked message, but the incoming message is not treated as an ack (because the MRP state that would do that has been torn down), we do not clear our "have un-acked message" state and end up discarding the incoming message. The fix is as follows: * Rename things to make it clear that what we really have is "waiting for an ack" state, which in fact _does_ get cleared when we run out of MRP retries, not an "un-acked message" state. * Have a separate state bit for tracking that we ran out of MRP retries on a message we sent.
bzbarsky-apple
added a commit
to bzbarsky-apple/connectedhomeip
that referenced
this pull request
Oct 6, 2023
After project-chip#29173 we can get into the following situation: 1. A message is sent. 2. Before we get an ack or response, all MRP retries happen, MRP gives up, but the exchange response timer has not been hit yet. 3. We get an actual response. 4. Because our exchange is marked as having an un-acked message, but the incoming message is not treated as an ack (because the MRP state that would do that has been torn down), we do not clear our "have un-acked message" state and end up discarding the incoming message. The fix is as follows: * Rename things to make it clear that what we really have is "waiting for an ack" state, which in fact _does_ get cleared when we run out of MRP retries, not an "un-acked message" state. * Have a separate state bit for tracking that we ran out of MRP retries on a message we sent.
mergify bot
pushed a commit
that referenced
this pull request
Oct 9, 2023
* Fix handling of a response that comes after MRP resends end. After #29173 we can get into the following situation: 1. A message is sent. 2. Before we get an ack or response, all MRP retries happen, MRP gives up, but the exchange response timer has not been hit yet. 3. We get an actual response. 4. Because our exchange is marked as having an un-acked message, but the incoming message is not treated as an ack (because the MRP state that would do that has been torn down), we do not clear our "have un-acked message" state and end up discarding the incoming message. The fix is as follows: * Rename things to make it clear that what we really have is "waiting for an ack" state, which in fact _does_ get cleared when we run out of MRP retries, not an "un-acked message" state. * Have a separate state bit for tracking that we ran out of MRP retries on a message we sent. * Address review comment.
HunsupJung
pushed a commit
to HunsupJung/connectedhomeip
that referenced
this pull request
Oct 23, 2023
…ed. (project-chip#29173) * Don't mark session defunct on response timeout if the message was acked. If we got an ack, the session is fine and there is no reason to mark it defunct. Fixes project-chip#29165 * Address review comment.
HunsupJung
pushed a commit
to HunsupJung/connectedhomeip
that referenced
this pull request
Oct 23, 2023
…-chip#29640) * Fix handling of a response that comes after MRP resends end. After project-chip#29173 we can get into the following situation: 1. A message is sent. 2. Before we get an ack or response, all MRP retries happen, MRP gives up, but the exchange response timer has not been hit yet. 3. We get an actual response. 4. Because our exchange is marked as having an un-acked message, but the incoming message is not treated as an ack (because the MRP state that would do that has been torn down), we do not clear our "have un-acked message" state and end up discarding the incoming message. The fix is as follows: * Rename things to make it clear that what we really have is "waiting for an ack" state, which in fact _does_ get cleared when we run out of MRP retries, not an "un-acked message" state. * Have a separate state bit for tracking that we ran out of MRP retries on a message we sent. * 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.
If we got an ack, the session is fine and there is no reason to mark it defunct.
Fixes #29165