Sorry if I was unclear. What if creating the Reader fails after the InputStream was created? Then there is no reader object to call close on... In not sure that protecting against a NPE is enough, so I want to close the InputStream in the finally clause. Closing the Reader as well is fine but optional IMO.
On Friday, August 23, 2013, Gary Gregory wrote: > Hi Remko, > > I tried it both ways and it just looks simpler to read this way. The > reader passes the close to its wrapped stream. In both cases, you need to > close the reader (or the stream) and protect that close with a null guard. > > Gary > > > On Thu, Aug 22, 2013 at 7:10 PM, Remko Popma <remko.po...@gmail.com>wrote: > > 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 reade > > -- > E-Mail: garydgreg...@gmail.com <javascript:_e({}, 'cvml', > 'garydgreg...@gmail.com');> | ggreg...@apache.org <javascript:_e({}, > 'cvml', 'ggreg...@apache.org');> > Java Persistence with Hibernate, Second > Edition<http://www.manning.com/bauer3/> > JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> > Spring Batch in Action <http://www.manning.com/templier/> > Blog: http://garygregory.wordpress.com > Home: http://garygregory.com/ > Tweet! http://twitter.com/GaryGregory >
