[Matter.framework] When the device is entering suspended state and at… #35973
+4
−4
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
… the same time _ensureSubscriptionForExistingDelegates is called with an error from getting a session it crashes with _os_unfair_lock_recursive_abort since it is called on the same thread and the error handling code also try to get a lock
|
Review changes with SemanticDiff. |
woody-apple
approved these changes
Oct 8, 2024
|
PR #35973: Size comparison from 0a2e58d to 491bcb7 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
anush-apple
approved these changes
Oct 8, 2024
jtung-apple
approved these changes
Oct 8, 2024
| if (self.suspended) { | ||
| MTR_LOG_ERROR("%@ suspended: can't get session for node %016llX-%016llx (%llu)", self, self.compressedFabricID.unsignedLongLongValue, nodeID, nodeID); | ||
| // TODO: Can we do a better error here? | ||
| completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE], nil); | ||
| dispatch_async(_chipWorkQueue, ^{ |
There was a problem hiding this comment.
So the thing is... asyncGetCommissionerOnMatterQueue can also sync-call completion, right?
There was a problem hiding this comment.
We need to either fix whoever is calling directlyGetSessionForNode to handle the fact that they might get a sync-call in whatever state they are in to the error handler, or change the API documentation for getSessionForNode and the implementations of both that and directlyGetSessionForNode to guarantee async callbacks.
We used to not be able to use the Matter queue there, because without a running controller we potentially had no running queue. But now that we never shut the queue down (right?) maybe we can consistently deliver the errors on that queue....
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 same time _ensureSubscriptionForExistingDelegates is called with an error from getting a session it crashes with _os_unfair_lock_recursive_abort since it is called on the same thread and the error handling code also try to get a lock
Problem
While attempting to reproduce a specific memory leak with Darwin-framework-tool, I encountered a crash caused by a recursive lock issue.
In essence, this occurs because, when the tool is suspended, directlyGetSessionForNode returns synchronously. However, the code that triggered this behavior already held the lock, and the code managing subscription errors also attempts to acquire the same lock.
When this happens, the thread states are as follows: