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.

Sign up for GitHub

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

Jump to bottom

Fix memory/disk usage leak when creating many ScriptingContaiers #4747

Closed
wants to merge 3 commits into from
Closed

Fix memory/disk usage leak when creating many ScriptingContaiers #4747

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c12b347
Fix memory/disk usage leak when creating many ScriptingContaiers
jpinsonault Aug 9, 2017
1bbcc43
Cache jar paths instead of URLs
jpinsonault Aug 28, 2017
d850eff
Use URLUtil.getPath instead of URL.getPath
jpinsonault Aug 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion core/src/main/java/org/jruby/embed/internal/SingleThreadLocalContextProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,14 @@
*/
package org.jruby.embed.internal;

import java.util.Map;
import org.jruby.Ruby;
import org.jruby.embed.LocalVariableBehavior;
import org.jruby.util.JarResource;

import java.io.File;
import java.net.URL;
import java.util.List;
import java.util.Map;

/**
*
Expand All @@ -56,6 +61,19 @@ private LocalContext getLocalContext() {
return instance;
}

private void terminateJarIndexCacheEntries() {
List<String> cachedJarPaths = getRuntime().getJRubyClassLoader().getCachedJarPaths();

for (String jarPath : cachedJarPaths){
// Remove reference from jar cache
JarResource.removeJarResource(jarPath);

// Delete temp jar on disk
File jarFile = new File(jarPath);
jarFile.delete();
}
}

@Override
public Ruby getRuntime() {
return getLocalContext().getRuntime();
Expand All @@ -79,6 +97,8 @@ public boolean isRuntimeInitialized() {
@Override
public void terminate() {
getLocalContext().remove();

terminateJarIndexCacheEntries();
}

}
10 changes: 10 additions & 0 deletions core/src/main/java/org/jruby/util/JRubyClassLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
import java.io.OutputStream;
import java.lang.management.ManagementFactory;
import java.net.URL;
import java.util.ArrayList;
import java.util.List;

import org.jruby.util.log.Logger;
import org.jruby.util.log.LoggerFactory;
Expand All @@ -62,6 +64,8 @@ public class JRubyClassLoader extends ClassDefiningJRubyClassLoader {

private static volatile File tempDir;

private List<String> cachedJarPaths = new ArrayList<String>();

public JRubyClassLoader(ClassLoader parent) {
super(parent);
}
Expand All @@ -86,6 +90,8 @@ public void addURL(URL url) {
out.close();
in.close();
url = f.toURI().toURL();

cachedJarPaths.add(url.getPath());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

}
catch (IOException e) {
throw new RuntimeException("BUG: we can not copy embedded jar to temp directory", e);
Expand Down Expand Up @@ -160,6 +166,10 @@ private static String systemTmpDir() {
return "";
}

public List<String> getCachedJarPaths(){
return cachedJarPaths;
}

/**
* @deprecated use {@link #close()} instead
*/
Expand Down
16 changes: 16 additions & 0 deletions core/src/main/java/org/jruby/util/JarCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,4 +157,20 @@ public JarIndex getIndex(String jarPath) {
return index;
}
}

public boolean remove(String jarPath){
String cacheKey = jarPath;

synchronized (indexCache) {
JarIndex index = indexCache.get(cacheKey);
if (index == null){
return false;
}
else{
index.release();
indexCache.remove(cacheKey);
return true;
}
}
}
}
6 changes: 5 additions & 1 deletion core/src/main/java/org/jruby/util/JarResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;

abstract class JarResource extends AbstractFileResource {
public abstract class JarResource extends AbstractFileResource {
private static Pattern PREFIX_MATCH = Pattern.compile("^(?:jar:)?(?:file:)?(.*)$");

private static final JarCache jarCache = new JarCache();
Expand Down Expand Up @@ -79,6 +79,10 @@ private static JarResource createJarResource(String jarPath, String entryPath, b
return null;
}

public static boolean removeJarResource(String jarPath){
return jarCache.remove(jarPath);
}

private final String jarPrefix;
private final JarFileStat fileStat;

Expand Down