Revisit Commons Logging vs java.util.logging vs SLF4J vs Log4j 2 [SPR-14512]

[spring-projects-issues]

Juergen Hoeller opened SPR-14512 and commented

Following up on recent discussions about the good old Commons Logging vs SLF4J conflict (a manual exclusion needed in the POM etc) and the recent attractiveness of Log4J 2's logging API:

Let's revisit our logging API arrangement. A straight migration to SLF4J doesn't seem to be a worthwhile option from a 2017 perspective, in particular since Log4J 2 actually has a nicer API. However, a straight migration to Log4J 2 isn't ideal either since so many people use SLF4J at the moment. And of course, none of those "simple" migration options addresses the problem of backwards compatibility with Spring extensions which happen to use our exposed protected logger reference... which is a hard Commons Logging reference in pre-compiled binaries.

Beyond a straight migration, we may also introduce our own logging abstraction (sigh). Alternatively, we could ship a variant of the Commons Logging API in our own spring-core.jar, controlling the target binding through our own adaptive setup logic: This is essentially what the JCL-over-SLF4J adapter does, and it seems to work fine there. As long as we're able to bind to java.util.logging, SLF4J and Log4J 2 without too much hassle (ideally through autodetection in the classpath), it might be a sweet compromise for the backwards compatibility issue.

Of course, it would also be nice to have a richer logging API available for our internal purposes. Log4J 2 looks quite attractive there but we might not want to require it at runtime, in particular for SLF4J users or java.util.logging users in application server environments. We could also use a facade of our own internally or augment our variant of Commons Logging accordingly.

---

Affects: 5.0 M1

Issue Links:

• Revisit subsystem-oriented logging [SPR-8122] #12774 Revisit subsystem-oriented logging
• Migrate logger from commons-logging to slf4j (remove commons-logging dependency) [SPR-6607] #11273 Migrate logger from commons-logging to slf4j (remove commons-logging dependency)
• DATACMNS-1040 Remove exclusion for Commons Logging
• spring-core source jar contains two copies of source code [SPR-15462] #20022 spring-core source jar contains two copies of source code
• Consider migrating from Commons-Logging to SLF4j [SPR-5327] #10000 Consider migrating from Commons-Logging to SLF4j
• commons-logging LogFactory and String arguments [SPR-16226] #20774 commons-logging LogFactory and String arguments
• Fix compilation warning on JDK 9: use deprecated method LogRecord.setMillis [SPR-15453] #20014 Fix compilation warning on JDK 9: use deprecated method LogRecord.setMillis
• Remove duplicate commons logging classes from spring-jcl [SPR-16062] #20611 Remove duplicate commons logging classes from spring-jcl
• spring-jcl routes logging inefficiently against SLF4J with log4j-to-slf4j setup [SPR-17586] #22118 spring-jcl routes logging inefficiently against SLF4J with log4j-to-slf4j setup

0 votes, 11 watchers

[spring-projects-issues]

Marten Deinum commented

Related: #11273, #10000

I really hope Spring won't come up with YALA (Yet Another Logging Abstraction) as there are already many of those (this xkcd comic comes to mind). Also auto-detection of loggers isn't that what made a mess out of Commons Logging in the first place?!.

[spring-projects-issues]

Juergen Hoeller commented

From my perspective, Commons Logging just has a bad reputation due to its rather naive early versions, but also due to the personal agenda of SLF4J's original creator. Anyway, leaving history aside, Commons Logging actually works quite well in its later incarnations as long as its API as well as the logging provider are shipped with the application. Runtime detection is only an issue if the Commons Logging API itself is shipped in common class loaders (e.g. Tomcat level), with the actual logging provider living in a more specific class loader (e.g. WAR level). That wouldn't work with SLF4J either but nobody even attempted it there so nobody perceived it as a design flaw. In all other respects, Commons Logging is not really different from what many other frameworks do in terms of runtime detection, including us all over the place against so many third-party providers. I mean, take Boot...

As for this ticket, it is primarily about improving usage scenarios:

• no manual exclude of standard Commons Logging necessary for SLF4J users
• straightforward setup with Log4J 2 (ideally not requiring SLF4J or any special Commons Logging bridge on the classpath)
• automatic defaulting to java.util.logging if none of the above is present at runtime (primarily for app server deployments)

And as a bonus, we'd love a richer logging API available to us internally. Log4J 2's API looks most attractive there among the existing options. A custom Supplier-based arrangement for log messages, efficiently bound to Java 8 method references, looks quite attractive as well.

So with those goals taken into account, I'm not quite sure what the solution will be. The only friction-free outcome might be a logging arrangement of our own, possibly with variants of the Commons Logging API classes shipped in spring-core.jar for backwards-compatibility purposes (similar to how we do it for the AOP Alliance interfaces already) but with the logging provider logic implemented by ourselves, similar to how SLF4J does it.

[spring-projects-issues]

Juergen Hoeller commented

A related discussion, including what libraries should be doing: https://www.reddit.com/r/java/comments/5mqkq5/why_you_should_prefer_the_log4j2_api_to_slf4j_in/

[spring-projects-issues]

Phil Webb commented

One sensible step in 5.0 might be to make all Log log members private. We seem to remember that we have a few protected ones at the moment which causes binary compatibility issues. A major release would be a perfect time to hide those so that logging concerns don't leak.

[spring-projects-issues]

Juergen Hoeller commented

We don't accidentally expose protected Log fields, we do so across implementation hierarchies so that they always log under the concrete class name. So they automatically leak to custom subclasses as well, for plenty of class hierarchies in the core container as well as in the web framework... extended by other portfolio projects as well application-specific subclasses.

If we turned all Log fields private, we'd break binary compatibility for all such subclasses and make it unnecessarily hard to have a logging-under-concrete-class-name arrangement. It would help for future log library changes, but at this point I'm rather concerned about the immediate breakages for existing 4.x-based extensions, and about not logging under abstract base class categories.

[spring-projects-issues]

Juergen Hoeller commented

I've opted for the approach with an embedded org.apache.commons.logging.Log interface in spring-core, combined with a custom org.apache.commons.logging.LogFactory implementation with a detection switch between Log4J 2.x, SLF4J 1.7, and java.util.logging as a fallback. This means that any typical Commons Logging LogFactory.getLog(Class/String) keeps working fine in source and binary compatible form, with no need for an external jar.

This implementation does not support any of Commons Logging's flexible configuration. In that sense, it works as a replacement for the Log4j 2 Commons Logging bridge as well as the JCL-over-SLF4J bridge, both of which become irrelevant for Spring-based setups as a consequence (with no need for manual excludes of the standard Commons Logging API jar anymore either). Furthermore, for simple setups without an external logging provider, Spring does not require any extra jar on the classpath anymore since our embedded log factory automatically switches to java.util.logging in such a scenario.

The Log4j 2 and SLF4J bridges will still work when on the classpath, with either such an external bridge or our spring-core Commons Logging bridge being picked up depending on classpath order... but since they're leading to equivalent delegation at runtime, this doesn't really matter. Nevertheless, the whole point of this exercise is to remove the need for such external bridges (and the corresponding standard Commons Logging API exclude), so let's drop them wherever we can and streamline our project setups and application starters accordingly.

As a side note, our new java.util.logging adapter is more efficient than the good old Commons Logging Jdk14Logger implementation since it resolves the source location lazily (like the JBoss Logging JUL adapter does). This won't make a difference for default JUL setup which always includes that class and method location. However, if the log output is configured to avoid the source location, we skip its expensive resolution completely now, just like Log4J and SLF4J do.

This arrangement works fine for me locally, so I'll commit it to master tonight. Let's see what the platform integration tests say...

[spring-projects-issues]

Phil Webb commented

Will this cause classloader issues if an app server provide a version of the commons-logging jar? The problem I can see with this approach is there's now no way to exclude org.apache.commons.logging.Log. If the parent classloader provides org.apache.commons.logging.Log but we've got our own copy in spring-core I think we might end up with cast exceptions.

We're also very likely to end up in the situation where two copies of org.apache.commons.logging.* classes end up on the classpath. It's highly likely that jcl-over-slf4j ends up on the classpath.

I know the existing commons-logging dependency isn't ideal, but I'm worried that copies of classes in spring-core might cause more issues. The current situation is at least well understood and for Boot most users not even something they need to worry about.

[spring-projects-issues]

Phil Webb commented

/cc Andy Wilkinson since he's been involved in the Spring Boot LogggingSystem pain.

[spring-projects-issues]

Juergen Hoeller commented

As I was hinting at above, the Log4j 2 and SLF4J bridges will still work when on the classpath: It's then just a matter of classpath order, but since our own bridge and their bridges lead to the same end delegate, the outcome doesn't really matter. And in the end, the whole point is that those bridges shouldn't be on the classpath anymore. Since no libraries bring those bridges onto the classpath themselves, this is something Boot 2.0 can control in its project starter setups.

As for application server scenarios, application-local classes always override server-level classes. From our perspective, we always want our delegate to be used, never the application server's. There can't be any cast exceptions either if our local variant is consistently used. It's certainly worse if somebody forgets to declare a Commons Logging dependency and unintentionally ends up using a server-level Commons Logging API with a locally embedded logging provider; that leads to exactly the class-cast pain that Commons Logging is so famous for. As far as I can see, our embedded setup avoids any such potential.

That said, let's see what we encounter in integration tests. I'd at least like to try this approach and see how far we get in our CI builds.

[spring-projects-issues]

Andy Wilkinson commented

As I was reading the description of the changes, the same concern that Phil's described came to mind. I'm not sure that we'll end up with class cast exceptions as the parent class loader's version should be preferred, but it does sound like the new arrangement will be very sensitive to a second copy of the org.apache.commons.logging.* classes appearing on the class path (either in a parent class loader or in the same class loader) and the order in which they appear. If spring-core appears first then its version should be preferred and its detection logic will be used, but if the other copy appears first then its detection logic will be used instead. That could lead to an unexpected change in logging back end when a new dependency is added that happens to pull in commons logging.

[spring-projects-issues]

Juergen Hoeller commented

Talking to Phil on Skype, I'll push this tonight and we can see how it goes, rolling it back if necessary.

Andy, I've been spending quite a bit of time looking at third-party projects around us, and as far as I'm aware, as of Spring 5, we do not support any that bring in Commons Logging anymore (e.g. no Velocity anymore). So this shouldn't be a problem in practice, and if it is for some esoteric case, we can work out a solution for those affected.

My take is simply that we need to first-class align with Log4J 2 as well as SLF4J out there, and equally well with java.util.logging. Requiring extra bridge jars for those, even with a manual exclude necessary, is unnecessary pain for the 99% of users out there falling into those three categories. That's what need to address.

At least, Log4J 1.x is EOL now, as are other historic Commons Logging backends. We literally only have three logging API targets to deal with in 2017.

[spring-projects-issues]

Juergen Hoeller commented

A deep-dive brought forward that we technically still got three supported optional dependencies which depend on Commons Logging:

• Apache HttpClient
• Castor XML
• HtmlUnit

All of them seem to work fine against our embedded variant, but of course all three declare a dependency on Commons Logging in their POM and therefore require an explicit exclude in order to arrive at a clean classpath. That's no different from using them with the regular Log4J 2 Commons Logging adapter or with JCL-over-SLF4J, of course, in which case they require the same exclude. And for java.util.logging, both our adapter and the standard Commons Logging factory fall back to it at runtime anyway, so it doesn't really matter which one gets picked up. We should document the recommended exclude for those three libraries though.

[spring-projects-issues]

Andy Wilkinson commented

In aligning Spring Boot with this change, it's become apparent that we also need to consider dependencies upon org.slf4j:jcl-over-slf4j. It is quite widely used, including all of our Spring Data projects. The build for Spring Boot's starters is configured to fail when there are multiple copies of the same class on the classpath. The build for every Spring Data-related starter failed due to transitive dependencies upon jcl-over-slf4j and spring-core. Given Spring Data Kay's Spring Framework 5 base, it would seem to be reasonable for the Spring Data projects to drop their dependency upon org.slf4j:jcl-over-slf4j in the Kay release train.

[spring-projects-issues]

Juergen Hoeller commented

Oliver Drotbohm, since you've been the original trigger for this change, I suppose dropping jcl-over-slf4j (and the corresponding Commons Logging exclude) from Spring Data Kay is ok with you?

[spring-projects-issues]

Andy Wilkinson commented

This change has caused an interesting failure in Spring Boot's Maven plugin (it depends upon spring-core) when using Maven 3.2. The failure is:

Caused by: java.lang.NoClassDefFoundError: org/slf4j/spi/LocationAwareLogger
	at org.apache.commons.logging.LogFactory$Slf4jLog.<init>(LogFactory.java:274)
	at org.apache.commons.logging.LogFactory$Slf4jDelegate.createLog(LogFactory.java:150)
	at org.apache.commons.logging.LogFactory.getLog(LogFactory.java:95)
	at org.apache.commons.logging.LogFactory.getLog(LogFactory.java:83)
	at org.springframework.core.io.support.SpringFactoriesLoader.<clinit>(SpringFactoriesLoader.java:59)
	at org.springframework.boot.loader.tools.Repackager.getLayoutFactory(Repackager.java:207)
	at org.springframework.boot.loader.tools.Repackager.repackage(Repackager.java:173)
	at org.springframework.boot.maven.RepackageMojo.repackage(RepackageMojo.java:212)
	at org.springframework.boot.maven.RepackageMojo.execute(RepackageMojo.java:199)
	at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:132)
	... 20 more
Caused by: java.lang.ClassNotFoundException: org.slf4j.spi.LocationAwareLogger
	at org.codehaus.plexus.classworlds.strategy.SelfFirstStrategy.loadClass(SelfFirstStrategy.java:50)
	at org.codehaus.plexus.classworlds.realm.ClassRealm.unsynchronizedLoadClass(ClassRealm.java:259)
	at org.codehaus.plexus.classworlds.realm.ClassRealm.loadClass(ClassRealm.java:235)
	at org.codehaus.plexus.classworlds.realm.ClassRealm.loadClass(ClassRealm.java:227)
	... 30 more

As far as I can tell, the Plexus class loader allows org.slf4j.LoggerFactory to be loaded, but prevents access to org.slf4j.spi.LocationAwareLogger. I think that's what's signified by this debug output:

[DEBUG]   Imported: org.slf4j.* < plexus.core

Could the check in LogFactory for SLF4J being on the classpath be tightened up a bit?