fix: close streams in LoggerContextAdmin.setConfigLocationUri

[SebTardif]

What Problem This Solves

LoggerContextAdmin.setConfigLocationUri opens a FileInputStream or URL stream and builds ConfigurationSource(InputStream, File/URL). That constructor documents that the caller owns the stream.

Built-in factories (XmlConfiguration, JsonConfiguration, PropertiesConfigurationFactory) close getInputStream() when they consume it. That is factory-side cleanup, not a substitute for the caller contract: if a factory path never reads the stream (or returns before consuming it), the descriptor can leak. Same ownership theme as #4127.

Evidence

Leak path (why close is still needed)

Xml/JSON/properties factories close streams they consume. The remaining risk is caller ownership: open stream → hand to ConfigurationSource → factory that never calls getInputStream() (or fails before consume) → stream stays open. Defensive fix is try-with-resources on the caller side while keeping a stream-backed source so resetInputStream() / monitorInterval still re-open the file (buffering into data would short-circuit that and keep stale config; thanks @ramanathan1504).

Fix

try (final InputStream in = new FileInputStream(configFile)) {
  final ConfigurationSource configSource = new ConfigurationSource(in, configFile);
  final Configuration config = ConfigurationFactory.getInstance().getConfiguration(loggerContext, configSource);
  loggerContext.start(config);
}

Same pattern for the URL branch. Double-close after XmlConfiguration's finally is safe.

Red-green

export JAVA_HOME=$(/usr/libexec/java_home -v 17)
./mvnw -pl log4j-core,log4j-core-test -am test \
  -Dtest=LoggerContextAdminSetConfigLocationUriTest \
  -Dsurefire.failIfNoSpecifiedTests=false

Red (production try-with-resources removed, factory that never consumes stream):

AssertionFailedError: expected: <173> but was: <177>  // open FD count

Green (with try-with-resources): 4 tests, 0 failures.

Summary

• Keep stream-backed ConfigurationSource (no full buffer)
• try-with-resources around load for file and URL
• Regression test with non-consuming factory + FD/delete check
• Changelog entry

[ramanathan1504]

@SebTardif thanks for this one, the missing close() is real, I could follow it in the code.

I pulled it down and ran it locally (./mvnw test -pl log4j-core,log4j-core-test -am -Dtest=LoggerContextAdminSetConfigLocationUriTest), two things came up:

1. I reverted LoggerContextAdmin.java back to 2.x and kept your tests, all 3 still pass. They only check the config loads, not that anything is closed. Can you make at least one of them fail without the production change?

2. The buffering looks like it changes more than the close. ConfigurationSource.resetInputStream() returns early when data != null and gives back the same bytes, and that is the method reconfigure() uses. getFile() is still not null so initializeWatchers still registers the file watcher, so a config set over JMX with monitorInterval would keep seeing the file change but keep loading the old config. Same idea as your #4125 — can you keep the stream-backed source and just widen the try-with-resources around the getConfiguration call? Smaller diff and no behaviour change.

One more, XmlConfiguration, JsonConfiguration and PropertiesConfigurationFactory all close getInputStream() in a finally already, so which path is the one that actually leaks? Good to have it written down so the changelog says the right thing.

Let me know when it's updated and I will review it again.

[SebTardif]

@ramanathan1504 thanks for the detailed review — updated.

1. Red-green test: setConfigLocationUri_closesCallerOwnedStreamWhenFactoryDoesNotConsumeIt installs a ConfigurationFactory that returns DefaultConfiguration without reading the stream. Without the try-with-resources, open FD count rises (expected: 173 but was: 177 locally). With the fix, FD count holds and the temp file can be deleted (Windows lock path). Happy-path tests still load real XML.

2. No full buffering: dropped the toByteArray / Source+byte[] approach. Source stays stream/file-backed so resetInputStream() still re-opens the file for monitorInterval watchers.

3. Leak note: Xml/JSON/Properties factories already close when they consume the stream. The gap is the caller-owns-stream contract when a factory path never consumes (or fails before consume). try-with-resources on the JMX method covers that without changing reconfigure/watch behavior.

Ready for another look.

[SebTardif]

@vy Follow-ups from your review are on the tip (b323bff7):

1. File branch uses Files.newInputStream(configFile.toPath()).
2. Closed-stream proof no longer depends on FD counts or deleting the config file. A test ConfigurationFactory captures source.getInputStream() without consuming it; after setConfigLocationUri returns, reading from that stream throws (stream closed).

Could you take another look when you have a moment?