Fixed memory leaks within real time client #1100
Conversation
| // We don't pass the block directly; instead, we put it in a property, and | ||
| // read it back from the property once the timer fires. This gives us the | ||
| // chance to set the property to nil when cancelling the timer, thus | ||
| // releasing our retain on the block early. dispatch_block_cancel doesn't do | ||
| // this, it retains the block even if you cancel the dispatch until the | ||
| // dispatch time passes. (How this is a good idea escapes me.) | ||
| // | ||
| // From Apple's documentation [1]: | ||
| // | ||
| // > Release of any resources associated with the block object is delayed | ||
| // > until execution of the block object is next attempted (or any execution | ||
| // > already in progress completes). | ||
| // | ||
| // https://developer.apple.com/documentation/dispatch/1431058-dispatch_block_cancel |
There was a problem hiding this comment.
I wrote this to preserve the existing behavior / intention, but I'm not sure this is actually needed.
When using blocks with GCD, they're copied (not retained). Variables referenced outside of the scope of the block have different semantics, but typically the thing we care about is how it affects an objects life cycle and whether or not we can mutate its value from within the block. That said, the resources that "aren't released" are either primitive values (bools, ints, etc) or a pointer, all of which have very little overhead.
It seems to me that capturing weak variables is likely the solution to whatever problem this was trying to solve, but I could be wrong!
|
@QuintinWillison Any updates on possibly getting some of @mikepulaski 's PRs merged? They fix some mem leaks with your client that others would benefit from. Looks like the Swift compiler is adding a feature to properly detect bridged Objc -> Swift nullable issues and we noticed that it might affect your codebase. The ARTBaseMessage.connectionId can become nil if the connection enters a failed state when there are messages still in the queue. Simple fix might be to make some objects nullable (that are become nil) so that Swift code can handle it properly by returning early or whatever. Let us know if you want more details! |
|
Hi @SpencerCDixon and @mikepulaski - sorry for the delay on this. My time has been soaked up by another project recently and nobody else has picked this up either. I'm going to try to set aside some time in the next 48 hours or so to take a look. Thanks for your patience. 😁 |
|
Nice one! Thank you. Having a semaphore coordinating the scheduled block seems reasonable to me. This will also solve the issue with BTW, I ran the whole test suite (including the Soak tests) a couple of times and I had no issues. |
|
@mikepulaski Sorry for the delay. Is there any chance you could rebase atop our updated |
|
@QuintinWillison It looks like the conflict is just related to the Xcode project (because I added a new test class to verify the fix). It should be pretty easy to fix the conflict on your side, and was hoping this would have been fixed much quicker as we've had to ship a forked version of your client for several releases now. It's not very straight forward for me to rebase, FYI, as we've been using our forked main branch as the version we're deploying, which has the various PRs I've opened already merged. |
|
ok, thanks @mikepulaski ... I'll see what we can do on this from our end. We're literally sprint planning right now so I'll surface this to the top of the queue. Sorry for the delays. We're doing our best but resources have been limited. |
|
No problem, I get it! Thanks for your help, @QuintinWillison. |
|
@QuintinWillison Can we close this PR when we have this one #1130 ? |
|
@lukasz-szyszkowski that would make sense, yes... but I see, currently at least, we're awaiting green CI before we can land #1130. |
|
I'm closing this PR, changes are HERE |
In short,
ARTScheduledBlockHandlecreates a reference cycle to itself which is never broken if the handler is not cancelled.Real time event handlers also create strong references to themselves and this cycle was not broken when the timers were stopped.
I added a
XCTestto assert the behavior, which could probably be converted to use your testing stack pretty easily -- I just wasn't familiar enough to do to it justice (and it seems like it had its own effect on reference counting) 🙃This fixes issue #1099.