Fix continuation memory leak in Ares.query #31
Conversation
dieb
force-pushed
the
memory-leak-continuation
branch
from
eb37167 to
547fbec
Compare
|
@swift-server-bot add to allowlist |
| let pointer = handlerPointer.assumingMemoryBound(to: QueryReplyHandler.self) | ||
| let handler = pointer.pointee | ||
| defer { | ||
| pointer.deinitialize(count: 1) |
There was a problem hiding this comment.
Cool! I was making similar changes as well but was missing this line of code.
| @@ -276,11 +280,6 @@ extension Ares { | |||
| } | |||
There was a problem hiding this comment.
Can we also change struct QueryReplyHandler to class QueryReplyHandler please? That combined with pointer.deinitialize(count: 1) I see deinit getting called.
There was a problem hiding this comment.
Absolutely, just pushed a new commit to reflect those changes.
I'm interested in understanding the rationale behind class vs struct here. Could you please share more about your thought process? Swift noob eager to learn here 😄
My best guess is that class deinit can free up its members, though my tests indicate no leaks with struct.
There was a problem hiding this comment.
I was told that the address of a struct is not stable (we were thinking deallocate might not be working on the right address because of this), plus we are doing all these pointer manipulations already so class probably makes more sense.
|
@swift-server-bot add to allowlist |
1 similar comment
|
@swift-server-bot add to allowlist |
|
@swift-server-bot test this please |
|
Something is wrong with the webhook so CI is not getting triggered. Will ask someone for help looking into it in the morning. |
|
@swift-server-bot test this please |
Absolutely, thanks for the oppo. Should be a quick thing. |
dieb
force-pushed
the
memory-leak-continuation
branch
from
b0e8c14 to
6445386
Compare
|
@yim-lee I made the change for DNSSD but I was unable to test locally, test_concurrency hangs. I'm also getting errors on CAresDNSResolverTests |
Move defer deallocation block to after initialization. Use class instead of struct for DNSSD.QueryReplyHandler.
@dieb Does this happen even before these changes?
Are these errors due to no results? |
|
Same errors are happening in main and 1f5d6f4 (before #30), so I'm guessing it's something to do with my local environment. Running Edit: errors are connection refused and test_concurrency hanging. Test log |
| // The handler might be called multiple times so don't deallocate inside `callback` | ||
| defer { | ||
| let pointer = handlerPointer.assumingMemoryBound(to: QueryReplyHandler.self) | ||
| pointer.deinitialize(count: 1) |
There was a problem hiding this comment.
Without this line of code, we have these leaks:
The above is modifying test_queryAAAA to run self.resolver.queryAAAA 100K times.
With this line there were no leaks.
There was a problem hiding this comment.
Thanks @dieb.
Tests pass locally on macOS for me and I ran Instruments on modified test_queryAAAA (basically run the query many times in a loop) with and without this changeset and could see that leaks are fixed, so I think these changes are good.
@ktoso I would appreciate if you could review this as well. Thanks in advance. 🙏
Motivation
#29
Modifications
Frankly not too sure exactly why this fixes the leak (Swift noob here), but it may be that
pointer.deallocate()only frees the pointer, and not the underlying initialized.Result
QueryReplyHandler gets deallocated properly and so does the continuation that was leaking.
Leaks from this are gone when running A/AAAA queries.
Test Plan
Running Xcode leaks instrument in my app, some code examples in #29 .