[Python] Fix stack race between Python & Matter thread #19724
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
pullapprove
bot
requested review from
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13,
hawk248,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kghost,
kpschoedel,
lazarkov,
LuDuda,
mlepage-google,
msandstedt,
mspang and
rgoliver
pullapprove
bot
requested review from
robszewczyk,
sagar-apple,
saurabhst,
selissia,
tcarmelveilleux,
tecimovic,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21,
yufengwangca and
yunhanw-google
mrjerryjohns
force-pushed
the
python/fix-stack-race
branch
from
5eaccec to
0c2fc95
Compare
andy31415
approved these changes
Jun 17, 2022
msandstedt
approved these changes
Jun 17, 2022
turon
approved these changes
Jun 17, 2022
|
PR #19724: Size comparison from ab7fb48 to 0c2fc95 Increases (2 builds for cc13x2_26x2, telink)
Decreases (2 builds for cc13x2_26x2, esp32)
Full report (41 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
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.
Problem
When attempting to clean-up some of the subscription tests to call
Shutdown()on theSubscriptionTransactionobject before moving onto another test (otherwise, it causes occasional Cirque failure issues with attribute changes being posted to an event loop context that is now closed), ran into an issue where we were aborting the underlyingReadClientfrom a different threading context than Matter immediately upon receiving an attribute value.This resulted in the underlying ReadClient being destroyed before the stack could finish completing the processing of the report, resulting in errors sending out a StatusResponse due to not having an exchange :S
Fix
When calling C++ methods, ensure to dispatch those on the Matter execution context.
I've also cleaned up
ChipDeviceCtrlto not dispatch Python function calls on the Matter threading context - instead, only do so right at the edge where we call into the C++ layer. This avoids issues with test code that assume (correctly), that most of our IM Python API surface is meant to be callable from any execution context. This is the general assumption in pretty much all of the Python API surface (unless explicitly called out).Also fixes a use-after-free bug where we were inadvertently freeing up the heap-allocated dataVersionList prematurely.