RFR: JDK-8046565: Platform Logger API and Service

[Mandy Chung]

> On Oct 14, 2015, at 4:20 AM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
> 
> Hi Mandy,
> 
> On 14/10/15 07:21, Mandy Chung wrote:
>> 
>>> On Oct 8, 2015, at 5:26 AM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>> 
>>> webrev:
>>> http://cr.openjdk.java.net/~dfuchs/8046565/proto.01/webrev.01/
>> 
>> System.Logger
>> 
>> Several log methods throws NPE if the level is legible and the parameter is null.  E.g.
>> * @throws NullPointerException if {@code level} is {@code null}, or if the level
>> *         is loggable and {@code msgSupplier} is {@code null}.
>> 
>> Should it throw NPE if the input parameter is null regardless of whether the level is loggable?
> 
> Yes probably. The reason it is like that is that it mimics
> what java.util.logging.Logger does (at least for message suppliers), but
> it is probably not a good idea to propagate this bad practice.
> 
> I will change System.Logger spec to mandate to always throw NPE
> if Supplier<String> is null - and ensure that the implementations
> we have follow the spec.


There are other methods having similar @throws that should be updated as well.

> 
>> RuntimePermission(“loggerFinder”) is required when
>> 1. a LoggerFinder provider is instantiated
>> 2. LoggerFinder::getLoggerFinder() is called
>> 3. LoggerFinder::getLogger is called
>> 
>> This is very specific to finding system logger (more than just the provider construction check).  It does seem worth defining a specific Permission type for it e.g. LoggerPermission.    Since LoggerFinder::getLogger requires permission check, does LoggerFinder::getLoggerFinder() need the explicit permission check?   We will need to consult with the security experts.
> 
> This is why it was first called "controlSystemLoggers" until
> somebody suggested to change it to "loggerFinder" ;-)
> But since it protects all access to LoggerFinder then naming
> it "loggerFinder" was actually a good suggestion :-)
> 
> When I started working on this I added a new LoggerPermission
> class similar to java.util.logging.Permission. I received
> strong pushback against it and it was suggeseted I used
> RuntimePermission instead - which suits me well.
> 

I slightly prefer a specific Permission type for this.

> I believe LoggerFinder::getLoggerFinder() should be protected by
> the permission.

This sounds reasonable to me.  

> :
>> LazyLoggers::getLazyLogger(String name, Class<?> caller)
>> - does it need permission check? it’s currently commented out
> 
> This class should already be protected by packageAccess checks and
> module access checks (since it's not in an exported package).
> So I don't think it needs the additional permission check
> which should have been performed (if needed) by upstream code.
> The commented code should obviously go away before pushing,
> I left it to elicit confirmation that it was OK to leave
> it out.

Take it out then.  I was not sure if you have it commented as a question to the reviewer or not.

> 
>> public class LoggerWrapper<L extends Logger> extends AbstractLoggerWrapper<L>
>>     implements Logger, PlatformLoggerBridge
>> 
>> - AbstractLoggerWrapper implements Logger, PlatformLoggerBridge, ConfigurableLoggerBridgem.  Is "implements Logger, PlatformLoggerBridge” needed at all?
> 

What I tried to point out is the class declaration.  Should LoggerWrapper simply extend AbstractLoggerWrapper (and drop the “implements….”)?  It’s minor and try to understand why you declare “implements…” explicitly that are the super interfaces of AbstractLoggerWrapper.


> :
> 
> 
>> SimpleConsoleLogger.Formatting
>> static final String FORMAT_PROP_KEY =
>>             "java.util.logging.SimpleFormatter.format";
>> - is this used when java.logging is not present?  It’s for the simple console logger formatting.  If so, the property name should be renamed to idk.logger.xxx?
> 
> Yes. It's used both by java.util.logging and the SimpleConsoleLogger.
> I think you did introduce this property - it was also used both by
> java.util.logging and the PlatformLogger proxy - I simply
> translated the code from the proxy to SimpleConsoleLogger.

Yes I did.  It’s a standard property that java.util.logging supports.  PlatformLogger proxy used it.  I think we should look into some simple mechanism for simple console configuration when java.logging is not present (a RFE to cover it).  It would be confusing to reuse “java.util.logging.” property to configure simple console in the absence of java.logging.

> SimpleConsoleLogger are also used as temporary loggers when
> LogManager is not initialized and has no custom configuration,
> just as PlatformLogger used to do. That functionality was
> simply translated from PlatformLogger to LazyLoggers/BootstrapLogger.

That’s the next big thing to review. 

> 
>> Can you explain the difference of sun.util.logging and sun.util.logger package? I think the reason to have two different packages is to make sure that sun.util.logger not used by other modules.  What other issue to put the classes in sun.util.logging - the existing package?  I don’t have any issue to create a new package but the name is hard to differentiate.   Maybe rename sun.util.logger to jdk.internal.logger if not in sun.util.logging?
> 
> sun.util.logging contains the 'legacy' PlatformLogger and all
> the code needed to bridge PlatformLogger.
> It can go away when we get to the point where nobody uses
> PlatformLogger any more.
> 
> :
> In an ideal situation then in the mid-long term sun.util.logging
> should hopefully disappear from java.base - and only
> sun.util.logger would remain.
> 

I’m glad that it’s a plan to migrate existing uses of sun.util.logging.PlatformLogger to System.Logger and ultimately PlatformLogger will go away.
 
> Do you think it would be preferable to move all the classes
> from sun.util.logger over to sun.util.logging?
> The distinction between new and legacy/bridge might be less
> clear, and there might be a little more risk of having modules
> which already read sun.util.logging foraging inside - but it
> could be worth it as it would mean one less new package…

I think it’s better to rename the new package to jdk.internal.logger to make it easier to distinct.

Mandy