Fix resource leaks in ConfigurationSource when loading via URL#4127
Fix resource leaks in ConfigurationSource when loading via URL#4127SebTardif wants to merge 3 commits into
Conversation
|
"Great work on this! Resource leaks in configuration loading can be incredibly difficult to debug, especially in containerized or high-availability environments. This is a solid improvement to Log4j's stability. Suggestions for Completion:To make this PR "merge-ready" for the standards, we should add a failure test case and a changelog entry: 1. Add a Failure Test Case Add this to @Test
void testJarFileLeakOnFailure() throws Exception {
final Path jarFile = prepareJarConfigURL(tempDir);
// Path inside JAR that does NOT exist
final URL brokenJarConfigURL = new URL("jar:" + jarFile.toUri().toURL() + "!/missing/file.xml");
final long expectedFdCount = getOpenFileDescriptorCount();
// This triggers the FileNotFoundException internally
ConfigurationSource configSource = ConfigurationSource.fromUri(brokenJarConfigURL.toURI());
assertNull(configSource, "Source should be null on failure");
// 1. Verify File Descriptors (Unix/Linux)
assertEquals(expectedFdCount, getOpenFileDescriptorCount(),
"File descriptors leaked after failed configuration load!");
// 2. Verify File Lock (Windows)
try {
Files.delete(jarFile);
} catch (IOException e) {
fail("Could not delete JAR file! A handle was leaked in the failure path.");
}
}2. Add a Changelog Entry <?xml version="1.0" encoding="UTF-8"?>
<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns="https://logging.apache.org/xml/ns"
xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
type="fixed">
<issue id="4127" link="https://github.com/apache/logging-log4j2/pull/4127"/>
<description format="markdown">
Fix resource leaks in ConfigurationSource when loading via URL
</description>
</entry>Thanks again for the contribution! don't forgot to apply spotless !!😊 |
In getConfigurationSource(URL), the URLConnection opened via UrlConnectionFactory.createConnection() was never closed or disconnected when an error occurred (FileNotFoundException, IOException, URISyntaxException). For HttpURLConnection this leaks a socket; for JarURLConnection this leaks a file handle. Fix: use a success flag with try-finally to ensure the connection is cleaned up on all error paths while leaving it open on the success path where the stream is handed off to ConfigurationSource. Also replace the redundant FileUtils.fileFromUri(url.toURI()) call on the return line with the existing 'file' variable.
Add testNoJarFileLeakOnFailure to verify that a failed JAR configuration load (missing entry) does not leak file descriptors or hold a lock on the JAR file. Add changelog entry for the ConfigurationSource URL leak fix.
7cc7f49 to
3fde88b
Compare
|
Thanks for the detailed suggestions @ramanathan1504! Added both:
Spotless applied. All 4 ConfigurationSourceTest tests pass. |
|
I pushed a follow-up commit to address this in the PR branch. What I changed:
Please avoid force-pushing in future updates so review history and inline comments remain easy to track. Thanks for the support! |
What this PR does
In
log4j-core/.../config/ConfigurationSource.java, methodgetConfigurationSource(URL):url.openConnection()creates a URLConnection, but on both error paths (FileNotFoundException, IOException/URISyntaxException) the connection is never closed or disconnected.For
HttpURLConnectionthis leaks a socket. ForJarURLConnectionthis leaks a file handle (relevant to locked JARs on Tomcat redeploy scenarios).Fix:
boolean successflag with try-finally pattern to disconnect/close on error pathsfilevariable instead of callingFileUtils.fileFromUri(url.toURI())a second time