Skip to content
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

native memory leak when reading files from inside a .jar #1888

Closed
KevinCorcoran opened this issue Aug 5, 2014 · 8 comments
Closed

native memory leak when reading files from inside a .jar #1888

KevinCorcoran opened this issue Aug 5, 2014 · 8 comments
Labels
Milestone

Comments

@KevinCorcoran
Copy link

@KevinCorcoran KevinCorcoran commented Aug 5, 2014

File.file? seems to leak native (non-JVM) memory when it is passed a path of a file inside a .jar. This occurs on 1.7.13 but not on 1.7.12

Something along the lines of ...

loop do
  File.file?("file:/home/bbrowning/.m2/repository/org/jruby/jruby-complete/1.7.14.dev-SNAPSHOT/jruby-complete-1.7.14.dev-SNAPSHOT.jar!/META-INF/jruby.home/lib/ruby/gems/shared/specifications/default/rdoc-4.0.1.gemspec")
end

... and java -jar ~/.m2/repository/org/jruby/jruby-complete/1.7.13/jruby-complete-1.7.13.jar leak.rb should demonstrate this issue.

Here's a bit of context from #jruby today, for posterity's sake:

bbrowning
JarResource.java is a likely culprit

bbrowning
so it looks like perhaps some inputstreams are not ever being closed

bbrowning
and the JVM uses some native code here for every inputstream that's opened

bbrowning
my gut is https://github.com/jruby/jruby/blob/jruby-1_7/core/src/main/java/org/jruby/util/JarResource.java#L68 is not being closed ever

bbrowning
something has to close those InputStreams that get created in JarResource

bbrowning
the whole FileResource interface probably needs a close() method on it

bbrowning
a no-op for some impls, but for things like JarFileResource it would clean itself up

bbrowning
dfr|work: headius: in case you guys come around, we need to figure out how to clean up JarFileResources - their entryStream is a ZipInputStream that must be closed to free native memory when it's no longer needed

Many thanks to @bbrowning for being awesome and getting to the bottom of this for us!

@headius
Copy link
Member

@headius headius commented Aug 6, 2014

A dirty fix would be to make those resource objects have finalizers, so they will eventually GC and clean up. That isn't going to be reliable, of course, so the better option would be what @bbrowning suggested: FileResource interface should have a close() method implemented by all impls, and we should be calling it when we're done with the resources we've requested.

@headius headius added the core label Aug 6, 2014
@headius headius added this to the JRuby 1.7.14 milestone Aug 6, 2014
@headius
Copy link
Member

@headius headius commented Aug 7, 2014

Here's the dumb fix I mentioned, for posterity:

diff --git a/core/src/main/java/org/jruby/util/JarFileResource.java b/core/src/main/java/org/jruby/util/JarFileResource.java
index ab09869..de204be 100644
--- a/core/src/main/java/org/jruby/util/JarFileResource.java
+++ b/core/src/main/java/org/jruby/util/JarFileResource.java
@@ -75,4 +75,8 @@ class JarFileResource extends JarResource {
   public ChannelDescriptor openDescriptor(ModeFlags flags, POSIX posix, int perm) throws ResourceException {
     return new ChannelDescriptor(openChannel(flags, posix, perm), flags);
   }
+
+  protected void finalize() throws Throwable {
+      entryStream.close();
+  }
 }

@ratnikov
Copy link
Contributor

@ratnikov ratnikov commented Aug 11, 2014

+1 for headius's fix.

I'm hesitant about FileResource#close since I'm not really sure about how long the FileResource are meant to live: sometimes it's just one call, sometimes (e.g. RubyFile or RubyDir) it's for duration of the ruby object, so making sure things are closed properly seems tricky.

What's the drawback of the finalize option?

@ratnikov
Copy link
Contributor

@ratnikov ratnikov commented Aug 12, 2014

From IRC disucssion:

  1. finalize is probably not good, since native memory != heap, so we may be doing well on heap so no GC takes place, and running out of native memory.

  2. Seems like currently best solution is to open up the stream only when it is requested by getInputStream and make sure that requesters close it.

@ratnikov
Copy link
Contributor

@ratnikov ratnikov commented Aug 13, 2014

Hmm, seems like the getInputStream isn't the culprit, since file? doesn't exercise that code path (I jammed a System.out.println in JarFileResource#getInputStream and got nothing in stdout).

Will keep poking....

@enebo
Copy link
Member

@enebo enebo commented Aug 13, 2014

So does your PR fix the leak? I can see how it will not open as many streams but I have not traced through how file? can create a library like this.

@bbrowning
Copy link
Contributor

@bbrowning bbrowning commented Aug 13, 2014

Since the originally reported leak script was run on my machine, I've built the 1.7 branch with this PR and can confirm the leak is indeed fixed, at least in the File.file? case shown in this issue.

This should hopefully fix the native memory leak reported in #1791 as well.

@enebo
Copy link
Member

@enebo enebo commented Aug 13, 2014

Fixed by PR #1898

@enebo enebo closed this Aug 13, 2014
@enebo enebo removed this from the JRuby 1.7.14 milestone Aug 27, 2014
@enebo enebo added this to the JRuby 1.7.15 milestone Aug 27, 2014
@enebo enebo removed this from the JRuby 1.7.15 milestone Aug 27, 2014
@enebo enebo added this to the JRuby 1.7.14 milestone Aug 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants