Fix memory/disk usage leak when creating many ScriptingContaiers by jpinsonault · Pull Request #4747 · jruby/jruby

jpinsonault · 2017-08-22T22:06:17Z

This PR is to resolve #3928, File descriptors from stdlib jars are leaked when ScriptingContainers are terminated

It's very much a work in progress, and I was hoping to get some feedback on

If this is close to a reasonable approach
What we would need to do to move this closer to a good solution
What kind of testing/validation you'd like to see to be comfortable with this change
Is cleaning up these files even possible/desirable when the scripting container isn't using a single threaded scope. i.e. threadsafe/concurrent/singleton. They seem to share references and I'm not sure how or if their use cases should be handled.

This biggest problem I currently see with it is that I was unable to find a way to cleanup the jars and references without making the JarResource class public. Since I'm not very experienced with this codebase, I'm especially interested in your opinions on how to clean up the jars in a cleaner way.

The basic strategy here is:

In JRubyClassLoader: When jars are extracted to a temporary folder, keep track of their paths
When a single threaded scripting container is terminated, its SingleThreadLocalContextProvider based context provider goes through each of the temp jars, and cleans up the jarCache references and deletes the jars from disk

In the case of a single threaded container, this should be safe. My understanding is that each single threaded container has its own class loader and context provider. Additionally, each time a container causes the class loader to extract a stdlib jar into a temp directory, it creates a uniquely named file for that class loader. So, removing these references will not affect any other scripting containers.

Since our use case in #3928 primarily involves using the single threaded local context scope, I've focused on that. But if this work should involve improvements to the other context providers we'd be happy to implement that as well.

So far I've been testing this manually by just creating scripting containers in a loop, giving them a script that requires a few stdlib jars, and verifying that the jars are created and destroyed on disk as expected. I've also been using yourkit to verify that the jar cache doesn't leak references

kares · 2017-08-23T05:56:09Z

core/src/main/java/org/jruby/util/JRubyClassLoader.java

        return "";
    }

+    public List<URL> getTempUrls(){


would be great to have the naming a bit consistent with existing API e.g. addURL(URL)
... getCachedURLs() or maybe indicate that there's temp file caching (that those URLs are actually local files)
in which case we should rather return paths than URLs, makes sense (getTempPaths or getCachedPaths)?

Yeah, that makes sense. What about cachedJarPaths and getCachedJarPaths? Or is that too verbose?

looks much better than the current naming ... go for whatever you feel is appropriate.
getter sounds fine to me esp. if its simply returning a field

Since these are always paths to files, cache the paths instead of URLs Also rename tempUrls to cachedJarPaths

jpinsonault · 2017-08-28T23:39:43Z

I pushed a commit changing the variable name to cachedJarPaths, and changed it to hold String paths instead of URLs

kares · 2017-08-29T05:25:35Z

looking good, is it still a WiP?
did you consider having a try {} catch around the jarFile.delete, to silence IOExceptions, would it make sense to not halt the termination in case of not being able to delete (e.g. file deleted previously) ?

kares · 2017-08-29T07:15:10Z

core/src/main/java/org/jruby/util/JRubyClassLoader.java

                in.close();
                url = f.toURI().toURL();
+
+                cachedJarPaths.add(url.getPath());


not sure how much its relevant but URL had issues - hopefully not for file: protocol but it might be worth a check
... why org.jruby.util.URLUtil.getPath exists and whether it should be used here

I looked into the issue, and from what I was able to understand, people recommend using Paths/Path objects if you're on java 7 or higher. Does jruby need to support java 6?

I also saw recommendations to use url.toURI().getSchemeSpecificPart() to get the String path, which is what URLUtil.getPath does, so it might be easier, and safe, to just use that utility function here

OK, feel free to update to using URLUtil than. also JRuby 9 only supported Java >= 7

jpinsonault · 2017-08-29T20:25:45Z

If it's looking good to you then I guess it's not a WIP.

According to https://docs.oracle.com/javase/7/docs/api/java/io/File.html#delete(), File.delete doesn't throw an IOException, and just returns false if the file couldn't be deleted. Which seems like fine behavior to me. I tried putting it in a try catch with an IOException and the compiler also complains that it will never throw that exception

kares · 2017-09-02T08:50:45Z

PR looking good, but I do not understand why its only tied to SingleThreadLocalContextProvider,
you just do not care about the other context-providers cause that is the one you're using?
but if this to be shipped it could be done for all, or any reason no to have the same cleanup there?

headius · 2017-10-31T19:56:58Z

@jpinsonault @kares This is probably something that would be good to put in 9.1.14, right? Can we land it yet?

headius · 2017-10-31T19:57:06Z

cc @enebo

enebo · 2017-10-31T20:50:03Z

@headius @jpinsonault @kares if this is good on a single provider and solid I say it is better than nothing. Expecting all providers to be done would be great but this will make .14 better... SHIP IT! :)

@jpinsonault If this can be done in the other providers are you up for adding it to those too?

headius · 2017-10-31T21:51:00Z

I will merge this and extend it to the other providers.

headius · 2017-10-31T22:07:47Z

I believe it makes most sense to move this into JRubyClassLoader's close/teardown logic, which is already called by ScriptingContainer.terminate. I'll do that and get everything merged.

Based on #4747 fix for #3928.

Fix memory/disk usage leak when creating many ScriptingContaiers

c12b347

kares reviewed Aug 23, 2017

View reviewed changes

Cache jar paths instead of URLs

1bbcc43

Since these are always paths to files, cache the paths instead of URLs Also rename tempUrls to cachedJarPaths

kares reviewed Aug 29, 2017

View reviewed changes

jpinsonault changed the title ~~[Work in progress] Fix memory/disk usage leak when creating many ScriptingContaiers~~ Fix memory/disk usage leak when creating many ScriptingContaiers Aug 29, 2017

Use URLUtil.getPath instead of URL.getPath

d850eff

kares added this to the JRuby 9.2.0.0 milestone Sep 2, 2017

kares added the embed label Sep 2, 2017

headius added a commit that referenced this pull request Oct 31, 2017

Move temp jar cleanup into JRubyClassLoader's teardown logic.

6493b3a

Based on #4747 fix for #3928.

headius closed this Oct 31, 2017

headius modified the milestones: JRuby 9.2.0.0, JRuby 9.1.14.0 Oct 31, 2017

headius mentioned this pull request Oct 31, 2017

File descriptors from stdlib jars are leaked when ScriptingContainers are terminated #3928

Closed

Uh oh!

Conversation

jpinsonault commented Aug 22, 2017

Uh oh!

kares Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpinsonault Aug 23, 2017

Choose a reason for hiding this comment

Uh oh!

kares Aug 26, 2017

Choose a reason for hiding this comment

Uh oh!

jpinsonault commented Aug 28, 2017

Uh oh!

kares commented Aug 29, 2017

Uh oh!

kares Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

jpinsonault Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

kares Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

jpinsonault Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

jpinsonault commented Aug 29, 2017

Uh oh!

kares commented Sep 2, 2017

Uh oh!

headius commented Oct 31, 2017

Uh oh!

headius commented Oct 31, 2017

Uh oh!

enebo commented Oct 31, 2017

Uh oh!

headius commented Oct 31, 2017

Uh oh!

headius commented Oct 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kares Aug 23, 2017 •

edited

Loading