[Python] Only auto re-subscribe after initial subscription #34370
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 #34370: Size comparison from df8c9da to a0dbc95 Full report (20 builds for cc13x4_26x4, cc32xx, mbed, nrfconnect, nxp, qpg, stm32, tizen)
|
The subscription logic waits for the first successful subscription before the Read() call is being returned (the future is awaited which is only released on handleSubscriptionEstablished). If the first subscription attempt fails (e.g. because the CASE session doesn't establish) the Read() never returns, not with an error but also not with a subscription transaction. And since the Python side has no access to the SubscriptionTransaction object at this point yet, there is also no way to stop this subscription attempt. With this change, we only resubscribe if the initial subscription was successful. This changes semantics slightly, but really allows the caller to decide if it wants to continue try to establish the subscription.
agners
force-pushed
the
only-auto-resubscribe-if-subscription-succeeded-once
branch
from
a0dbc95 to
f2546cb
Compare
|
PR #34370: Size comparison from df8c9da to f2546cb Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
axelnxp,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chapongatien,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
doru91,
fessehaeve,
harimau-qirex,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs and
jmartinez-silabs
pullapprove
bot
requested review from
sharadb-amazon,
tecimovic,
tehampson,
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
cecille
approved these changes
Jul 17, 2024
|
Stefan - if you have some time, would you mind adding that description into the function docstring around the autoResubscribe parameter? |
Yeah good point. I've created a new PR with some other missing parameters: #34380. |
tehampson
approved these changes
Jul 17, 2024
arkq
approved these changes
Jul 18, 2024
agners
added a commit
to home-assistant-libs/chip-wheels
that referenced
this pull request
Jul 18, 2024
This adds Subscription fixes from the master branch to our 1.3 branch. Specifically, this only auto re-subscribes after an initial subscription succeeded. With that the Read call no longer hangs forver in case there is a communication issue with the device. Specifically, this integrates changes from the following PRs - project-chip/connectedhomeip#34370 - project-chip/connectedhomeip#34372
j-ororke
pushed a commit
to j-ororke/connectedhomeip
that referenced
this pull request
Jul 31, 2024
…hip#34370) The subscription logic waits for the first successful subscription before the Read() call is being returned (the future is awaited which is only released on handleSubscriptionEstablished). If the first subscription attempt fails (e.g. because the CASE session doesn't establish) the Read() never returns, not with an error but also not with a subscription transaction. And since the Python side has no access to the SubscriptionTransaction object at this point yet, there is also no way to stop this subscription attempt. With this change, we only resubscribe if the initial subscription was successful. This changes semantics slightly, but really allows the caller to decide if it wants to continue try to establish the subscription.
agners
added a commit
to home-assistant-libs/python-matter-server
that referenced
this pull request
Sep 3, 2024
With the update of the Matter SDK wheels to 2024.7.1 (#809) the SDK no longer auto-resubscribes when the read of the subscription fails (see project-chip/connectedhomeip#34370). The Server had a comment that it relies on mDNS rediscovery to re-establish the subscription. But this is not always the case. This change adds a retry mechanism to the subscription setup for a fixed period of time after seeing it via mDNS. Additionally, it avoids recreating the subscription if the liveness timeout did not occur yet. This puts more trust into the SDK handling the subscriptions.
agners
added a commit
to home-assistant-libs/python-matter-server
that referenced
this pull request
Sep 3, 2024
With the update of the Matter SDK wheels to 2024.7.1 (#809) the SDK no longer auto-resubscribes when the read of the subscription fails (see project-chip/connectedhomeip#34370). The Server had a comment that it relies on mDNS rediscovery to re-establish the subscription. But this is not always the case. This change adds a retry mechanism to the subscription setup for a fixed period of time after seeing it via mDNS. Additionally, it avoids recreating the subscription if the liveness timeout did not occur yet. This puts more trust into the SDK handling the subscriptions.
agners
added a commit
to home-assistant-libs/python-matter-server
that referenced
this pull request
Sep 3, 2024
With the update of the Matter SDK wheels to 2024.7.1 (#809) the SDK no longer auto-resubscribes when the read of the subscription fails (see project-chip/connectedhomeip#34370). The Server had a comment that it relies on mDNS rediscovery to re-establish the subscription. But this is not always the case. This change adds a retry mechanism to the subscription setup for a fixed period of time after seeing it via mDNS. Additionally, it avoids recreating the subscription if the liveness timeout did not occur yet. This puts more trust into the SDK handling the subscriptions.
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.
The subscription logic waits for the first successful subscription before the Read() call is being returned (the future is awaited which is only released on handleSubscriptionEstablished). If the first subscription attempt fails (e.g. because the CASE session doesn't establish) the Read() never returns, not with an error but also not with a subscription transaction. And since the Python side has no access to the SubscriptionTransaction object at this point yet, there is also no way to stop this subscription attempt.
With this change, we only resubscribe if the initial subscription was successful. This changes semantics slightly, but really allows the caller to decide if it wants to continue try to establish the subscription.