[Python] Avoid RuntimeException if APIs with future raise an error #34354
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,
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,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kiel-apple and
kkasperczyk-no
pullapprove
bot
requested review from
tecimovic,
tehampson,
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
|
PR #34354: Size comparison from 4ed9b7e to d38b1f3 Full report (4 builds for mbed, nrfconnect)
|
agners
force-pushed
the
fix-error-handling-for-apis-with-futures
branch
2 times, most recently
from
0b37346 to
e6e2312
Compare
|
PR #34354: Size comparison from df8c9da to e6e2312 Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
arkq
reviewed
Jul 17, 2024
arkq
approved these changes
Jul 17, 2024
Currently, when calling an API which uses a future causes an error
(e.g. CommissionWithCode with an invalid code), then the API call
already returns an error. In this case the call `raise_on_error()` on
the returned PyChipError object make sure that an exception is raised.
However, this also causes the `CallbackContext` context manager to
exit.
At this point the future is initialized but never completed, which
triggers the previously introduced sanity check in `CallbackContext`:
`RuntimeError("CallbackContext future not completed")`.
Remove the RuntimeError as existing the context manager early without
completing the future is a use case (when the call setting up the
callback raises an exception).
Instead, just cancel the future in the context manager if it hasn't
been complete yet, in case someone has a reference to it and expects
it to complete.
Also, since most API calls return PyChipError, this changes
`CallAsync()` to raise an exception by default instead of returning a
PyChipError object. If the PyChipError object is required or an API
returns something else, the CallAsyncWithResult() method can be used.
agners
force-pushed
the
fix-error-handling-for-apis-with-futures
branch
from
e6e2312 to
79f226b
Compare
|
PR #34354: Size comparison from a30dd12 to 79f226b Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
cecille
approved these changes
Jul 17, 2024
agners
added a commit
to home-assistant-libs/chip-wheels
that referenced
this pull request
Jul 17, 2024
By default the Python controller gets built with unit test configuration enabled. This adds extra functionality to inject errors etc. for testing. There is no known negative effect to have it enabled, but it is certainly better to disable it for a production setup. Furthermore this fixes error handling for APIs with callbacks and improves logging somewhat. Specifically, this integrates changes from the following PRs - project-chip/connectedhomeip#34346 - project-chip/connectedhomeip#34354 - project-chip/connectedhomeip#34368
j-ororke
pushed a commit
to j-ororke/connectedhomeip
that referenced
this pull request
Jul 31, 2024
…roject-chip#34354) Currently, when calling an API which uses a future causes an error (e.g. CommissionWithCode with an invalid code), then the API call already returns an error. In this case the call `raise_on_error()` on the returned PyChipError object make sure that an exception is raised. However, this also causes the `CallbackContext` context manager to exit. At this point the future is initialized but never completed, which triggers the previously introduced sanity check in `CallbackContext`: `RuntimeError("CallbackContext future not completed")`. Remove the RuntimeError as existing the context manager early without completing the future is a use case (when the call setting up the callback raises an exception). Instead, just cancel the future in the context manager if it hasn't been complete yet, in case someone has a reference to it and expects it to complete. Also, since most API calls return PyChipError, this changes `CallAsync()` to raise an exception by default instead of returning a PyChipError object. If the PyChipError object is required or an API returns something else, the CallAsyncWithResult() method can be used.
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.
Currently, when calling an API which uses a future causes an error (e.g. CommissionWithCode with an invalid code), then the API call already returns an error. In this case the call
raise_on_error()on the returned PyChipError object make sure that an exception is raised. However, this also causes theCallbackContextcontext manager to exit.At this point the future is initialized but never completed, which triggers the previously introduced sanity check in
CallbackContext:RuntimeError("CallbackContext future not completed").Remove the RuntimeError as existing the context manager early without completing the future is a use case (when the call setting up the callback raises an exception).
Instead, just cancel the future in the context manager if it hasn't been complete yet, in case someone has a reference to it and expects it to complete.
Also, since most API calls return PyChipError, this changes
CallAsync()to raise an exception by default instead of returning a PyChipError object. If the PyChipError object is required or an API returns something else, the CallAsyncWithResult() method can be used.