Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapter for Platform Logging (JEP 264) #232

Merged
merged 7 commits into from
Aug 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
<module>slf4j-simple</module>
<module>slf4j-nop</module>
<module>slf4j-jdk14</module>
<module>slf4j-jdk-platform-logging</module>
<module>slf4j-log4j12</module>
<module>slf4j-ext</module>
<module>jcl-over-slf4j</module>
Expand All @@ -74,7 +75,7 @@
<module>osgi-over-slf4j</module>
<module>integration</module>
<module>slf4j-site</module>
<module>slf4j-migrator</module>
<module>slf4j-migrator</module>
</modules>

<dependencies>
Expand Down
53 changes: 53 additions & 0 deletions slf4j-jdk-platform-logging/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">

<modelVersion>4.0.0</modelVersion>

<parent>
<artifactId>slf4j-parent</artifactId>
<groupId>org.slf4j</groupId>
<version>2.0.0-alpha0</version>
</parent>

<artifactId>slf4j-jdk-platform-logging</artifactId>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<packaging>jar</packaging>
<name>SLF4J JDK Platform Logging Integration</name>
<description>Integrated SLF4J with the Platform Logging API added by JEP 264 in Java 9</description>

<url>http://www.slf4j.org</url>

<dependencies>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>

<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-simple</artifactId>
<version>2.0.0-alpha0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>${junit.version}</version>
<scope>test</scope>
</dependency>
</dependencies>

<build>
<plugins>
<!-- target Java 9+ -->
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<release>11</release>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</configuration>
</plugin>
</plugins>
</build>

</project>
5 changes: 5 additions & 0 deletions slf4j-jdk-platform-logging/src/main/java/module-info.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module org.slf4j.jdk {
requires org.slf4j;
provides java.lang.System.LoggerFinder
with org.slf4j.jdk.SLF4JSystemLoggerFinder;
}
Comment on lines +1 to +5
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
package org.slf4j.jdk;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.MissingResourceException;
import java.util.ResourceBundle;

import static java.util.Objects.requireNonNull;

/**
* Adapts {@link Logger} to {@link System.Logger}.
*/
class SLF4JSystemLogger implements System.Logger {

private static final Logger INTERNAL_LOGGER = LoggerFactory.getLogger(SLF4JSystemLogger.class);

private final Logger logger;

public SLF4JSystemLogger(Logger logger) {
this.logger = requireNonNull(logger);
}

@Override
public String getName() {
return logger.getName();
}

@Override
public boolean isLoggable(Level level) {
switch(level) {
case ALL:
// fall-through intended because `ALL` is loggable if the
// lowest level is enabled
case TRACE:
return logger.isTraceEnabled();
case DEBUG:
return logger.isDebugEnabled();
case INFO:
return logger.isInfoEnabled();
case WARNING:
return logger.isWarnEnabled();
case ERROR:
return logger.isErrorEnabled();
case OFF:
// all logging is disabled if the highest level is disabled
return !logger.isErrorEnabled();
default:
INTERNAL_LOGGER.error(
"SLF4J internal error: unknown log level {} passed to `isLoggable` (likely by the JDK).", level);
return true;
Comment on lines +49 to +51
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
Comment on lines +48 to +52
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

@Override
public void log(Level level, ResourceBundle bundle, String msg, Throwable thrown) {
String message = getResourceStringOrMessage(bundle, msg);
switch(level) {
case ALL:
// fall-through intended because a message is visible on all log levels
// if it is logged on the lowest level
case TRACE:
logger.trace(message, thrown);
break;
case DEBUG:
logger.debug(message, thrown);
break;
case INFO:
logger.info(message, thrown);
break;
case WARNING:
logger.warn(message, thrown);
break;
case ERROR:
logger.error(message, thrown);
break;
case OFF:
// don't do anything for a message on level `OFF`
break;
default:
INTERNAL_LOGGER.error(
"SLF4J internal error: unknown log level {} passed to `log` (likely by the JDK).", level);
}
Comment on lines +80 to +83
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

}

@Override
public void log(Level level, ResourceBundle bundle, String format, Object... params) {
String message = getResourceStringOrMessage(bundle, format);
switch(level) {
case ALL:
// fall-through intended because a message is visible on all log levels
// if it is logged on the lowest level
case TRACE:
logger.trace(message, params);
break;
case DEBUG:
logger.debug(message, params);
break;
case INFO:
logger.info(message, params);
break;
case WARNING:
logger.warn(message, params);
break;
case ERROR:
logger.error(message, params);
break;
case OFF:
// don't do anything for a message on level `OFF`
break;
default:
INTERNAL_LOGGER.error(
"SLF4J internal error: unknown log level {} passed to `log` (likely by the JDK).", level);
}
Comment on lines +111 to +114
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

}

private static String getResourceStringOrMessage(ResourceBundle bundle, String msg) {
if (bundle == null || msg == null)
return msg;
// ResourceBundle::getString throws:
//
// * NullPointerException for null keys
// * ClassCastException if the message is no string
// * MissingResourceException if there is no message for the key
//
// Handle all of these cases here to avoid log-related exceptions from crashing the JVM.
try {
return bundle.getString(msg);
} catch (MissingResourceException ex) {
return msg;
} catch (ClassCastException ex) {
return bundle.getObject(msg).toString();
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package org.slf4j.jdk;

import org.slf4j.LoggerFactory;

/**
* Uses SLF4J's {@link LoggerFactory#getLogger(String)} to get a logger
* that is adapted for {@link System.Logger}.
*/
public class SLF4JSystemLoggerFinder extends System.LoggerFinder {

@Override
public System.Logger getLogger(String name, Module module) {
// 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
Comment on lines +13 to +29
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this make sense?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var slf4JLogger = LoggerFactory.getLogger(name);
return new SLF4JSystemLogger(slf4JLogger);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Implementation-Title: slf4j-ext
Bundle-ManifestVersion: 2
Bundle-SymbolicName: slf4j.ext
Bundle-Name: slf4j-jdk-platform-logging
Bundle-Vendor: SLF4J.ORG
Bundle-RequiredExecutionEnvironment: JavaSE-1.8
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • is that the correct Bundle-RequiredExecutionEnvironment or should it be Java 9?
  • no Export-Package, right?
  • oops, forgot Import-Package

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
org.slf4j.jdk.SLF4JSystemLoggerFinder
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package org.slf4j.jdk;

import org.junit.After;
import org.junit.Before;
import org.junit.Test;

import java.io.ByteArrayOutputStream;
import java.io.PrintStream;
import java.util.ServiceLoader;

import static org.junit.Assert.*;

public class SLF4JSystemLoggerTest {

private static final PrintStream ERROR_OUT = System.err;
private static final ByteArrayOutputStream OUTPUT = new ByteArrayOutputStream();
private static final PrintStream ERROR_OUT_REPLACEMENT = new PrintStream(OUTPUT);

@Before
public void setUp() {
System.setErr(ERROR_OUT_REPLACEMENT);
}

@After
public void tearDown() {
System.setErr(ERROR_OUT);
}

@Test
public void loggerFinderLoadedAsService() {
// this method asserts that a `LoggerFinder` of the correct type was loaded
getSLF4JSystemLoggerFinder();
}

@Test
public void loggerFinderReturnsLogger() {
var logger = getSLF4JSystemLogger();
assertNotNull("LoggerFinder must return logger", logger);
}

@Test
public void loggerLogsMessage() {
var logger = getSLF4JSystemLogger();
var message = "Test system logging!";
logger.log(System.Logger.Level.INFO, message);

String output = getOutput();

assertTrue("Captured output should contain logged message.", output.contains(message));
}

private static System.LoggerFinder getSLF4JSystemLoggerFinder() {
// this fails when test is executed from the module path
// because the module declaration does not declare
// `uses System.LoggerFinder`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ceki ceki Aug 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return ServiceLoader.load(System.LoggerFinder.class).stream()
.filter(finderProvider -> SLF4JSystemLoggerFinder.class.isAssignableFrom(finderProvider.type()))
.findAny()
.map(ServiceLoader.Provider::get)
.orElseThrow();
}

private static System.Logger getSLF4JSystemLogger() {
var loggerFinder = getSLF4JSystemLoggerFinder();
return loggerFinder.getLogger("TestLogger", SLF4JSystemLoggerTest.class.getModule());
}

private static String getOutput() {
ERROR_OUT_REPLACEMENT.flush();
return OUTPUT.toString();
}

}