-
Notifications
You must be signed in to change notification settings - Fork 75
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
Fix MacOS memory leaks. #97
Conversation
Test done. |
Note that I'm not the maintainer of this repository anymore. I could review PRs if there's a need for it, but I'd prefer not to. |
Sorry, I don't know who else is maintaining this repository. Has it stopped maintaining it? |
From the commit history it's visible that it's still being maintained. @complexspaces let me know if you need help reviewing this PR. |
Thanks for the ping. This repository is still actively maintained, I've just been very busy/stretched recently. I'll try to have a look over this by the weekend. |
@complexspaces any updates on this? |
Hi, I apologize again. I've been trying to re-familiarize myself with this code and the macOS pasteboard APIs. I am still working on it. |
guys can we merge this pull request🙏?the memory leaks have affected the experience of my users. |
We can try and move this one forward again, yes. I have been very hesitant to finalize a review or merge it because of my unfamiliarity with the macOS clipboard APIs (in comparison with the other platforms), which results in me being unsure of this approach. |
Hi everyone, thank you for the patience. I've opened an alternative PR to resolve the memory leaks in #105. For those impacted by this in their applications, could I ask you to give it a try? cc @zhouyangtingwen @mchaliadzinau From what I can tell, the get methods were not leaking and it was only the setters causing problems. For example, running the
The same applies to getting text:
Therefore, after more review, I am not sure what this PR was specifically trying to fix because I was unable to reproduce the seemingly-problematic/leaking |
Unfortunately, I pulled your branch, but there is still a memory leak after local testing You can look at my code implementation: |
@complexspaces |
Thank you for the update, @ChurchTao. Its really strange that I wasn't able to reproduce this earlier but I will definitely take another look. I appreciate the source link as well. Is there a specific image format you have on your clipboard when copying or does it happen with any kind of image? |
The use case I tested was screenshots generated by other apps, but I guess this has nothing to do with the image format. |
When I use @ChurchTao ‘s https://github.com/ChurchTao/arboard.git, the memory leak problem is resolved |
Thank you all for the confirmations. I will spend time this week attempting to locate a root cause. |
OK,thank you |
Hey again everyone. I've made another set of updates in #106. I was able to reproduce the memory usage mentioned here by running the various Can I ask everyone to give this new branch a try and let me know if it fixes the issues in your applications? |
thank you very much, let me try😁 |
@complexspaces Unfortunately, it still doesn't work. I suggest you to try my branch. My code is based on this answer: SSheldon/rust-objc#117 (comment) |
@ChurchTao Sorry to hear it still hasn't worked, but thank you for the update. FWIW, while I have read that answer, I have gotten rid of the problematic TIFF methods in my branch so that the system framework codepath which holds onto image data for a while doesn't get called anymore. I was hopeful that would help to resolve the leak, especially since it seemed to in a synthetic test. Perhaps it is the case that all of the Objective-C clipboard methods are returning autoreleased objects that need a pool? |
As far as my personal use is concerned, there will be no problem with getText, but if there is no autoreleasepool when getting pictures, it will definitely leak. To be honest, I am not clear about objc, maybe you can find some inspiration in objc's Issue. |
Hey again, this should hopefully be the last delay. @ChurchTao after reviewing the new comments from the May I ask for you to give my branch one last try? I checked out, and built, your clipboard manager and gave it a try, in which I did not see forever-unbound memory usage when copying larger, 4K, images to the clipboard. I also added you as a co-author in the commit for proper accreditation. If my branch works, I will merge it (obsoleting this changeset) and release a new version on crates.io shortly after. |
Good job, I tried your new version and it worked! @complexspaces |
The memory leak fixes are now published as part of 3.2.1 on crates.io. I'm going to go ahead and close this like discussed above, but thank you again everyone for the help, testing, and feedback. |
See SSheldon/rust-objc#117
and #24