RFR: JDK-8046565: Platform Logger API and Service (original) (raw)

Daniel Fuchs daniel.fuchs at oracle.com
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 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 extends AbstractLoggerWrapper 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 FORMATPROPKEY = "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

More information about the core-libs-dev mailing list