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

[Daniel Fuchs]

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
>>
>