-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Preventing self from deallocating during performPatchUpdates. #137
Preventing self from deallocating during performPatchUpdates. #137
Conversation
Codecov Report
@@ Coverage Diff @@
## master #137 +/- ##
==========================================
+ Coverage 93.25% 93.25% +<.01%
==========================================
Files 38 38
Lines 3097 3099 +2
==========================================
+ Hits 2888 2890 +2
Misses 209 209
Continue to review full report at Codecov.
|
@@ -501,7 +509,10 @@ extension BrickCollectionView: AsynchronousResizableDelegate { | |||
weak var weakLayout = self.layout | |||
self.performBatchUpdates({ | |||
weakLayout?.updateHeight(indexPath, newHeight: newHeight) | |||
}, completion: completion) | |||
}, completion: { completed in | |||
let _ = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be the one that is the problem? All the other ones capture self
in the performBatchUpdates
-block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am note sure. I was seeing the issue in invalidateVisibility. I can test it out real quick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was still reproducing the crash if I made this performBatchUpdates
capture self
. But it does seem this is where the cause of the crash is as the crash no longer happens if I add let _ = self
into the completion handler.
I kind of think that we should keep let _ = self
in all completion handlers to avoid the issue appearing anywhere else. What do you think Ruben?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isolated the change to just this line of code. If it's all good, accept the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be optimized out when the optimizer runs
c189a01
to
6149a9d
Compare
6149a9d
to
361fe54
Compare
This should fix the occasional error
UICollectionView was deallocated while an update was in flight
. I tried writing unit tests around this but couldn't force the crash to trigger.