Adapter for Platform Logging (JEP 264) #232

nipafx · 2020-03-10T21:17:36Z

JEP 264: Platform Logging API and Service introduced a mechanism to Java 9 that allows third-party logging frameworks to handle JDK log messages (like those Swing produces; not JVM messages). This PR adapts org.slf4j.LoggerFactory and org.slf4j.Logger to the new interfaces java.lang.System.LoggerFinder and java.lang.System.Logger and - following the ServiceLoader documentation - creates a service provider-configuration file and a module declaration, so the integration works from class path and module path.

Closes SLF4J-442.

nipafx

Done reviewing myself. 😁 One more question: Where should this be documented? Should I put something together? (Oops, two questions.)

nipafx · 2020-03-10T21:21:23Z

slf4j-jdk-platform-logging/pom.xml

+    <version>2.0.0-alpha0</version>
+  </parent>
+
+  <artifactId>slf4j-jdk-platform-logging</artifactId>


I'm not sure what the best name would be for the artifact, module, package, and classes. 😊 At the moment they're a little all over the place using the terms jdk, platform, and system. I think a single term in all places would be better.

nipafx · 2020-03-10T21:22:32Z

slf4j-jdk-platform-logging/src/test/java/org/slf4j/jdk/SLF4JSystemLoggerTest.java

+        var loggerFinders = new ArrayList<System.LoggerFinder>();
+        // this fails when test is executed from the module path
+        // because the module declaration does not declare
+        // `uses System.LoggerFinder`


At the moment, only the class path integration is tested. Module path integration would fail here.

I observe the following error:

[ERROR] org.slf4j.jdk.SLF4JSystemLoggerTest.loggerLogsMessage Time elapsed: 0.003 s <<< ERROR!
java.util.ServiceConfigurationError: java.lang.System$LoggerFinder: module org.slf4j.jdk does not declare uses
at java.base/java.util.ServiceLoader.fail(ServiceLoader.java:589)
at java.base/java.util.ServiceLoader.checkCaller(ServiceLoader.java:575)
at java.base/java.util.ServiceLoader.(ServiceLoader.java:504)
at java.base/java.util.ServiceLoader.load(ServiceLoader.java:1687)
at org.slf4j.jdk@2.0.0-alpha4-SNAPSHOT/org.slf4j.jdk.SLF4JSystemLoggerTest.getSLF4JSystemLoggerFinder(SLF4JSystemLoggerTest.java:63)
at org.slf4j.jdk@2.0.0-alpha4-SNAPSHOT/org.slf4j.jdk.SLF4JSystemLoggerTest.getSLF4JSystemLogger(SLF4JSystemLoggerTest.java:83)
at org.slf4j.jdk@2.0.0-alpha4-SNAPSHOT/org.slf4j.jdk.SLF4JSystemLoggerTest.loggerLogsMessage(SLF4JSystemLoggerTest.java:49)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:567)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
...

nipafx · 2020-03-10T21:25:02Z

slf4j-jdk-platform-logging/src/main/resources/META-INF/MANIFEST.MF

+Bundle-SymbolicName: slf4j.ext
+Bundle-Name: slf4j-jdk-platform-logging
+Bundle-Vendor: SLF4J.ORG
+Bundle-RequiredExecutionEnvironment: JavaSE-1.8


is that the correct Bundle-RequiredExecutionEnvironment or should it be Java 9?

no Export-Package, right?

oops, forgot Import-Package

nipafx · 2020-03-10T21:27:09Z

slf4j-jdk-platform-logging/src/main/java/org/slf4j/jdk/SLF4JSystemLogger.java

+                INTERNAL_LOGGER.error(
+                        "SLF4J internal error: unknown log level {} passed to `isLoggable` (likely by the JDK).", level);
+                return true;


I was unsure how to such handle error cases. They're highly unlikely to occur (JDK API should be very stable), but if they do, IMHO SLF4J should not crash, misbehave as little as possible, but let the user know that there's a problem (hence the error logger). See below for two more such cases.

To avoid error cases from future JDK changes, you could compare to Level.getSeverity().
Basic idea is that you don't want to map all j.u.l. levels, you want to start from the available SLF4J levels and determine into which bracket the passed-in j.u.l. level fits. (FINER/FINEST etc. have different severities, despite being mapped to the same j.u.l. level.)

nipafx · 2020-03-10T21:27:32Z

slf4j-jdk-platform-logging/src/main/java/org/slf4j/jdk/SLF4JSystemLoggerFinder.java

+        // JEP 264[1], which introduced the Platform Logging API,
+        // contains the following note:
+        //
+        //  > An implementation of the LoggerFinder service should make it
+        //  > possible to distinguish system loggers (used by system classes
+        //  > from the Bootstrap Class Loader (BCL)) and application loggers
+        //  > (created by an application for its own usage). This distinction
+        //  > is important for platform security. The creator of a logger can
+        //  > pass the class or module for which the logger is created to the
+        //  > LoggerFinder so that the LoggerFinder can figure out which kind
+        //  > of logger to return.
+        //
+        // If backends support the distinction support this distinction and
+        // once `LoggerFactory`'s API is updated to forward a module, we
+        // should do that here.
+        //
+        // [1] https://openjdk.java.net/jeps/264


Does this make sense?

I'm not sure that I quite get the point: the System.Logger API is intended for JDK classes, not for applications, or is it?

The javadoc of System.LoggerFinder requires a slightly different thing:

* In addition, when a security manager is present, loggers provided to * system classes should not be directly configurable through the logging * backend without requiring permissions. * <br> * It is the responsibility of the provider of * the concrete {@code LoggerFinder} implementation to ensure that * these loggers are not configured by untrusted code without proper * permission checks, as configuration performed on such loggers usually * affects all applications in the same Java Runtime.

We don't know the logging backends and how they configure things. So we have to check with the SecurityManager for all cases, don't we? But this is already checked by the constructor of the System.LoggerFinder... I'm confused.

I'm not sure that I quite get the point: the System.Logger API is intended for JDK classes, not for applications, or is it?

It will be used by JDK classes, but it's implemented here and the quote addresses implementations, so I assume it applies to us. Didn't think to look at the JavaDoc, though, and it's not mentioned there, so maybe it's outdated?

Re security manager, it is indeed weird that System.LoggerFinder already does the check. Not sure what to do here.

nipafx · 2020-03-10T21:28:47Z

slf4j-jdk-platform-logging/src/main/java/module-info.java

+module org.slf4j.jdk {
+    requires org.slf4j;
+    provides java.lang.System.LoggerFinder
+            with org.slf4j.jdk.SLF4JSystemLoggerFinder;
+}


Should this be in src/java9? Would be more consistent but then again, the entire thing only works on Java 9+.

java.lang.System.LoggerFinder was introduced in Java9. Thus there would be no point in a multi-release jar.

nipafx · 2020-03-10T21:30:21Z

slf4j-jdk-platform-logging/pom.xml

+      <plugin>
+        <artifactId>maven-compiler-plugin</artifactId>
+        <configuration>
+          <release>11</release>


Which Java version do you want to target? Java 9 is the most lenient, but I wouldn't encourage use of outdated versions.

I would say 9 as the slf4j project currently requires Java 8 to at runtime and 9 at build time. Requiring 9 for this module seems consistent.

t1 · 2020-03-11T03:36:33Z

slf4j-jdk-platform-logging/src/main/java/org/slf4j/jdk/SLF4JSystemLogger.java

+            default:
+                INTERNAL_LOGGER.error(
+                        "SLF4J internal error: unknown log level {} passed to `isLoggable` (likely by the JDK).", level);
+                return true;
+        }


I would pull the default out of the switch block, so in the highly unlikely case that the JDK adds another enum value, the compiler will complain about the switch not being exhaustive :-)

Suggested change

default:

INTERNAL_LOGGER.error(

"SLF4J internal error: unknown log level {} passed to `isLoggable` (likely by the JDK).", level);

return true;

}

}

INTERNAL_LOGGER.error(

"SLF4J internal error: unknown log level {} passed to `isLoggable` (likely by the JDK).", level);

return true;

The compiler doesn't do that check for switch statements (your IDE may, though). Whether the code is in the default branch or not, makes no difference.

t1 · 2020-03-11T03:37:10Z

slf4j-jdk-platform-logging/src/main/java/org/slf4j/jdk/SLF4JSystemLogger.java

+            default:
+                INTERNAL_LOGGER.error(
+                        "SLF4J internal error: unknown log level {} passed to `log` (likely by the JDK).", level);
+        }


same as above:

Suggested change

default:

INTERNAL_LOGGER.error(

"SLF4J internal error: unknown log level {} passed to `log` (likely by the JDK).", level);

}

}

INTERNAL_LOGGER.error(

"SLF4J internal error: unknown log level {} passed to `log` (likely by the JDK).", level);

t1 · 2020-03-11T03:38:17Z

slf4j-jdk-platform-logging/src/main/java/org/slf4j/jdk/SLF4JSystemLogger.java

+            default:
+                INTERNAL_LOGGER.error(
+                        "SLF4J internal error: unknown log level {} passed to `log` (likely by the JDK).", level);
+        }


same as above:

Suggested change

default:

INTERNAL_LOGGER.error(

"SLF4J internal error: unknown log level {} passed to `log` (likely by the JDK).", level);

}

}

INTERNAL_LOGGER.error(

"SLF4J internal error: unknown log level {} passed to `log` (likely by the JDK).", level);

t1 · 2020-03-11T04:13:36Z

slf4j-jdk-platform-logging/src/main/java/org/slf4j/jdk/SLF4JSystemLoggerFinder.java

+        // JEP 264[1], which introduced the Platform Logging API,
+        // contains the following note:
+        //
+        //  > An implementation of the LoggerFinder service should make it
+        //  > possible to distinguish system loggers (used by system classes
+        //  > from the Bootstrap Class Loader (BCL)) and application loggers
+        //  > (created by an application for its own usage). This distinction
+        //  > is important for platform security. The creator of a logger can
+        //  > pass the class or module for which the logger is created to the
+        //  > LoggerFinder so that the LoggerFinder can figure out which kind
+        //  > of logger to return.
+        //
+        // If backends support the distinction support this distinction and
+        // once `LoggerFactory`'s API is updated to forward a module, we
+        // should do that here.
+        //
+        // [1] https://openjdk.java.net/jeps/264


I'm not sure that I quite get the point: the System.Logger API is intended for JDK classes, not for applications, or is it?

The javadoc of System.LoggerFinder requires a slightly different thing:

* In addition, when a security manager is present, loggers provided to * system classes should not be directly configurable through the logging * backend without requiring permissions. * <br> * It is the responsibility of the provider of * the concrete {@code LoggerFinder} implementation to ensure that * these loggers are not configured by untrusted code without proper * permission checks, as configuration performed on such loggers usually * affects all applications in the same Java Runtime.

We don't know the logging backends and how they configure things. So we have to check with the SecurityManager for all cases, don't we? But this is already checked by the constructor of the System.LoggerFinder... I'm confused.

t1 · 2020-03-11T04:29:05Z

slf4j-jdk-platform-logging/src/test/java/org/slf4j/jdk/SLF4JSystemLoggerTest.java

+
+    @Test
+    public void loggerFinderLoadedAsOnlyService() {
+        // this method asserts that there is exactly one `LoggerFinder` and its of the correct type


Is this an actual requirement? It could be over-specified. We only would have to change the getSLF4JSystemLoggerFinder method to filter the SLF4JSystemLoggerFinder (see below).

Yeah, that makes sense.

t1 · 2020-03-11T04:36:50Z

slf4j-jdk-platform-logging/src/test/java/org/slf4j/jdk/SLF4JSystemLoggerTest.java

+        var loggerFinders = new ArrayList<System.LoggerFinder>();
+        // this fails when test is executed from the module path
+        // because the module declaration does not declare
+        // `uses System.LoggerFinder`
+        ServiceLoader.load(System.LoggerFinder.class).forEach(loggerFinders::add);
+        assertEquals("There should be exactly one `LoggerFinder`.", 1, loggerFinders.size());
+        var loggerFinder = loggerFinders.get(0);
+        assertEquals("The `LoggerFinder` should be of type `SLF4JSystemLoggerFinder`.",
+                     SLF4JSystemLoggerFinder.class,
+                     loggerFinder.getClass());
+        return loggerFinder;


Maybe:

Suggested change

var loggerFinders = new ArrayList<System.LoggerFinder>();

// this fails when test is executed from the module path

// because the module declaration does not declare

// `uses System.LoggerFinder`

ServiceLoader.load(System.LoggerFinder.class).forEach(loggerFinders::add);

assertEquals("There should be exactly one `LoggerFinder`.", 1, loggerFinders.size());

var loggerFinder = loggerFinders.get(0);

assertEquals("The `LoggerFinder` should be of type `SLF4JSystemLoggerFinder`.",

SLF4JSystemLoggerFinder.class,

loggerFinder.getClass());

return loggerFinder;

// this fails when test is executed from the module path

// because the module declaration does not declare

// `uses System.LoggerFinder`

return ServiceLoader.load(System.LoggerFinder.class).stream()

.filter(finder → finder instanceof SLF4JSystemLoggerFinder)

.findAny()

.orElseThrow();

Note: this orElseThrow requires JDK 10.

ceki · 2021-08-10T17:45:50Z

Rebase attempt

ben-manes · 2021-08-29T04:23:15Z

In the last alpha, using this results in Illegal configuration-file syntax error. The META-INF/services was changed to an invalid line, package org.slf4j.jdk.platform.logging.SLF4JSystemLoggerFinder, whereas it should only be the FQCN per this PR. (I was trying to use this + slf4j-nop for testing)

nipafx · 2021-08-29T09:19:45Z

Indeed, that's not gonna work. :)

nipafx · 2021-08-29T09:25:08Z

Created #265.

Nicolai Parlog added 6 commits March 10, 2020 19:22

Create project structure

46c634a

Import proof of concept

4128670

Add service provider-configuration file for use on class path

ca3f942

Set up testing

019eec0

Polish

9066751

Ensure ResourceBundle::getString can't crash program

82b61d7

nipafx commented Mar 10, 2020

View reviewed changes

t1 approved these changes Mar 11, 2020

View reviewed changes

Relax service discovery in test

04bc268

KengoTODA mentioned this pull request Aug 8, 2020

SLF4J-498: Introduce GitHub Actions to run build in the PR #246

Closed

gunnarmorling mentioned this pull request Sep 18, 2020

Add logging support moditect/layrry#32

Open

ceki merged commit 3c4b29b into qos-ch:2.0 Aug 10, 2021

nipafx mentioned this pull request Aug 29, 2021

Fix platform logging service registration #265

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapter for Platform Logging (JEP 264) #232

Adapter for Platform Logging (JEP 264) #232

nipafx commented Mar 10, 2020

nipafx left a comment

nipafx Mar 10, 2020

nipafx Mar 10, 2020

ceki Aug 10, 2021 •

edited

nipafx Mar 10, 2020

nipafx Mar 10, 2020

joachim-durchholz-six Aug 10, 2021

nipafx Mar 10, 2020

t1 Mar 11, 2020

nipafx Mar 11, 2020

nipafx Mar 10, 2020

ceki Aug 10, 2021

nipafx Mar 10, 2020

ceki Aug 10, 2021

t1 Mar 11, 2020

nipafx Mar 11, 2020

t1 Mar 11, 2020

t1 Mar 11, 2020

t1 Mar 11, 2020

t1 Mar 11, 2020

nipafx Mar 11, 2020

t1 Mar 11, 2020

ceki commented Aug 10, 2021

ben-manes commented Aug 29, 2021

nipafx commented Aug 29, 2021

nipafx commented Aug 29, 2021

Adapter for Platform Logging (JEP 264) #232

Adapter for Platform Logging (JEP 264) #232

Conversation

nipafx commented Mar 10, 2020

nipafx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceki Aug 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceki commented Aug 10, 2021

ben-manes commented Aug 29, 2021

nipafx commented Aug 29, 2021

nipafx commented Aug 29, 2021

ceki Aug 10, 2021 •

edited