Fix liveness timer not firing for subscriptions #21693
Merged
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 #21693: Size comparison from b6859d7 to 85d699e Increases (41 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, p6, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (41 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, p6, telink)
|
yunhanw-google
approved these changes
Aug 5, 2022
lowered to 50ms (from 20s previously I believe), it breaks subscriptions setup in the tests that have a max interval of 0s. The timer fires way too rapidly for the test to function correctly. Fixed the tests to at least specify a max interval of 1s.
|
PR #21693: Size comparison from b6859d7 to ec5194d 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)
|
|
PR #21693: Size comparison from b6859d7 to eb996ce 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)
|
…tImpl to cover the chip-tool usecase
eb996ce to
7ed9d64
Compare
|
PR #21693: Size comparison from 34a8570 to 7ed9d64 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)
|
facbceb to
70b4ae2
Compare
70b4ae2 to
79d3447
Compare
bzbarsky-apple
approved these changes
Aug 8, 2022
bzbarsky-apple
requested changes
Aug 8, 2022
|
PR #21693: Size comparison from c3cc4a2 to 79d3447 Increases (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
MessagingContext and added MessagingContext::SetMRPMode to make it possible for some tests to set it to a more responsive modality.
bzbarsky-apple
approved these changes
Aug 9, 2022
|
PR #21693: Size comparison from 3860268 to 287a46b Increases (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
|
@mrjerryjohns can you rebase this? |
isiu-apple
pushed a commit
to isiu-apple/connectedhomeip
that referenced
this pull request
Sep 16, 2022
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.
Fixes #21546
On subscriptions created with
ReadClient::SendRequest,ReadClient::RefreshLivenessCheckTimerwas erroring out due to the incorrect assumption thatmReadPrepareParams.mSessionHolderwould betracking a valid session, which it isn't in the re-sub disabled case.
Fixes:
mReadPrepareParams.mSessionHolderalways track the latest valid session in all subscription related cases.RefreshLivenessCheckTimerto be the type of function that just returns an error and let callers deal with callingClose(). Fix up callers to handle the error returned appropriately.MessagingContexthelper to use a much lower idle/active retrans timeout values for the test sessions to facilitate faster tests. This should provide benefits across the board (e.gTestWriteChunkingdrops for 19s to 5s!). By doing altering it at this level, various tests that accelerated failure discovery by altering the liveness timeouts can go to a more representative model of failure injection by doing stuff like dropping packets (like a real network would) to simulate certain classes of failures instead, while still ensuring the tests run reasonably fast in CI.Tests:
TestSubscribeAttributeTimeouttest that checks for subscriptions timing out due to a liveness failure. Ensured that it failed prior to the change, and passes after.