On Thu, Aug 22, 2013 at 10:02 PM, Gary Gregory <garydgreg...@gmail.com>wrote:
> On Thu, Aug 22, 2013 at 8:31 PM, Remko Popma <remko.po...@gmail.com>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 <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 | 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
>>>
>>
>
>
> --
> E-Mail: garydgreg...@gmail.com | 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
>
--
E-Mail: garydgreg...@gmail.com | 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
