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

Don't cache whole scripts in development mode, allows sub resources to hot reload again #279

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Don't cache whole scripts in development mode, allows sub resources to hot reload again #279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
71 changes: 1 addition & 70 deletions core/src/main/java/org/kohsuke/stapler/AbstractTearOff.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,9 @@
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

import java.io.File;
import java.lang.ref.Reference;
import java.lang.ref.SoftReference;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

Expand All @@ -47,21 +41,9 @@
*/
public abstract class AbstractTearOff<CLT,S,E extends Exception> extends CachingScriptLoader<S,E> {

private static final Logger LOGGER = Logger.getLogger(AbstractTearOff.class.getName());

protected final MetaClass owner;
protected final CLT classLoader;

private static final class ExpirableCacheHit<S> {
private final long timestamp;
private final Reference<S> script;
ExpirableCacheHit(long timestamp, S script) {
this.timestamp = timestamp;
this.script = new SoftReference<>(script);
}
}
private final Map<String, ExpirableCacheHit<S>> cachedScripts = new ConcurrentHashMap<>();

protected AbstractTearOff(MetaClass owner, Class<CLT> cltClass) {
this.owner = owner;
if(owner.classLoader!=null)
Expand Down Expand Up @@ -113,58 +95,7 @@ public S resolveScript(String name) throws E {
res = getResource(name.substring(0, dot) + ".default" + name.substring(dot));
}
if (res != null) {
if (MetaClass.NO_CACHE) {
File file = fileOf(res);
if (file == null) {
LOGGER.log(Level.FINE, "no file associated with {0}", res);
return parseScript(res);
} else {
long timestamp = file.lastModified();
if (timestamp == 0) {
LOGGER.log(Level.FINE, "no timestamp associated with {0}", file);
return parseScript(res);
} else {
ExpirableCacheHit<S> cached = cachedScripts.get(res.toString());
if (cached == null) {
S script;
if (LOGGER.isLoggable(Level.FINE)) {
long start = System.nanoTime();
try {
script = parseScript(res);
} finally {
LOGGER.log(Level.FINE, "cache miss; took {0}ms to parse {1}", new Object[] {(System.nanoTime() - start) / 1_000_000, res});
}
} else {
LOGGER.log(Level.FINE, "cache miss on {0}", res);
script = parseScript(res);
}
cachedScripts.put(res.toString(), new ExpirableCacheHit<>(timestamp, script));
return script;
} else {
S script;
if (timestamp == cached.timestamp) {
script = cached.script.get();
if (script == null) {
LOGGER.log(Level.FINE, "cache hit on {0} but value collected", res);
} else {
LOGGER.log(Level.FINE, "cache hit on {0}", res);
}
} else {
LOGGER.log(Level.FINE, "expired cache hit on {0}", res);
script = null;
}
if (script == null) {
script = parseScript(res);
cachedScripts.put(res.toString(), new ExpirableCacheHit<>(timestamp, script));
}
return script;
}
}
}
} else {
LOGGER.log(Level.FINE, "standard CachingScriptLoader logic applies to {0}", res);
return parseScript(res);
}
return parseScript(res);
}

return null;
Expand Down
80 changes: 72 additions & 8 deletions jelly/src/main/java/org/kohsuke/stapler/jelly/CustomTagLibrary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

package org.kohsuke.stapler.jelly;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import org.apache.commons.jelly.JellyContext;
import org.apache.commons.jelly.JellyException;
import org.apache.commons.jelly.Script;
Expand All @@ -33,10 +34,17 @@
import org.kohsuke.stapler.MetaClassLoader;
import org.xml.sax.Attributes;

import java.io.File;
import java.lang.ref.Reference;
import java.lang.ref.SoftReference;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;
import java.util.Hashtable;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

/**
* {@link TagLibrary} that loads tags from tag files in a directory.
Expand All @@ -60,7 +68,16 @@ public final class CustomTagLibrary extends TagLibrary {
/**
* Compiled tag files.
*/
private final Map<String,Script> scripts = new Hashtable<String,Script>();
private final Map<String,ExpirableCacheHit<Script>> scripts = new ConcurrentHashMap<>();

private static final class ExpirableCacheHit<S> {
private final long timestamp;
private final Reference<S> script;
ExpirableCacheHit(long timestamp, S script) {
this.timestamp = timestamp;
this.script = new SoftReference<>(script);
}
}

private final List<JellyTagFileLoader> loaders;

Expand Down Expand Up @@ -107,11 +124,29 @@ public Tag createTag(String name, Attributes attributes) throws JellyException {
*/
private Script load(String name) throws JellyException {

Script script = scripts.get(name);
if(script!=null && !MetaClass.NO_CACHE)
return script;
ExpirableCacheHit<Script> cachedScript = scripts.get(name);

if (cachedScript != null) {
if (!MetaClass.NO_CACHE) {
return cachedScript.script.get();
}

script=null;
URL res = classLoader.getResource(basePath + '/' + name + ".jellytag");
if (res == null) {
res = classLoader.getResource(basePath + '/' + name + ".jelly");
}
if (res != null) {
File file = fileOf(res);
if (file != null) {
long timestamp = file.lastModified();
if (timestamp == cachedScript.timestamp) {
return cachedScript.script.get();
}
}
}
}

Script script =null;
if(MetaClassLoader.debugLoader!=null)
script = load(name, MetaClassLoader.debugLoader.loader);
if(script==null)
Expand All @@ -127,21 +162,50 @@ private Script load(String name, ClassLoader classLoader) throws JellyException
res = classLoader.getResource(basePath + '/' + name + ".jelly");
if(res!=null) {
script = loadJellyScript(res);
scripts.put(name,script);
File file = fileOf(res);
if (file == null) {
throw new IllegalStateException("file should not be null here");
}
scripts.put(name, new ExpirableCacheHit<>(file.lastModified(), script));
return script;
}

for (JellyTagFileLoader loader : loaders) {
Script s = loader.load(this, name, classLoader);
if(s!=null) {
scripts.put(name,s);
// can't determine timestamp here, will be cached in production mode
scripts.put(name, new ExpirableCacheHit<>(0, s));
return s;
}
}

return null;
}

private static final Pattern JAR_URL = Pattern.compile("jar:(file:.+)!/.*");

@SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "Files are read from approved plugins, not from user input.")
private static File fileOf(URL res) {
try {
switch (res.getProtocol()) {
case "file":
return new File(res.toURI());
case "jar":
Matcher m = JAR_URL.matcher(res.toString());
if (m.matches()) {
return new File(new URI(m.group(1)));
} else {
return null;
}
default:
return null;
}
} catch (URISyntaxException | IllegalArgumentException x) {
return null; // caching is a best effort
}
}


private Script loadJellyScript(URL res) throws JellyException {
// compile script
JellyContext context = new CustomJellyContext(master);
Expand Down