New webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.01 -- daniel On 9/10/14 1:42 PM, Daniel Fuchs wrote:

On 9/10/14 1:25 PM, Alan Bateman wrote:

...

On 10/09/2014 10:49, Daniel Fuchs wrote:

...

Hi,

Please find below a proposed patch for

8043306 - Provide a replacement for the API that allowed to listen for LogManager configuration changes https://bugs.openjdk.java.net/browse/JDK-8043306

Proposed Patch: http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.00/

It would be nice if ConfigurationListener could be an interface. I realize this might mean looking again at the security concerns in this area again to see if we could get away without requiring a construction time permission check. Clearly if it could be an interface then it begs the question as to why it's just not a Runnable but that probably means yet more effort to figure out the right security and whether the access control context should be recorded when registering.

One nice thing about ConfigurationListener being an abstract class is that I don't need to invent an 'IdentityCopyOnWriteArrayList' which is what I would prefer to use if equals() could be overriden by implementations.

Also I like it that you need to have the LoggingPermission("Control") to subclass ConfigurationListener.

In practice - I believe that there aren't that many application which need this functionality - and I also believe that for those that need it then there will be a single ConfigurationListener that will be registered only once, and therefore having an abstract class for ConfigurationListener shouldn't be that much of a hassle for them.

...

Assuming ConfigurationListener remains as an abstract class then the no-arg constructor will need a one-line javadoc comment.

Oh - right - thanks for catching that.

...

A small suggestion for addConfigurationListener to return LogManager to facilitate method invocation chaining when configuring the LogManager. It could be done for remove too but that is probably less interesting.

Good suggestion.

...

Another comment on addConfigurationListener is that about the wording "re-read and the configuration is changed". I think the configuration listeners are invoked when ever the configuration is read even if there aren't any changes.

Right. Although I believe it would be difficult (though maybe not impossible) to manage to register a configuration listener before the configuration is read for the first time ;-)

...

The javadoc links in the readConfiguration methods make the line length a bit inconsistent with the existing javadoc, maybe the package name could be just dropped from the links or the link moved to the next line.

OK - I'll see what I can do.

Thanks Alan!

-- daniel

...

-Alan.

On 9/10/14 2:25 PM, Paul Sandoz wrote:

FWIW i think you could do:

removeIf(e -> listener == e);

Ah ah. Interesting!

Idiomatically callback methods are often prefixed with on:

onConfigurationLoaded.

Thanks! Exactly what I was looking for :-) I'll wait for more comments before updating the webrev, but I like your suggestion. I should have remembered the idiom - it's been too long since I have code my last 'onMouseClicked()' :-)

My inclination is this should be an interface if at all possible. The permission check on construction seems odd to me but i don't understand the security requirements here. It's very tempting to suggest not defining any new interface and reuse Runnable, since as you indicate below this is not expected to be common functionality and there is likely to be a single instance registered.

I'd actually prefer not to use Runnable here if it could be avoided... -- daniel

Paul.

...

Also I like it that you need to have the LoggingPermission("Control") to subclass ConfigurationListener.

In practice - I believe that there aren't that many application which need this functionality - and I also believe that for those that need it then there will be a single ConfigurationListener that will be registered only once, and therefore having an abstract class for ConfigurationListener shouldn't be that much of a hassle for them.

On 9/11/2014 9:17 AM, Daniel Fuchs wrote:

On 9/10/14 1:25 PM, Alan Bateman wrote:

...

On 10/09/2014 10:49, Daniel Fuchs wrote:

...

Hi,

Please find below a proposed patch for

8043306 - Provide a replacement for the API that allowed to listen for LogManager configuration changes https://bugs.openjdk.java.net/browse/JDK-8043306

Proposed Patch: http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.00/

It would be nice if ConfigurationListener could be an interface. I realize this might mean looking again at the security concerns in this area again to see if we could get away without requiring a construction time permission check. Clearly if it could be an interface then it begs the question as to why it's just not a Runnable but that probably means yet more effort to figure out the right security and whether the access control context should be recorded when registering.

Given that both Paul & Alan have indicated that they would prefer to use interfaces for listeners, I have been working on a version that uses 'Runnable'.

Thanks to Paul & Alan for all the suggestions that went into this new patch (especially for refining the spec and suggesting a nice way to rewrite addConfigurationListener() using lambdas)!

http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.03/

Looks good to me. It's much cleaner. Mandy

Hello, Just a note, acc is going to leak the context class loader until the listener is removed. That should be noted in the documentation. Also if there is a runtime exception during run() of a listener it will block any other listeners to be invoked and the exception is going to be propagated past readConfiguration(). I suppose that's ok however the listeners are to be invoked in a 'random' order depending on their identityHashCode. So if there is an exception in the last registered there is no guarantee to invoke even the 1st added listener. The entire process is not deterministic which is not ok for listeners invocation. Stanimir

Good point. But I expect the concrete listener class will also be usually loaded by the context class loader - so isn't that kind of expected anyway? I didn't want to stuff too many implementation details into the spec. Maybe that could be an API note however. Would anyone be able to suggest a good wording?

On leaks. The c-tor of LogManager leaks the thread group and the current AccessControlContext till JVM shutdown. The constructor code should be something like the one below with Cleaner getting ThreadGroup in its c-tor as well. java.security.AccessController.doPrivileged(//zip thread.inheritedAccessControlContext new java.security.PrivilegedAction<Void>() { public Void run() { // put the cleaner in the systsm threadgroup ThreadGroup grp = Thread.currentThread().getThreadGroup(); for(ThreadGroup parent;null!=(parent = grp.getParent());) { grp = parent; } Cleaner cleaner = new Thread(grp); cleaner.setContextClassLoader(null);//zip the classloader // Add a shutdown hook to close the global handlers. try { Runtime.getRuntime().addShutdownHook(cleaner); } catch (IllegalStateException e) { // If the VM is already shutting down, // We do not need to register shutdownHook. } return null; } }); Also if there is a runtime exception during run() of a listener it will

...

block any other listeners to be invoked and the exception is going to be propagated past readConfiguration(). I suppose that's ok however the listeners are to be invoked in a 'random' order depending on their identityHashCode. So if there is an exception in the last registered there is no guarantee to invoke even the 1st added listener. The entire process is not deterministic which is not ok for listeners invocation.

The 'old' deprecated code was already like that. This doesn't mean we shouldn't try to make it better - but I have to ask whether it's worth the trouble.

However the listeners are to be invoked in the same order they have been added.

Are there applications where there will be several listeners?

Not sure, however listeners can be added at any time. readConfiguration(InputStream) is public too, so it's plausible.

And if there are - shouldn't these listener run their code in a try { } catch ( ) { } to ensure that the exception will be logged - or rather reported - where it belong?

IMO, the API can't demand any cooperation from user space code .

IMO the only thing we could do here is to silently drop any runtime/exception or error, which I am a bit reluctant to do; I believe it's the concrete implementation of the listener which should take care of that.

I'd rather run the listeners in try/catch and throw the 1st exception at the end. Defining the 1st however requires obeying the order the listeners have been added. As far as I see there is no documentation how the listeners are supposed to behave or be invoked.

Stanimir

On 09/12/2014 12:45 PM, Daniel Fuchs wrote:

...

However the listeners are to be invoked in the same order they have been added.

I am still unconvinced this is worth the additional complexity it would bring to the implementation. The deprecated methods were using HashMap to store listeners, and therefore the order in which listeners were invoked was also undefined. Nobody has ever complained.

What about using a synchronized LinkedHashMap<ListenerAction, ListenerAction>() with the following double-purpose inner class: private static final class ListenerAction implements PrivilegedAction<Void> { private final Runnable listener; private final AccessControlContext acc; ListenerAction(Runnable listener, AccessControlContext acc) { this.listener = listener; this.acc = acc; } void invoke() { if (acc == null) { run(); } else { AccessController.doPrivileged(this, acc); } } // not to be run() directly - use invoke() instead @Override public Void run() { listener.run(); return null; } @Override public int hashCode() { return System.identityHashCode(listener); } @Override public boolean equals(Object obj) { return (obj instanceof ListenerAction) && ((ListenerAction) obj).listener == listener; } }

(http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/3ae82f0c6b31/src/share/c...)

...

I'd rather run the listeners in try/catch and throw the 1st exception at the end. Defining the 1st however requires obeying the order the listeners have been added. As far as I see there is no documentation how the listeners are supposed to behave or be invoked.

We could do that - invoke all listeners and rethrow the exception caught by the first listener that failed (on the basis that if any other listener subsequently fails it may be because the previous listener has left the system in an inconsistent state, and that therefore the first exception caught has 'more value' than the others).

I am not completely convinced this is a better behavior than simply stopping invoking listeners at the first exception. I wonder if there would be a good wording to say that listeners are not supposed to throw, but that if they do, the exceptions will be propagated...

You're probably right. But in case you changed your mind, you could rethrow the 1st exception thrown, with possible further exceptions added to it a suppressed exceptions... Regards, Peter

best regards,

-- daniel

On 9/12/14 1:55 PM, Peter Levart wrote:

On 09/12/2014 12:45 PM, Daniel Fuchs wrote:

...

...

However the listeners are to be invoked in the same order they have been added.

I am still unconvinced this is worth the additional complexity it would bring to the implementation. The deprecated methods were using HashMap to store listeners, and therefore the order in which listeners were invoked was also undefined. Nobody has ever complained.

What about using a synchronized LinkedHashMap<ListenerAction, ListenerAction>() with the following double-purpose inner class:

private static final class ListenerAction implements PrivilegedAction<Void> { private final Runnable listener; private final AccessControlContext acc;

ListenerAction(Runnable listener, AccessControlContext acc) { this.listener = listener; this.acc = acc; }

void invoke() { if (acc == null) { run(); } else { AccessController.doPrivileged(this, acc); } }

// not to be run() directly - use invoke() instead @Override public Void run() { listener.run(); return null; }

@Override public int hashCode() { return System.identityHashCode(listener); }

@Override public boolean equals(Object obj) { return (obj instanceof ListenerAction) && ((ListenerAction) obj).listener == listener; } }

yes - that's what I'm calling the additional complexity ;-) -- daniel

...

(http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/3ae82f0c6b31/src/share/c...)

...

I'd rather run the listeners in try/catch and throw the 1st exception at the end. Defining the 1st however requires obeying the order the listeners have been added. As far as I see there is no documentation how the listeners are supposed to behave or be invoked.

We could do that - invoke all listeners and rethrow the exception caught by the first listener that failed (on the basis that if any other listener subsequently fails it may be because the previous listener has left the system in an inconsistent state, and that therefore the first exception caught has 'more value' than the others).

I am not completely convinced this is a better behavior than simply stopping invoking listeners at the first exception. I wonder if there would be a good wording to say that listeners are not supposed to throw, but that if they do, the exceptions will be propagated...

You're probably right. But in case you changed your mind, you could rethrow the 1st exception thrown, with possible further exceptions added to it a suppressed exceptions...

Regards, Peter

...

best regards,

-- daniel

On 9/12/14 2:37 PM, Alan Bateman wrote:

On 12/09/2014 11:45, Daniel Fuchs wrote:

...

I am still unconvinced this is worth the additional complexity it would bring to the implementation. The deprecated methods were using HashMap to store listeners, and therefore the order in which listeners were invoked was also undefined. Nobody has ever complained.

I'm not convinced either. The previous methods were rarely used (prior to their deprecation and eventual removal there was a static analysis done on tens of thousands of projects to get some data on actual usage and there were only a handful of actual usages). The only reason for introducing alternative methods is that there is a solution for the tiny number of cases where LogManager can't be sub-classes. So I think it's best to keep it as simpler as possible.

Would modifying the specification of addConfigurationListener() as follows be sufficient to make the workings of the proposed implementation clearer? /** * Adds a configuration listener to be invoked each time the logging * properties are read and the configuration is changed. * If the listener is already registered the method does nothing. * <p> * The order in which the listeners are invoked is unspecified. * <br> * If a security manager is on, each listener will be invoked in the * {@linkplain AccessControlContext security context} with which it was * registered. * <p> * It is recommended that listeners do not throw errors or exceptions. * Exceptions or errors thrown by listeners will * be propagated to the caller of {@link #readConfiguration()} (or {@link * #readConfiguration(java.io.InputStream)}) preventing those listeners * that haven't been invoked yet from being fired. * * @param listener A configuration listener that will be invoked after the * configuration changed. * @return This LogManager. * @throws SecurityException if a security manager exists and if the * caller does not have LoggingPermission("control"). * @throws NullPointerException if the listener is null. * * @since 1.9 */ public LogManager addConfigurationListener(Runnable listener) { best regards, -- daniel

-Alan

Thanks Alan! I have updated the webrev with your suggestions: http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.04/ -- daniel On 9/12/14 3:54 PM, Alan Bateman wrote:

On 12/09/2014 14:38, Daniel Fuchs wrote:

...

Would modifying the specification of addConfigurationListener() as follows be sufficient to make the workings of the proposed implementation clearer?

/** * Adds a configuration listener to be invoked each time the logging * properties are read and the configuration is changed.

Would "... each time that the logging configuration is read" be sufficient? (just thinking that it might be more technically correct because the configuration might not have changed).

...

* If the listener is already registered the method does nothing. * <p> * The order in which the listeners are invoked is unspecified. That is okay, although it is the same as not saying anything :-)

...

* <br> * If a security manager is on, each listener will be invoked in the * {@linkplain AccessControlContext security context} with which it was * registered. An alternative is "The listener is invoked with privileges that are restricted by the calling context of this method".

...

* <p> * It is recommended that listeners do not throw errors or exceptions. * Exceptions or errors thrown by listeners will * be propagated to the caller of {@link #readConfiguration()} (or {@link * #readConfiguration(java.io.InputStream)}) preventing those listeners * that haven't been invoked yet from being fired. That seems okay. Here's a variation to consider:

"If a listener terminates with an uncaught error or exception then it will be propagated to the caller of readConfiguraiton and prevent other listeners from being notified".

-Alan.

On 9/12/14 4:42 PM, Alan Bateman wrote:

On 12/09/2014 15:16, Daniel Fuchs wrote:

...

Thanks Alan!

I have updated the webrev with your suggestions:

http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.04/

-- daniel

A minor suggestion for readConfiguration is that "Any register configuration listeners .." might be a bit better than "The configuration listeners ...".

In addConfigurationListener there is a <br> between the two sentences that talk about exceptions, I don't know if you intended that.

Done. I regenerated the webrev in place. http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.04/ -- daniel

Otherwise the javadoc looks good to me.

-Alan.

On 9/12/14 5:39 PM, Jason Mehrens wrote:

Daniel,

I suppose that the propagating uncaught exceptions to the caller was the previous behavior of the old property change methods but it seems out of place for the LogManager. The LogManager is a global resource so broken listener code in web app A could suppress notifications in web app B and C. That doesn't seem very nice. A lot of the LogManager code handles exception via catch, report or ignore, and continue when dealing with handlers etc. I would think that same strategy applies here too.

Hi Jason, Yes - that was also the behavior of the old property change methods. When the global LogManager calls itself readConfiguration it will silently ignore exceptions that might be raised. I'm not too keen in specifying that exceptions raised by listeners will be silently dropped. We're in the process of reinitializing the configuration, so trying to log the exceptions through a Logger would have its own risks too. Reporting exceptions raised during initialization and configuration is an area where java.util.logging is clearly lacking. For this patch - I think that our options are limited to the alternative: 1. propagate the exception 2. catch and silentlty drop the exception What is the 'better' behavior (and I agree neither of the two are ideal) probably depends on what is the typical use case for these listeners. My assumption is that in most cases - there will be a single listener, in which case 1. is probably better. I haven't seen any bug complaining about how exceptions were handled in the previous implementation, though I have seen some complaining about swallowed exceptions... That said - if there is consensus that 2. is better - I have no real objection except those stated above. best regards, -- daniel

Jason

----------------------------------------

...

Date: Fri, 12 Sep 2014 16:59:03 +0200 From: daniel.fuchs@oracle.com To: Alan.Bateman@oracle.com Subject: Re: RFR: 8043306 - Provide a replacement for the API that allowed to listen for LogManager configuration changes CC: core-libs-dev@openjdk.java.net

On 9/12/14 4:42 PM, Alan Bateman wrote:

...

On 12/09/2014 15:16, Daniel Fuchs wrote:

...

Thanks Alan!

I have updated the webrev with your suggestions:

http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.04/

-- daniel

A minor suggestion for readConfiguration is that "Any register configuration listeners .." might be a bit better than "The configuration listeners ...".

In addConfigurationListener there is a <br> between the two sentences that talk about exceptions, I don't know if you intended that. Done. I regenerated the webrev in place. http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.04/

-- daniel

...

Otherwise the javadoc looks good to me.

-Alan.

Hi Jason, Logging on System.err is usually frowned upon - especially if there are alternatives. May I suggest the following? 1. do option #1, and only fail after all listeners have been notified as Alan indicated 'it might not be too bad'. As Paul suggested to me privately we could use Throwable.addSuppressed to link the suppressed exceptions, if there are any. 2. Log an RFE for LogManager to use ErrorManager (or some similar object). IMHO the proposal #1 above should not cripple how LogManager will evolve in the future too much - especially if it is a rarely used functionality. Here is a new webrev - with updated test case: http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.05/ best regards -- daniel On 9/12/14 7:21 PM, Jason Mehrens wrote:

Daniel,

Agreed. My preference would be to:

1. Catch and report them to 'System.err'. That type of code happens in the LogManager and exceptions in this case are not the normal condition.

2. Don't specify how uncaught exceptions are handled in the javadocs.

3. Outside of this issue, make an RFE to allow the LogManager to contain a j.u.l.ErrorManager and use it to replace all calls to System.err to use this new root/default error manager. Once these exception are captured in a error manager you can alter the behavior (rethrow, write to console, ignore, report to O/S event log, etc.)

I worry that over specification of the exception handling might cripple how the LogManager can evolve with regard to future RFEs.

Jason

----------------------------------------

...

Date: Fri, 12 Sep 2014 18:54:12 +0200 From: daniel.fuchs@oracle.com To: jason_mehrens@hotmail.com CC: core-libs-dev@openjdk.java.net Subject: Re: RFR: 8043306 - Provide a replacement for the API that allowed to listen for LogManager configuration changes

On 9/12/14 5:39 PM, Jason Mehrens wrote:

...

Daniel,

I suppose that the propagating uncaught exceptions to the caller was the previous behavior of the old property change methods but it seems out of place for the LogManager. The LogManager is a global resource so broken listener code in web app A could suppress notifications in web app B and C. That doesn't seem very nice. A lot of the LogManager code handles exception via catch, report or ignore, and continue when dealing with handlers etc. I would think that same strategy applies here too.

Hi Jason,

Yes - that was also the behavior of the old property change methods. When the global LogManager calls itself readConfiguration it will silently ignore exceptions that might be raised.

I'm not too keen in specifying that exceptions raised by listeners will be silently dropped. We're in the process of reinitializing the configuration, so trying to log the exceptions through a Logger would have its own risks too. Reporting exceptions raised during initialization and configuration is an area where java.util.logging is clearly lacking.

For this patch - I think that our options are limited to the alternative: 1. propagate the exception 2. catch and silentlty drop the exception

What is the 'better' behavior (and I agree neither of the two are ideal) probably depends on what is the typical use case for these listeners. My assumption is that in most cases - there will be a single listener, in which case 1. is probably better. I haven't seen any bug complaining about how exceptions were handled in the previous implementation, though I have seen some complaining about swallowed exceptions...

That said - if there is consensus that 2. is better - I have no real objection except those stated above.

best regards,

-- daniel

...

Jason

----------------------------------------

...

Date: Fri, 12 Sep 2014 16:59:03 +0200 From: daniel.fuchs@oracle.com To: Alan.Bateman@oracle.com Subject: Re: RFR: 8043306 - Provide a replacement for the API that allowed to listen for LogManager configuration changes CC: core-libs-dev@openjdk.java.net

On 9/12/14 4:42 PM, Alan Bateman wrote:

...

On 12/09/2014 15:16, Daniel Fuchs wrote:

...

Thanks Alan!

I have updated the webrev with your suggestions:

http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.04/

-- daniel

A minor suggestion for readConfiguration is that "Any register configuration listeners .." might be a bit better than "The configuration listeners ...".

In addConfigurationListener there is a <br> between the two sentences that talk about exceptions, I don't know if you intended that. Done. I regenerated the webrev in place. http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.04/

-- daniel

...

Otherwise the javadoc looks good to me.

-Alan.

On 15/09/2014 17:03, Daniel Fuchs wrote:

:

Here is a new webrev - with updated test case:

http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.05/

I think this quite good now. Just on this wording: "If a listener terminates with an uncaught error or exception thenthe first exception that was raised will be propagated to the caller of readConfiguration after all listeners have been invoked." I think you can shorten this a bit by dropping "that was raised" from the sentence. Future maintainers might wonder why invokeConfigurationListeners takes a snapshot of the listeners so I think that deserves a comment in the code. -Alan

On 9/16/14 8:31 AM, Daniel Fuchs wrote:

Done.

http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.06

Looks good. Mandy

On 9/16/14 3:56 PM, Alan Bateman wrote:

...

On 15/09/2014 17:03, Daniel Fuchs wrote:

...

:

Here is a new webrev - with updated test case:

http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.05/

I think this quite good now.

Just on this wording:

"If a listener terminates with an uncaught error or exception thenthe first exception that was raised will be propagated to the caller of readConfiguration after all listeners have been invoked."

I think you can shorten this a bit by dropping "that was raised" from the sentence.

Future maintainers might wonder why invokeConfigurationListeners takes a snapshot of the listeners so I think that deserves a comment in the code.

-Alan

Hi, I updated the webrev at http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.07 * @implNote If more than one listener terminates with an uncaught error or * exception, an implementation may record the additional errors or * exceptions as {@linkplain Throwable#addSuppressed(java.lang.Throwable) * suppressed exceptions}. best regards, -- daniel On 16/09/14 17:31, Daniel Fuchs wrote:

Done.

http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.06

On 9/16/14 3:56 PM, Alan Bateman wrote:

...

On 15/09/2014 17:03, Daniel Fuchs wrote:

...

:

Here is a new webrev - with updated test case:

http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.05/

I think this quite good now.

Just on this wording:

"If a listener terminates with an uncaught error or exception thenthe first exception that was raised will be propagated to the caller of readConfiguration after all listeners have been invoked."

I think you can shorten this a bit by dropping "that was raised" from the sentence.

Future maintainers might wonder why invokeConfigurationListeners takes a snapshot of the listeners so I think that deserves a comment in the code.

-Alan

Hi Peter, On 9/12/14 9:14 AM, Peter Levart wrote:

Wouldn't it be cleaner to evaluate SM presence in the lambda wrapper which would always be constructed, like that:

Well - then we would always be storing the ACC, so maybe we could just always call AccessController.doPrivileged, whether there's a security manager or not. Do you think it would be cleaner? best regards, -- daniel On 9/12/14 9:14 AM, Peter Levart wrote:

On 09/11/2014 06:17 PM, Daniel Fuchs wrote:

...

On 9/10/14 1:25 PM, Alan Bateman wrote:

...

On 10/09/2014 10:49, Daniel Fuchs wrote:

...

Hi,

Please find below a proposed patch for

8043306 - Provide a replacement for the API that allowed to listen for LogManager configuration changes https://bugs.openjdk.java.net/browse/JDK-8043306

Proposed Patch: http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.00/

It would be nice if ConfigurationListener could be an interface. I realize this might mean looking again at the security concerns in this area again to see if we could get away without requiring a construction time permission check. Clearly if it could be an interface then it begs the question as to why it's just not a Runnable but that probably means yet more effort to figure out the right security and whether the access control context should be recorded when registering.

Given that both Paul & Alan have indicated that they would prefer to use interfaces for listeners, I have been working on a version that uses 'Runnable'.

Thanks to Paul & Alan for all the suggestions that went into this new patch (especially for refining the spec and suggesting a nice way to rewrite addConfigurationListener() using lambdas)!

http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.03/

best regards,

-- daniel

Hi Daniel,

Just a question about security and delayed execution...

If at the time the configuration listener is added to the LogManager, SecurityManager is not set, the listener will be invoked directly even if at time the listener is invoked, SM has been set. For example:

LogManager.getLogManager().addConfigurationListener(...); ... System.setSecurityManager(...); ... // then somewhere in privileged context... LogManager.getLogManager().readConfiguration(...);

Wouldn't it be cleaner to evaluate SM presence in the lambda wrapper which would always be constructed, like that:

public LogManager addConfigurationListener(Runnable listener) { final Runnable r = Objects.requireNonNull(listener); checkPermission(); final AccessControlContext acc = AccessController.getContext(); final Runnable pr = () -> { if (System.getSecurityManager() == null) { r.run(); } else { AccessController.doPrivileged((PrivilegedAction<Void>) () -> { r.run(); return null; }, acc); } }; // Will do nothing if already registered. listeners.putIfAbsent(r, pr); return this; }

Regards, Peter

On 09/12/2014 11:21 AM, Alan Bateman wrote:

On 12/09/2014 08:14, Peter Levart wrote:

...

:

Just a question about security and delayed execution...

If at the time the configuration listener is added to the LogManager, SecurityManager is not set, the listener will be invoked directly even if at time the listener is invoked, SM has been set. True but we typically don't get concerned about this. That is all bets are off if you allow untrusted code to run before setting the security manager.

I buy that, yes... Regards, Peter

So normally the assumption is that you are either running with or without a security manager, ignoring the case when it might be set or unset mid-flight. Also for the common case (running without a security manager) then you avoid needing to stash away the access control context as that has a number of side effects (Stanimir has picked up on this). Clearly there is a timing issue with code that runs early in the startup before the system class loader has fully initialized and the security manager set but great care has to be taken in those code paths.

-Alan

On 9/10/14 4:28 PM, Jason Mehrens wrote:

Daniel

I think you should be able to remove the 'other instanceof ConfigurationListener' branch in the ConfigurationListener.equals method. Should work the same with or without that branch.

Oh yes. I put it there just to avoid it being flagged by NetBeans: 'equals() method not checking type of its parameters' I guess it would be better to just return this == other; (which unfortunately doesn't remove the warning) but at least the intent would be clear. Thanks Jason! -- daniel

Jason

----------------------------------------

...

Date: Wed, 10 Sep 2014 11:49:51 +0200 From: daniel.fuchs@oracle.com To: core-libs-dev@openjdk.java.net Subject: RFR: 8043306 - Provide a replacement for the API that allowed to listen for LogManager configuration changes

Hi,

Please find below a proposed patch for

8043306 - Provide a replacement for the API that allowed to listen for LogManager configuration changes https://bugs.openjdk.java.net/browse/JDK-8043306

Proposed Patch: http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.00/

JDK-8029805 Removed LogManager addPropertyChangeListener and removePropertyChangeListener methods which were deprecated. These methods were deprecated in Java SE 8 and flagged for removal because they were problematic for modularity efforts. The idea was that an application needing to be informed of configuration changes could subclass LogManager and override readConfiguration.

The proposed patch adds to new methods to LogManager:

public void LogManager.addConfigurationListener(LogManager.ConfigurationListener listener); public void LogManager.removeConfigurationListener(LogManager.ConfigurationListener listener);

These methods can be used by those applications for which subclassing LogManager would be too impractical.

best regards,

-- daniel

Hi Peter, On 9/12/14 8:26 AM, Peter Levart wrote:

...

Oh yes. I put it there just to avoid it being flagged by NetBeans: 'equals() method not checking type of its parameters' I guess it would be better to just return this == other; (which unfortunately doesn't remove the warning)

What about using: super.equals(o) ? Does it remove the warning?

Well - the latest webrev uses Runnable and IdentityHashMap so the problem has solved itself ;-) (http://cr.openjdk.java.net/~dfuchs/webrev_8043306/webrev.03/) But for the record - no - super.equals(o) was my first implementation, which triggered the warning in the first place... Just to be clear: it was not a compiler warning - only an editor warning. best regards, -- daniel