Fix resource leaks in ConfigurationSource when loading via URL

[SebTardif]

What this PR does

In log4j-core/.../config/ConfigurationSource.java, method getConfigurationSource(URL): url.openConnection() creates a URLConnection, but on both error paths (FileNotFoundException, IOException/URISyntaxException) the connection is never closed or disconnected.

For HttpURLConnection this leaks a socket. For JarURLConnection this leaks a file handle (relevant to locked JARs on Tomcat redeploy scenarios).

Fix:

• Added a boolean success flag with try-finally pattern to disconnect/close on error paths
• Reused the existing file variable instead of calling FileUtils.fileFromUri(url.toURI()) a second time

[ramanathan1504]

@SebTardif

"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
Since the fix specifically targets the failure path (!success), we should add a test that intentionally fails to find a file inside a JAR to ensure the handle is released.

Add this to ConfigurationSourceTest.java:

@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
Log4j 2 uses an automated tool for release notes. Please create a new file: src/changelog/.2.x.x/fix_ConfigurationSource_leak.xml :

<?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 !!😊

[SebTardif]

Thanks for the detailed suggestions @ramanathan1504! Added both:

• Failure test: testNoJarFileLeakOnFailure — creates a JAR, requests a missing entry (!/missing/file.xml), and asserts no FD leak and no file lock after the failed load. Follows the same pattern as the existing testNoJarFileLeak and testLoadConfigurationSourceFromJarFile tests.

• Changelog entry: src/changelog/.2.x.x/fix_configuration_source_url_leak.xml

Spotless applied. All 4 ConfigurationSourceTest tests pass.

[ramanathan1504]

@SebTardif

I pushed a follow-up commit to address this in the PR branch.

What I changed:

• Updated ConfigurationSource to close only the already-opened InputStream on failure paths, so we avoid potential URL/JAR resource leaks.
• Updated ConfigurationSourceTest to validate the failure case (missing JAR entry), confirm no file descriptor increase, and ensure the JAR is deletable afterward.

Please avoid force-pushing in future updates so review history and inline comments remain easy to track.

Thanks for the support!

[vy]

@SebTardif, this test passes without your changes. Would you mind adding a test that fails without the changes, please?

[SebTardif]

@vy Added a negative test in 4bfa56c. It injects a counting HttpURLConnection via a custom URLStreamHandler and asserts two things:

1. disconnect() is called on the error path (fails against the original code, which has no finally block)
2. getInputStream() is called exactly once (fails against the intermediate fix that re-invoked getInputStream() in the finally block instead of closing the tracked stream)