On Tue, Jun 30, 2026 at 10:00 PM Mark Thomas <[email protected]> wrote:
>
> On 30/06/2026 20:53, Rémy Maucherat wrote:
>
> > So it should be possible to refactor it to:
> >
> > public long getFileLastModified(String path, boolean virtual)
> > throws IOException {
> > long lastModified = 0;
> > URLConnection urlConnection = null;
> > try {
> > urlConnection = getURLConnection(path, virtual);
> > try (InputStream _ = urlConnection.getInputStream()) {
> > lastModified = urlConnection.getLastModified();
> > } catch (Exception e) {
> > ExceptionUtils.handleThrowable(e);
> > }
> > } catch (IOException ignore) {
> > // Ignore this. It will always fail for non-file based includes
> > }
> > return lastModified;
> > }
> > > Do your Claude actually like the code pattern we have in Tomcat right
> > now ? Somehow I doubt it.
> I haven't been asking it about implementation detail.
>
> At the moment, it seems to be more concerned about code duplication.
> That is mostly fixable although we'd need a couple of copies.
Ah ok, weird. My model is more interested in the patterns it does not
like or that it thinks are weird.
> There is also a query whether the fd leak is real or not. It certainly
> was real when these fixes were originally written. I suspect it might
> not be considering the more extreme edge cases where the resource may be
> in a JAR packaged in another JAR/WAR. If I had (a lot more) time, I'd
> run some tests to look into that.
Let's say the URL is a https one. In that case, reading the JVM code,
you have to cast to HttpURLConnection and call disconnect() otherwise
getInputStream().close() does not do the same thing.
Actually, I see CloseableURLConnection in org.apache.commons.io, which
has AutoCloseable and all it does in its close() is:
if (conn instanceof HttpURLConnection) {
((HttpURLConnection) conn).disconnect();
}
Coincidentally, our code will not do that. However, for JAR URLs, our
strategy works perfectly (getInputStream().close() will close the JAR
file), but only as long as .setUseCaches(false) has been called. We do
it in the JreLeakPreventionListener using
URLConnection.setDefaultUseCaches("jar", false). If any of that
scaffolding is missing, getInputStream().close() does nothing useful.
Maybe this refactoring would be better:
1) Wrap into an AutoCloseable URL connection like commons.io is doing.
2) On connect, always set setsetUseCaches to false whenever possible,
especially for JarURLConnection.
3) On close, handle HttpURLConnection and call disconnect.
4) On close, handle JarURLConnection and make sure to call
getInputStream().close(), unless getInputStream has been called, in
which case it has to be assumed that the caller should close it.
I'll add the utility class for review.
About duplication, the pattern looks to be in plenty of spots:
org/apache/jasper/compiler/Compiler.java:
iuc.getInputStream().close();
org/apache/jasper/compiler/TldCache.java:
conn.getInputStream().close();
org/apache/jasper/JspCompilationContext.java:
uc.getInputStream().close();
org/apache/tomcat/Jar.java: urlConn.getInputStream().close();
org/apache/tomcat/util/file/ConfigurationSource.java:
connection.getInputStream().close();
org/apache/catalina/startup/ContextConfig.java:
uc.getInputStream().close();
org/apache/catalina/startup/ContextConfig.java:
uc.getInputStream().close();
org/apache/catalina/ssi/SSIServletExternalResolver.java:
urlConnection.getInputStream().close();
org/apache/catalina/users/MemoryUserDatabase.java:
uConn.getInputStream().close();
Rémy
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
