RFR: 8060132: Handlers configured on abstract nodes in logging.properties are not always properly closed

[Daniel Fuchs]

On 05/11/14 19:47, Stanimir Simeonoff wrote:
>   945                     } catch (Exception ex) {
>   946                         System.err.println("Can't load log handler \"" + type + "\"");
>   947                         System.err.println("" + ex);
>   948                         ex.printStackTrace();
>   949                     }
>
> I'm not quite sure if handling Error (in particular any subclass of LinkageError) would make sense here (similar to the invocation of listeners).
> Of course there are other case handling just Exception and using word instead of 'type' but that's matter of another clean up.

Yes... LogManager should probably have some kind of ErrorManager - and
not just for listener invocation...

best regards,

-- daniel


>
> Stanimir
>
> On Wed, Nov 5, 2014 at 6:15 PM, Daniel Fuchs<daniel.fuchs at oracle.com  <mailto:daniel.fuchs at oracle.com>>  wrote:
>
>     Hi Mandy, Stanimir,
>
>     Here is the new webrev:
>     http://cr.openjdk.java.net/~__dfuchs/webrev_8060132/webrev.__03/index.html
>     <http://cr.openjdk.java.net/~dfuchs/webrev_8060132/webrev.03/index.html>
>
>     best regards,
>
>     -- daniel
>
>
>     On 05/11/14 12:47, Daniel Fuchs wrote:
>
>         On 04/11/14 23:40, Mandy Chung wrote:
>
>             On 11/4/14 8:48 AM, Daniel Fuchs wrote:
>
>                 Hi,
>
>                 Here is a revised patch that introduces a new
>                 <logger>.handlers.__ensureCloseOnReset=true|false
>                 property.
>
>             You have explored several different options and as you have
>             found,
>             none of them is ideal.  I think having a property to specify
>             ensureCloseOnReset would be reasonable to address the memory
>             leak
>             concern I have.
>
>             Would it be better to name it <logger>.handlers.__closeOnReset?
>
>
>         Well - closeOnReset is the normal behavior - if the logger
>         is still around. So I believe ensureCloseOnReset is a better name.
>
>             I am less sure if we need to have a global switch to revert
>             the default - do we expect the number of <logger>.handlers
>             is large? I would expect there would be small number.
>
>
>         I would expect the cases where you need to specify
>         ensureCloseOnReset will be rare. So maybe we should wait
>         until the case arises before implementing a global switch:
>         we can add that later if it's needed. The local switches
>         will provide a workable (even though possibly cumbersome)
>         work around.
>
>                 This property can be used as a fallback to turn the new
>                 behavior
>                 off in the improbable case where an application may need
>                 to do
>                 so. I have updated the LogManager class-level API
>                 documentation.
>
>                 http://cr.openjdk.java.net/~__dfuchs/webrev_8060132/webrev.__02/
>                 <http://cr.openjdk.java.net/~dfuchs/webrev_8060132/webrev.02/>
>
>             line 104-114: s/"true"/{@code true}
>             {@code reset} should be {@link ...} to the reset method (the
>             first occurrance)
>
>
>         Will do.
>
>             It doesn't seem necessary to specify "strongly referenced".
>             Maybe it's good to say "it will ensure the handlers associated
>             with the loggers will be closed at shutdown or at reset.
>
>
>         OK
>
>             If "closeOnReset is false", application should ensure the
>             handlers be closed before the logger gets garbage collected."
>
>
>         I will try to find something.
>
>             - something along that line.
>
>             typos:
>             186:  explicitely -> explicitly
>
>
>         I keep on doing this same mistake again and again :-(
>
>             Perhaps PersistentLogger can be renamed to CloseOnResetLogger
>             to make it clear what these loggers are kept for.  Similarly
>             for the variable name "persistentLogger" may be renamed too.
>
>
>         OK.
>
>
>             917: final might be adding noise
>
>         I like final ;-)
>
>             940: persitentLoggers
>
>         OK
>
>
>             line 917: you don't need (names.length == 0) to determine
>             the default,
>             right?  if names.length == 0, it wouldn't add to the
>             persistentLoggers
>             list
>             as no iteration in the for loop.
>
>
>         I will use the syntax that Stanimir suggested. It's much simpler
>         than what I originally wrote. If names.length == 0 we don't need to
>         look up the ensureCloseOnReset property.
>
>         Thanks Mandy,
>
>         I will send a new webrev shortly - with both yours & Stanimir
>         comments
>         integrated (barring the global switch - as we could do that in a
>         followup revision if that's really needed).
>
>         best regards,
>
>         -- daniel
>
>
>
>             Mandy
>
>
>
>