Thanks Gary! I think that's fine.
It does look a bit verbose, perhaps we can make a Closer class in the
helpers package with methods like this:
public static void closeSilent(Closable closable) {
try {
if (in != null) {
in.close();
}
} catch (final Exception ignored) {
// ignored
}
}
public static void closeWarn(Closable closable, String name) {
try {
if (in != null) {
in.close();
}
} catch (final Exception ignored) {
LOGGER.warn("Could not close resource: " + name);
}
}
On Friday, August 23, 2013, Gary Gregory wrote:
> On Thu, Aug 22, 2013 at 10:02 PM, Gary Gregory
> <[email protected]<javascript:_e({}, 'cvml', '[email protected]');>
> > wrote:
>
>> On Thu, Aug 22, 2013 at 8:31 PM, Remko Popma
>> <[email protected]<javascript:_e({}, 'cvml', '[email protected]');>
>> > wrote:
>>
>>> 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.
>>>
>>
>> OK, I can see that.
>>
>> Since each JRE can implement the reader differently, is should also be
>> closed. This would be easier in Java 7 with a try-with-resources, so now I
>> have:
>>
>
> Arg, slippery keys. I mean:
>
> private String readContents(final URI uri, final Charset charset) throws
> IOException {
> InputStream in = null;
> Reader reader = null;
> try {
> in = uri.toURL().openStream();
> reader = new InputStreamReader(in, charset);
> final StringBuilder result = new StringBuilder(TEXT_BUFFER);
> final char[] buff = new char[PAGE];
> int count = -1;
> while ((count = reader.read(buff)) >= 0) {
> result.append(buff, 0, count);
> }
> return result.toString();
> } finally {
> try {
> if (in != null) {
> in.close();
> }
> } catch (final Exception ignored) {
> // ignored
> }
> try {
> if (reader != null) {
> reader.close();
> }
> } catch (final Exception ignored) {
> // ignored
> }
> }
> }
>
> Is there a better way to make sure all allocated resources are freed? The
> Reader contract is clear:
>
> /**
> * Closes the stream and releases any system resources associated with
> * it. Once the stream has been closed, further read(), ready(),
> * mark(), reset(), or skip() invocations will throw an IOException.
> * Closing a previously closed stream has no effect.
> *
> * @exception IOException If an I/O error occurs
> */
> abstract public void close() throws IOException;
>
> So that would be enough but as you point out, building the reader might
> blow up.
>
> In my JRE (Oracle) I see:
>
> public InputStreamReader(InputStream in, Charset cs) {
> super(in);
> if (cs == null)
> throw new NullPointerException("charset");
> sd = StreamDecoder.forInputStreamReader(in, this, cs);
> }
>
> and StreamDecoder is internal code to Oracle so who knows what it does.
>
> So... the InputStream should also be closed:
>
> /**
> * Closes this input stream and releases any system resources
> associated
> * with the stream.
> *
> * <p> The <code>close</code> method of <code>InputStream</code> does
> * nothing.
> *
> * @exception IOException if an I/O error occurs.
> */
> public void close() throws IOException {}
>
> but in this case the behavior of closing again, is undefined, so it should
> happen first, before closing the reader, whose contract states that it can
> handled being closed many times.
>
> Can this be done better?
>
> Gary
>
> Gary
>
>
>
>
> 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 <[email protected]>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
> }
> }
>
> + /**
> + *
>
>
>
>
> --
> E-Mail: [email protected] <javascript:_e({}, 'cvml',
> '[email protected]');> | [email protected] <javascript:_e({},
> 'cvml', '[email protected]');>
> 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
>
