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

[Daniel Fuchs]

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
>