RFR: JDK-8046565: Platform Logger API and Service

Wed Oct 14 11:20:58 UTC 2015

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.

> 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 believe LoggerFinder::getLoggerFinder() should be protected by
the permission. In the current design LoggerFinder::getLogger
is abstract - so the permission check is not enforced.
I had toyed with the idea of making getLogger final and make it
call a protected abstract demandLogger method instead, so that we
can enforce the permission check in getLogger. But I'm not sure
it is worth it given that you need a permission to get the
LoggerFinder object in the first place.
And it would be two more methods in the spec...

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

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

Yes. AbstractLoggerWrapper is a System.Logger and needs to
implement System.Logger.
PlatformLoggerBridge and ConfigurableLoggerBridge is bridging
code for PlatformLogger.
If the wrapped logger implements those then calls are directly
forwarded to the wrapped logger. Otherwise the default
mapping provided by AbstractLoggerWrapper is used.
This makes it possible to avoid transforming FINEST in FINER and
CONFIG in FINE when PlatformLogger is the front end and
java.util.logging is the default backend.

When component teams move from PlatformLogger to System.Logger
*they* will have to decide whether they want to downgrade
CONFIG to DEBUG or upgrade it to INFO.

> Are all *Logger and *LoggerWrapper types implementing Logger, PlatformLoggerBridge, ConfigurableLoggerBridge?  I might be missing it - I don’t see any non-configurable logger type implements only Logger, PlatformLoggerBridge.

All *LoggerWrapper extend AbstractLoggerWrapper.
All *Logger implementations that we provide implement those
interfaces as well - so that PlatformLogger calls can be tunneled
through to the concrete implementation returned by our default
provider implementation when there's no custom LoggerFinder
service plugged in.

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

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

sun.util.logger has all the new internal classes of the new service.
I could have put everything in sun.util.logging, but sun.util.logging
is already exported to several modules - so I preferred to keep it
separate. sun.util.logger is maybe not that a great name - but
sun.util.logging was already taken ;-)

Also I wanted to keep all restrictions that apply to sun.* packages
(protected by package access checks, flagged by IDEs if you try to
use them etc...). We don't want anybody start using sun.util.logger
directly (only java.logging needs it in order to implement the
internal service provider interface)

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.

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

> Similarly, sun.util.logging.internal.JdkLoggingProvider and sun.util.logger.JdkLoggerProvider - the package names are too close and they are in two different module.  Probably good to rename the classname - what about
> s/JdkLoggerProvider/SystemLoggerProvider/
> s/JdkLoggingProvider/LoggingProvider/

InternalLoggerProvider would be a better fit than SystemLoggerProvider.
LoggingProvider is OK I guess. Or InternalLoggerProviderImpl?

The Jdk prefix was here to indicate that these are JDK internals.
But obviously it didn't convey the meaning strongly enough.

WRT to package name there's a precedent because
sun.util.logging.resources is in java.logging module.
So this just adds sun.util.logging.internal next to it.
I am not a great fan of the name - but this is 1. internal and
2. I had no better idea. I couldn't put that class under
sun.util.logging.resources - could I?

What about InternalLoggerProvider for the service and
InternalLoggerProviderImpl for the impl provided by java.logging?

> I would have assumed that sun.util.logger.JdkLoggerProvider should be LoggerFinder.

It should not. That would also be confusing - I think. This is not
a LoggerFinder. It is an internal (sub)service provider interface
used by our internal default implementation of the LoggerFinder
service.

>  Is there any reason why it can’t extend LoggerFinder?

Yes, this is the internal service provider interface that
we use to figure out whether java.logging is present.
It's cleaner and safer than using reflection.
This is not an implementation of the LoggerFinder
service.

>  I think that would be cleaner and getJdkLogger is not needed.

We don't want java.logging to provide a service that
implements LoggerFinder.

At the moment, we throw a ServiceConfigurationError if ServiceLoader has
more than one implementation of the LoggerFinder service.
That is cleaner than having to instantiate all the services and then
decide which one to take and which one to discard.

>  Maybe DefaultLoggerFinder can be simplified.

Not sure there's much to do here.

> Have you considered moving out LoggerFinderLoader to a separate class?  This change adds many lines in System.java.

Could do it. That might not be a bad idea. LoggerFinderLoader should
be a name clear enough to convey the intent and avoid confusion.
So we could make it a package class instead of a static nested class.

> PlatformLogger - main reason to retain it is to minimize changes.  I wonder if changing it to an interface would help or not.  I’m still studying the sun.util.logger.* classes.

I had envisaged it. Then decided to keep it as a class.
This is currently also used by javafx modules - so I believe a 3 steps
strategy is better:

   1. step 1 => change PlatformLogger to log through System.Logger, but
      don't change it's public interface (except for deprecating
      setLevel), and minimize the behaviour changes when java.logging
      is present (the purpose of the *Bridge interfaces)

   2. step 2 => engage the component teams to plan for removing uses
      of PlatformLogger and use System.getLogger instead - possibly
      deprecate PlatformLogger at the time -

   3. step 3 => when all is stable and nobody uses PlatformLogger
      anymore, remove sun.util.logging package and all the bridging
      code from the classes in sun.util.logger.

I am not sure whether 1. 2. and 3. can/should be done in same
time frame. Only step 1. is in this JEP goals anyway.

>
> jdk/test/java/lang/System/Logger/custom/AccessSystemLogger.java
> jdk/test/java/lang/System/Logger/default/AccessSystemLogger.java
> jdk/test/java/lang/System/LoggerFinder/public/BaseLoggerFinderTest/AccessSystemLogger.java
> - they seem to be duplicated code to setup boot directory.  Worth putting them in logging test library?

I've been trying very hard to avoid using Libraries ;-).
Also I want to have access to the .class generated by jtreg, in order to
copy it in a directory that can be appended to the boot class path.
That's what the AccessSystemLogger.java does. It is first compiled
by jtreg - and is present in the classpath.
Then it is activated by the @run driver line - and it copies its
.class in a ./boot subdirectory
Then the @run main line will pick the ./boot directory and append
it to the bootclasspath.

> - what does the directory name "public" mean?  maybe just take that directory out?

oh - that might be a good idea too. It was meant to indicate that
the test in here might (possibly) serve to SQE because they don't
depend on any implementation specific classes.
I will take the directory out.

> jdk/test/java/lang/System/LoggerFinder/internal/backend/SystemClassLoader.java
> - copyright header

Argh. Thanks for catching that.

best regards,

-- daniel

>
> That’s all for today.
>
> Mandy
>
>