Skip to content

Commit

Permalink
Fix and simplify handling of JSPParser classloader
Browse files Browse the repository at this point in the history
The classloader was reset after each invocation by calls to
resetClassLoaders, so there is already little caching present here.

The old behavior though posed a different problem: the classloaders
were shared into multiple threads and calls to resetClassLoaders were
not coordinated, so the "slower" thread could be faced with a closed
classloader. For an URLClassLoader this means, that classes already
loaded work, new classes can't be loaded. This leads to difficult to
debug situations.

At runtime this was observed as:

Unable to read TLD "META-INF/liferay-portlet-ext.tld" from JAR file
"file:/home/matthias/.m2/repository/com/liferay/portal/release.portal.api/7.4.3.94-ga94/release.portal.api-7.4.3.94-ga94.jar":
org.apache.jasper.JasperException: PWC6112: Failed to load or
instantiate TagExtraInfo class: com.liferay.taglib.portlet.ActionURLTei
  • Loading branch information
matthiasblaesing committed Jul 12, 2024
1 parent 86c43be commit c7e9bad
Showing 1 changed file with 29 additions and 53 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,7 @@ public class WebAppParseSupport implements WebAppParseProxy, PropertyChangeListe
private ServletContext editorContext;
private ServletContext diskContext;
private JspRuntimeContext rctxt;
private ParserClassLoader waClassLoader;
private ParserClassLoader waContextClassLoader;

// This is intended for tests exclusively!!!
@SuppressWarnings("FieldMayBeFinal")
private static boolean noReset = false;
private URL[][] classLoaderSources = null;

// @GuardedBy(this)
/**
Expand Down Expand Up @@ -226,8 +221,6 @@ public JspParserAPI.JspOpenInfo getJspOpenInfo(FileObject jspFile, boolean useEd
return new JspParserAPI.JspOpenInfo(epd.isXMLSyntax(), epd.getEncoding());
} catch (IOException | JasperException | RuntimeException e) {
LOG.fine(e.getMessage());
} finally {
resetClassLoaders();
}
return DEFAULT_JSP_OPEN_INFO;
}
Expand Down Expand Up @@ -313,34 +306,14 @@ private void createClassLoaders() {
}

@SuppressWarnings("CollectionsToArray")
URL[] loadingURLs = loadingTable.values().toArray(new URL[0]);
@SuppressWarnings("CollectionsToArray")
URL[] tomcatURLs = tomcatTable.values().toArray(new URL[0]);
URL[][] urls = {
loadingTable.values().toArray(new URL[0]),
tomcatTable.values().toArray(new URL[0])
};

waClassLoader = new ParserClassLoader(loadingURLs, tomcatURLs, getClass().getClassLoader());
waContextClassLoader = new ParserClassLoader(loadingURLs, tomcatURLs, getClass().getClassLoader());
this.classLoaderSources = urls;
}

// #128360
private void resetClassLoaders() {
if (noReset) {
return;
}
URL[] loadingURLs = waClassLoader.getLoadingURLs();
URL[] tomcatURLs = waClassLoader.getTomcatURLs();
URL[] loadingContextURLs = waContextClassLoader.getLoadingURLs();
URL[] tomcatContextURLs = waContextClassLoader.getTomcatURLs();

try {
waClassLoader.close();
waContextClassLoader.close();
} catch (IOException | RuntimeException e) {
LOG.log(Level.WARNING, null, e);
}
waClassLoader = new ParserClassLoader(loadingURLs, tomcatURLs, waClassLoader.getParent());
waContextClassLoader = new ParserClassLoader(loadingContextURLs, tomcatContextURLs, waContextClassLoader.getParent());
}

// #127379 - XXX review after listening on a web module is possible
private FileObject getWebInf() {
if (wmRoot == null) {
Expand Down Expand Up @@ -444,10 +417,10 @@ public JspParserAPI.ParseResult analyzePage(FileObject jspFile, /*String compila
checkReinitCachesTask();
JspCompilationContext ctxt = createCompilationContext(jspFile, true);

try {
return callTomcatParser(jspFile, ctxt, waContextClassLoader, errorReportingMode);
} finally {
resetClassLoaders();
try (URLClassLoader cl = new ParserClassLoader(this.classLoaderSources[0], this.classLoaderSources[1], getClass().getClassLoader())) {
return callTomcatParser(jspFile, ctxt, cl, errorReportingMode);
} catch (IOException ex) {
throw new RuntimeException(ex);
}
}

Expand Down Expand Up @@ -484,7 +457,8 @@ public synchronized URLClassLoader getWAClassLoader() {
reinitOptions();
}
// XXX #128360 - classloader is leaking out - no chance to close it
return waClassLoader;
// Callers must close the classloader as soon as possible!
return new ParserClassLoader(this.classLoaderSources[0], this.classLoaderSources[1], getClass().getClassLoader());
}

@Override
Expand Down Expand Up @@ -717,22 +691,24 @@ void reinitTagLibMappings() {
}

Thread compThread = new WebAppParseSupport.InitTldLocationCacheThread(lc);
compThread.setContextClassLoader(waContextClassLoader);
long start = 0;
if (LOG.isLoggable(Level.FINE)) {
start = System.currentTimeMillis();
LOG.fine("InitTldLocationCacheThread start"); //NOI18N
}
compThread.start();

try {
compThread.join();
try (URLClassLoader cl = new ParserClassLoader(this.classLoaderSources[0], this.classLoaderSources[1], getClass().getClassLoader())) {
compThread.setContextClassLoader(cl);
long start = 0;
if (LOG.isLoggable(Level.FINE)) {
long end = System.currentTimeMillis();
LOG.log(Level.FINE, "InitTldLocationCacheThread finished in {0} ms", (end - start)); //NOI18N
start = System.currentTimeMillis();
LOG.fine("InitTldLocationCacheThread start"); //NOI18N
}
compThread.start();

try {
compThread.join();
if (LOG.isLoggable(Level.FINE)) {
long end = System.currentTimeMillis();
LOG.log(Level.FINE, "InitTldLocationCacheThread finished in {0} ms", (end - start)); //NOI18N
}
} catch (InterruptedException e) {
LOG.log(Level.INFO, null, e);
}
} catch (InterruptedException e) {
LOG.log(Level.INFO, null, e);
}

// obtain the current mappings after parsing
Expand All @@ -754,7 +730,7 @@ void reinitTagLibMappings() {
}
// cache tld files under WEB-INF directory as well
mappings.putAll(getImplicitLocation());
} catch (NoSuchFieldException | IllegalAccessException e) {
} catch (NoSuchFieldException | IllegalAccessException | IOException e) {
LOG.log(Level.INFO, null, e);
}
}
Expand Down

0 comments on commit c7e9bad

Please sign in to comment.