Gregory, I'm not sure I agree with this fix. NPE is not the only reason why creating the reader could fail. The input stream is the object associated with the native IO resource (the file handle). As such, it is safer to guarantee that the input stream is always closed no matter what happens. The Reader is just a java object with some char buffer space, and these resources will be GC'ed when it goes out of scope.
Remko PS Check out Assert#isNotNull in the helpers package. I like using this instead of an if statement: short, clearly shows intent, and can be used in assignment statements (often convenient in constructors). On Thursday, August 22, 2013, wrote: > Author: ggregory > Date: Thu Aug 22 14:58:44 2013 > New Revision: 1516477 > > URL: http://svn.apache.org/r1516477 > Log: > Make sure no resources are leaked managing streams and readers. > > Modified: > > logging/log4j/log4j2/trunk/core/src/main/java/org/apache/logging/log4j/core/jmx/LoggerContextAdmin.java > > Modified: > logging/log4j/log4j2/trunk/core/src/main/java/org/apache/logging/log4j/core/jmx/LoggerContextAdmin.java > URL: > http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/core/src/main/java/org/apache/logging/log4j/core/jmx/LoggerContextAdmin.java?rev=1516477&r1=1516476&r2=1516477&view=diff > > ============================================================================== > --- > logging/log4j/log4j2/trunk/core/src/main/java/org/apache/logging/log4j/core/jmx/LoggerContextAdmin.java > (original) > +++ > logging/log4j/log4j2/trunk/core/src/main/java/org/apache/logging/log4j/core/jmx/LoggerContextAdmin.java > Thu Aug 22 14:58:44 2013 > @@ -32,6 +32,7 @@ import java.nio.charset.Charset; > import java.util.Map; > import java.util.concurrent.Executor; > import java.util.concurrent.atomic.AtomicLong; > + > import javax.management.MBeanNotificationInfo; > import javax.management.Notification; > import javax.management.NotificationBroadcasterSupport; > @@ -186,11 +187,22 @@ public class LoggerContextAdmin extends > } > } > > + /** > + * > + * @param uri > + * @param charset MUST not be null > + * @return > + * @throws IOException > + */ > private String readContents(final URI uri, final Charset charset) > throws IOException { > - InputStream in = null; > + if (charset == null) { > + throw new IllegalArgumentException("charset must not be > null"); > + } > + Reader reader = null; > try { > - in = uri.toURL().openStream(); > - final Reader reader = new InputStreamReader(in, charset); > + InputStream in = uri.toURL().openStream(); > + // The stream is open and charset is not null, we can create > the reader safely without a possible NPE. > + reader = new InputStreamReader(in, charset); > final StringBuilder result = new StringBuilder(TEXT_BUFFER); > final char[] buff = new char[PAGE]; > int count = -1; > @@ -200,7 +212,9 @@ public class LoggerContextAdmin extends > return result.toString(); > } finally { > try { > - in.close(); > + if (reader != null) { > + reader.close(); > + } > } catch (final Exception ignored) { > // ignored > } > > >
