Fix memory leaks #45
Merged
Fix memory leaks #45
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
If a finalizer is passed a proc, and that proc has a reference to the object that it is supposed to finalize, your objects will never get garbage collected. http://www.mikeperham.com/2010/02/24/the-trouble-with-ruby-finalizers/ talks through some of this, but the high level is you end up with a reference chain of: collection_of_finalizers -> proc -> object That proc can't be collected because it's being held in the finalizer set, and that proc references the object, so the object can't be collected. affects typhoeus#30 fixes typhoeus#34 fixes typhoeus/typhoeus#229
I've been manually running this as:
RUBY_HEAP_SLOTS_GROWTH_FACTOR=1 RUBY_HEAP_MIN_SLOTS=1 RUBY_HEAP_SLOTS_INCREMENT=10 RUBY_HEAP_SLOTS_GROWTH_FACTOR=.1 ITERATIONS=75000 rspec -I lib -I profile profile/memory_leaks.rb
The ruby GC settings are that way to try and make ruby increase memory really
slowly, so that we can get more accurate info on what's going on.
ITERATIONS can be whatever, but I've been running it with a really large number
so that I can be more comfortable with the results.
Make sure to free the first form entry with curl_formfree http://curl.haxx.se/libcurl/c/curl_formadd.html fixes typhoeus#43
I think this all could also be simply replaced with CGI.escape, but we can change that later. fixes typhoeus#44
|
All the tests pass locally, but master fails. I'm assuming the travis build for this pull request is running against master+my changes? |
hanshasselberg
added a commit
that referenced
this pull request
Apr 9, 2013
Fix memory leaks Conflicts: lib/ethon/easy/form.rb
|
Awesome work! |
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.
This pull request fixes a bunch of issues. The primary one is it fixes the large memory leaks in Ethon (which propagate up to Typhoeus). Specifically, we at Groupon use Typhoeus for some internal HTTP calls, and in upgrading typhoeus (0.2.3 to 0.6.2) we noticed some very significant memory leaks.
After doing an analysis of the code (and reading up on FFI/finalizers), I found that none of the finalizers in Ethon were working properly. Because of the way they worked, they all were causing memory leaks. The version in this pull request redoes those, and leverages FFI::AutoPointer to do the cleanup (that is what AutoPointer is for). Switching to AutoPointer removes the need for this gem to use Objectspace, which in theory will make it behave better in JRuby.
This also fixes some issues where memory that was getting created by curl wasn't ever getting cleaned up. The largest issue was in the curl_escape call (Curl.escape). That was initializing C memory, but never actually recollecting it.
Finally, I've also created a test which looks for memory leaks. I don't know if it's something that should be added to the automated build (it still gets some false positives), but it's very useful for checking for memory leaks. From the commit message:
The commits themselves reference the issues that have been fixed/touched as part of this, but I'll copy them here for longevity:
fixes #43
fixes #44
affects #30
fixes #34
fixes typhoeus/typhoeus#229
This branch is based off of v0.5.10 since master has failing tests.
I personally would really appreciate a quick merge on this, and also a patch release. Preferably v0.5.11 would include just these changes, and there'd be a corresponding Typhoeus release.