logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Remko Popma <remko.po...@gmail.com>
Subject Re: svn commit: r1516477 - /logging/log4j/log4j2/trunk/core/src/main/java/org/apache/logging/log4j/core/jmx/LoggerContextAdmin.java
Date Fri, 23 Aug 2013 04:19:43 GMT
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 <garydgregory@gmail.com<javascript:_e({},
'cvml', 'garydgregory@gmail.com');>
> > wrote:
>
>> On Thu, Aug 22, 2013 at 8:31 PM, Remko Popma <remko.popma@gmail.com<javascript:_e({},
'cvml', 'remko.popma@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.popma@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
>          }
>      }
>
> +    /**
> +     *
>
>
>
>
> --
> E-Mail: garydgregory@gmail.com <javascript:_e({}, 'cvml',
> 'garydgregory@gmail.com');> | ggregory@apache.org <javascript:_e({},
> 'cvml', 'ggregory@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
>

Mime
View raw message