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

[Stanimir Simeonoff]

 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.

Stanimir

On Wed, Nov 5, 2014 at 6:15 PM, Daniel Fuchs <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
>
> 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/
>>>>
>>>>  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
>>>
>>>
>>
>