Re: [PR] Fix resource leaks in ConfigurationSource when loading via URL [logging-log4j2]

["ramanathan1504 (via GitHub)" <gi...@apache.org>]

ramanathan1504 commented on PR #4127:
URL: https://github.com/apache/logging-log4j2/pull/4127#issuecomment-4512070153

   @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`:
   ```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
   <?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 !!😊


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org