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.fu...@oracle.com <mailto:daniel.fu...@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
