I really love it :)

Very cool! How about some unit tests?

@mattbishop unit tests has been added 6fb91da d2c0254

Also can you send templated strings like:
"trace msg {}", echo() ?

@mattbishop This port is compatible with the default api. New jdk 8 methods has been add as default means no interface break has been introduced.

GJ

SLF4J needs to support JDK 1.5 whereas the Supplier interface was added in JDK 1.8. Even if the Supplier interface was available in JDK 1.5, we can't modify the Logger interface in order to preserve backward binary compatibility.

Having said that, this contribution increases the pressure to modernize the SLF4J API, which is welcome.

@ceki the jdk 1.8 support has been introduced as a separate module like jdk 1.4 support and not part of current and default api interfaces. It is import to note that introducing this module does not break existing libraries and interfaces as new methods has been add as default implementation.

I see. What would happen if some application A depended on two modules say B and C with module B depending on slf4j-api.jar and module C depending on slf4j-jdk18.jar? To make the problem easier, further assume that the user managed to include only slf4j-jdk18.jar in her class path (no slf4j-api.jar). Can classes in module B be loaded and run OK?

I personally cannot answer the above question with certainty. Can you?

@ceki what you describe here is a general problem that can be raised even with different version of the same libraries. As said, this module don't break compatibility, and one of the sample solution is to exclude the default slf4j-api because it is fully covered by slf4j-jdk18.jar

That does not solve the main problem (frozen Logger interface), but according to java documentation:

However, the compiler will treat any interface meeting the definition of a functional interface as a functional interface regardless of whether or not a FunctionalInterface annotation is present on the interface declaration.

In other words I'm basically certain that if you were to write:

public interface StringSupplier {
   public String get();
}

That code would compile with java5 and would enable you to type lambdas in a java8 client.

But because of the Logger interface freeze that's not really helpful sadly.

I would also add that if a jdk-1.8 interface is added, that could solve to a degree future problems with extensibility of the Logger API thanks to the new java8 feature of default methods in interfaces. That would enable adding new methods to the interface in the future without breaking compatibility.

@emmanueltouzery the main feature that has been add ( as new module like jdk-1.4 support) is the default non-intrusive supplier methods params.

@ouertani of course, and that's the reason why I'm interested in this bug. I'd like the feature to be merged, whether it's implemented through a jdk-1.8 module or through some extension of the existing 1.5 code.

Yes !! thanks @ouertani for this fix

Nice job @ouertani , works like a charm, I really enjoy the lazy evaluation feature, that's why I have merged your PR in my repo and switched to slf4j-api-jdk18, https://github.com/BenRhouma/slf4j

the lazy evaluation is really helpful to enhance performance when running in prod, especially when we use an algorithm full of trace or debug logs.

Main difference between SLF4j and SLF4j-JDK18

Logger.info  (() ->"info " + echo());
Logger.trace(() ->"trace " + echo());

public static  String echo (){
     Logger.info("echo invoked");
     return "echo body";
}

output using slf4j-api and trace is disabled

17:16:56.403 [main] INFO - echo invoked
17:16:56.406 [main] INFO - info echo body
17:16:56.406 [main] INFO - echo invoked <-- method echo is invoked even if trace is disabled

output using slf4j-jdk18 and trace is disabled

17:17:57.205 [main] INFO - echo invoked
17:17:57.207 [main] INFO - info echo body

+1 This is very valuable functionality. Would love to see this in master. Thanks @ouertani!

Indeed very good functionality. +1

This doesn't appear to support parameterized strings, which is pretty limiting. It's also very difficult to tell what's actually different since the an entire module was copied, resulting in a lot of duplication.

@nathansgreen I saw you proposal of lambda support in slf4j mailing list. Can you explain what are the pros of your solution? Maybe you can submit PR with your solution so author of current PR might close it if your solution is better or has better feedback from slf4j maintainers.

@garagatyi I was hoping for some feedback/constructive criticism before opening a PR. I don't know what features people really want with a lambda-enabled API, so the PR history could get pretty convoluted. I also have two separate branches with two different implementations. Either could be used independently or they could be merged, but I won't know what needs to happen just thinking about it all by myself. Also, I haven't had time yet, but my plan was to convert a bunch of code from Spring Framework to the new API to get a feel for how it looks in a decent-sized project with decent code quality.

Pros:

1. No code is copied. The existing PR has a lot of copied code and I think that's unnecessary.
2. The regression test suite provides strong confidence in binary compatibility. SLF4J has long emphasized compatibility, so I don't feel the need to argue for its value.
3. It's easy to use for most common cases. I rarely need to make more than two expensive calls for a parameterized logging statement, so that's covered.
4. Correctness. The following example is possible and I've seen it in the wild a handful of times:

if (logger.isInfoEnabled()) {
    logger.debug("Message {} with {} really {} expensive {} arguments", a.eval(), b.eval(), c.eval(), d.eval());
}

If lambdas are first-class in the logging API, this problem goes away. (It ought to be caught in code review, but mistakes happen.)

The other thing I was hoping to put a bit of time on was more low-level analysis. Performance testing and digging into the compiler-generated bytecodes would provide the data needed to document the new API and when/if the old API is more appropriate.

The separate module approach (slf4j-api-jdk18) would be a nightmare for both the maintainers of SLF4J and for its users.

1. Any bug fix in slf4j-api would need to be ported to slf4j-api-jdk18 and vice versa.

2. Even assuming that slf4j-api-jdk18 is binary compatible with slf4j-api, users would need to exclude slf4j-api and replace it with slf4j-api-jdk18 in their build files. We just can't impose such a change on millions of unsuspecting users.

As I have indicated repeatedly, while lambda methods in the Logger interface are desirable, in my opinion, a separate module such as slf4j-api-jdk18 is not the way to get there.

@ouertani Is the separate module approach motivated by not wishing to impose Java 8 on users allowing them to stick with slf4j-api which requires only Java 5?

This is from http://mailman.qos.ch/pipermail/slf4j-dev/2016-December/004593.html

Hello all,

Given that JDK 7 earlier are used by over 50% of Java projects, I do not
think we can force users to upgrade to Java 8. Thus, SLF4J is unlikely
to bump its JDK requirement to version 8 for yet some time.

On the bright side, there is a way for org.slf4j.Logger to support Java8
lambdas, or just lambdas, without requiring Java 8. As stated in [1],
the compiler will treat any interface meeting the definition of a
functional interface as a functional interface regardless of whether or
not a @FunctionalInterface annotation is present on the interface
declaration.

This means that we can create a Supplier interface such as

public interface Supplier<T> {
     T get();
}

and then add the following method to org.slf4j.Logger interface.

package org.slf4j;

public interface Logger {
   ....
   public void debug(Supplier<T> supplier);
   ....
}

This will add lambda support without requiring Java 8 support at build time.

The downside of this approach is that implementations of the
org.slf4j.Logger interface as found in logback and modules such as
slf4j-simple, slf4j-log412, slf4j-jdk14 will need to be updated. Given
that the number of such implementations is limited, I think the
requirement is quite acceptable.

Your comments are welcome,

Ceki's proposal sounds solid. Looks like a good solution for everyone.
The downside of adding quite a few new methods to an interface is obviously bad, but could potentially be managed. I guess that means incrementing the major version of slf4j to 2.0.0, right?

worth noting that I think that suppliers should work with message format

log.debug("foo {} bar {}", foo::toString, () -> foo.getBar().toString );

and I've noticed implementing this wrapper myself that java doesn't really (the compiler warns lots...) like a varargs of suppliers, so I ended up making an interface is so far

https://bitbucket.org/xenworks/logging/src/9d1bb594f790ae4bc77e3e7ecd994c0026e12ab4/src/main/java/com/xenoterracide/logging/Logger.java?at=master&fileviewer=file-view-default

I will probably end up licensing this under the apache 2, haven't made it official though as I'm debating on certain aspects of said license. Also it does need some improvements. feedback on what I've done, patches, etc, welcome. If someone wants me to add it to maven central I can probably do that.

ceki's proposal (see above) sounds eminently workable, but I would like to tweak it a bit. Basing the lambda-based logging on the Supplier interface he defines can cause compilation problems. To wit, if any operation inside the lambda declares a checked exception, such as:

log.debug(() -> foo.getBar())

where

class Foo {
    ...
    Bar getBar() throws IOException {
        return barGetter.retrieveFor(this);
    }
    ...
}

Then the code will fail to compile, since Supplier.get() does not throw IOException.

It turns out that there is an easy fix, but it has to be built into the design though. First, redefine the Supplier interface as follows:

public interface Supplier<T, E extends Exception> {
     T get() throws E;
}

Then, inside the Logger interface, define each method that uses Supplier as follows:

public interface Logger {
    ...
    <E extends Exception> void trace(Supplier<String, E> message) throws E;
    <E extends Exception> void debug(Supplier<String, E> message) throws E;
    <E extends Exception> void info(Supplier<String, E> message) throws E;
    <E extends Exception> void warn(Supplier<String, E> message) throws E;
    <E extends Exception> void error(Supplier<String, E> message) throws E;
    ...
}

Then the design will be robust enough to withstand checked exceptions inside the lambda.

Nice one @BalRoger. Of course the old trace/debug/info/warn/error methods won't declare the new (generic) exceptions, so existing client code won't be affected.

In case anyone else needs to assure themselves that it will compile without casts:

import java.io.IOException;

public class LambdaTest {

    public interface Supplier<T, E extends Exception> {
        T get() throws E;
    }

    public interface Logger {
        <E extends Exception> void trace(Supplier<String, E> message) throws E;
        <E extends Exception> void debug(Supplier<String, E> message) throws E;
        <E extends Exception> void info(Supplier<String, E> message) throws E;
        <E extends Exception> void warn(Supplier<String, E> message) throws E;
        <E extends Exception> void error(Supplier<String, E> message) throws E;
    }

    public static void main(String[] args)
            throws InterruptedException, IOException {
        Logger log = new Logger() {
            @Override
            public <E extends Exception> void trace(Supplier<String, E> message)
                    throws E {
                System.out.println("TRACE: " + message.get());
            }

            @Override
            public <E extends Exception> void debug(Supplier<String, E> message)
                    throws E {
                System.out.println("DEBUG: " + message.get());
            }

            @Override
            public <E extends Exception> void info(Supplier<String, E> message)
                    throws E {
                System.out.println("INFO: " + message.get());
            }

            @Override
            public <E extends Exception> void warn(Supplier<String, E> message)
                    throws E {
                System.out.println("WARN: " + message.get());
            }

            @Override
            public <E extends Exception> void error(Supplier<String, E> message)
                    throws E {
                System.out.println("ERROR: " + message.get());
            }
        };
        log.debug(LambdaTest::debugMessage);
        log.info(() -> {
            Thread.sleep(0);
            return "This lambda can throw InterruptedException.";
        });
    }

    private static String debugMessage() throws IOException {
        return "This method declares IOException.";
    }

}

FWIW, such a solution would require ugly explicit types if calling from Kotlin:

import LambdaTest.*

fun main(args: Array<String>) {
    val log = DumbLogger()
    log.debug(Supplier<String, Exception> { ioException() })
    log.info<Exception> {
        Thread.sleep(0)
        "This lambda can throw InterruptedException."
    }
}

private fun ioException(): String {
    return "This method declares IOException."
}

Hopefully libraries like kotlin-logging will find a solution (the KLogger interface already extends org.slf4j.Logger to add lambda/function overloads of the trace/debug/info/warn/error methods, so adding similar methods to org.slf4j.Logger might cause trouble.)

Any thoughts on this proposal @oshai?

personally, I prefer the suggestion of @ceki. declaring the methods as throwing exception is not coherent with to original methods and I haven't seen such signature in other java libs.

@oshai , Let me address your reasons one at a time. First, the exception-safe version of these methods is MUCH more coherent with the original usage in terms of actual impact on the utility of code.

In the original API you would enter something like the following (based on my original example above):

log.debug(foo.getBar());

And, assuming the surrounding code was able to handle IOException, everything would be fine.

Now, because foo.getBar() is a somewhat expensive operation, it would be desirable to convert the logging to the lambda-based format:

log.debug(() -> foo.getBar());

Without the exception-safety generic parameterization, this code will fail to compile, regardless of how IOException is handled in the surrounding code. I find this to be a significant and debilitating change in the coherence (and utility!) of the logging interface.

Second you argue against a more robust exception-safe solution because "the standard java libs don't to it that way"? Well, quite frankly I find the the Java-8-defined functional interfaces to be of extremely limited usefulness for precisely that reason. I find quite often that I have to create an exception-safe alternative to a standard interface in java.util.function just to make it useful for me. If I can't put code that declares checked exceptions in a lambda, about half of all my opportunities to use lambdas to good advantage (including, most especially, those involving logging) won't work.

If the exception-unfriendly solution is adopted, I will continue to use my homegrown logging wrapper that actually works and ignore the half-broken "standard" solution.

since the only point of the exception is probably an auto-catch->log instead of making it a generic just make it T get() throws Throwable; I haven't needed that for logging... but have needed it for Try wrappers.

If you use the generic exception parameter you can convert the logging from current style to lambda-style without having to change the surrounding code.

If you use Throwable instead of the generic, you have to change the surrounding code to handle Throwable. That is, the following will not suffice:

Given this original code:

try {
    log.debug(foo.getBar());
} catch (IOException ioe) {
    // whatever ...
}

Then changing to lambda-style will force you to change the surrounding code to handle any Throwable, not just IOException. The following code will not compile:

try {
    log.debug(() -> foo.getBar());
} catch (IOException ioe) {
    // whatever ...
}

Thus, the minimal change to support lambda-style is now:

try {
    log.debug(() -> foo.getBar());
} catch (IOException ioe) {
    // whatever ...
} catch (Throwable t) {
    throw new RuntimeException(t);
}

No additional changes to the original code are needed if the exception is handled via generics. Well, aside from the current-style to lambda-style change itself.

I was thinking that you'd have

 void debug( ThrowableSupplier supplier ) {
     if ( this.isDebugEnabled() { 
          try {
             this.debug( supplier.get() ) {
          }
          catch ( Throwable e )  {
             this.debug( "debug supplier threw exception {}", e ) 
          }
     }
}

thus your caller doesn't need to handle the exception thrown by the supplier, I wouldn't think you'd want to anyways. That said I'd probably try to not log things that throw checked exceptions.

I see your point, but as a programmer I'm not sure I want my logging system intercepting my exceptions. For example, given your code what would happen if debug level is disabled? The exception would be lost! [EDIT: applies to code as originally written. The edit in the previous example fixes that.]

I personally would rather have the exception behavior with lambdas be as close to the original behavior as I could get.

I updated my sample at little, but if you had debug level disabled the supplier would never be called, thus the exception couldn't happen. The point of the lambda's in the first place is to lazily evaluate the supplier, thus no log level, no evaluation. I'm ok with treating a logging exception as internal to the logger... as it's not really part of my application code, but that's a personal preference, otherwise you want want to instead pass 2 functions, a logging Supplier and an exception Consumer

That said you might be better off not having the Exception on the supplier and instead using something like Try in javaslang I've also been writing something similar. I have also been working on writing something similar due to dealing with checked exceptions in filter and map

The supplier would be called regardless of log level in your code as [EDIT: originally] written. Let me tweak it a bit:

void debug( ThrowableSupplier supplier ) {
     if( !log.isDebugEnabled() ) { return; }
     try {
          log.debug( supplier.get() ) {
     }
     catch ( Throwable e )  {
          log.debug( "debug supplier threw exception {}", e ) 
     }
}

Okay, now under this code you have an even better point. But I have still lost control of the exception. If debug level IS enabled the logging system will now log the exception at the debug level. So NOW if my log analysis only looks for exceptions at the warn level or above, I'm out of luck. I'll miss this one.

Furthermore, and this is a minor nitpick, easily fixed, your exception logging statement, as written, does not print out the stack trace, only the exception message. It's a small point but it drives home the fact that my program's exception handling would now be partially usurped by the logging system. This does not seem to me to be an area of responsibility that the logging system should take on.

Okay, now under this code you have an even better point.

fundamentally it's the same code afaik, I presume the JIT compiler would be able to optimize that to the same thing.

Furthermore, and this is a minor nitpick, easily fixed, your exception logging statement, as written, does not print out the stack trace, only the exception message.

mm, our system would print out the stack trace in that case if you pass the exception object, so does spring boot.

so NOW if my log analysis only looks for exceptions at the warn level or above, I'm out of luck.

maybe you shouldn't do that... maybe you should analyze all exceptions, and simply disable logging at certain levels... just a thought, I don't have your use case

This does not seem to me to be an area of responsibility that the logging system should take on.

sure, though I would argue that perhaps it shouldn't handle checked exceptions at all, javaslang.

alternatively as already mentioned

void debug( ThrowableSupplier supplier ) {
     if( !log.isDebugEnabled() ) { return; }
     this.debug( supplier, e -> log.debug(  "debug supplier threw exception {}", e );
}

void debug( ThrowableSupplier supplier, Consumer<Throwable> consumer ) {
     if( !log.isDebugEnabled() ) { return; }
     try {
          log.debug( supplier.get() ) {
     }
     catch ( Throwable e )  {
         conumer.accept( e );
     }
}

now you could write

  log.debug(() -> foo.getBar(), e -> throw new RuntimeException(e));

I think you want to change your 3rd from last line in your [EDIT: original] 2nd debug method to:

consumer.accept(e)

Yeah, this works. If the consensus is to go this way I will grudgingly accept it. I still think the generic exception parameter is a more elegant solution, and would result in general in more minimal code changes to get roughly the same behavior with lambdas than does this solution.

Once you introduce a Supplier function which could throw an exception, even a RuntimeException, the problem of what to do with that exception does become significant.

Personally, I like @BalRoger's "generic throws" idea (Kotlin interop notwithstanding), but propagating the logging exception isn't always what you want:

1. Sometimes you just want to make absolutely sure your logging can't break your application, and the last thing you want to do is propagate such an exception.
   I know I hate it when my logging (perhaps added to help me debug a problem) actually causes another problem of its own.

2. Other times you do want to treat every exception as significant (after all, it's probably a bug in your code, even if it is logging code) and abort processing immediately by propagating the exception (perhaps directly, or perhaps wrapped in a LoggingRuntimeException).

Perhaps it would be justified to add a parameter to LoggerFactory.getLogger(), so that the user can choose a strategy for handling such logging exceptions (or pass in a ThrowableConsumer). I wouldn't really want to be passing in an exception handler to every debug logging call.

While I like the approach @BalRoger and @seanf outline, to pass through checked exceptions, I think it might be a bit overkill. I don't think it would be unreasonable to provide a way to use lambdas, constrained to runtime exceptions only. This is consistent with the limitations in the use of lambdas elsewhere in Java 8, such as in streams, and I think it's an acceptable trade-off to have the feature.

Any chance this could become a thing soon?

So this PR basically got stuck on exception handling?

@seanf - I see @BalRoger proposal in line with what is available now - when you use a method with checked exception to compute a value to log you must take care of the appropriate try/catch. If the method throws an unchecked exception it may break your application using the functional approach as well as the old, imperative one. There really is no difference.

AFAICS, @BalRoger proposal cleanly resolves the every day logging API usage, with the exception of cumbersome calls from Kotlin. There does not seem to be any other critical deficiencies in this PR which should keep it on hold, right?

Anyway, it would be nice to get this through, given that Java 8 was EOL-ed this March.

I'm not sure why but lambdas aren't available as params in 2.0.0-alpha... are they supposed to be?

A new slf4j-api module is out of the question.

Well, there's slf4j-api 2.0.x by now... never say never again 😉 However, it indeed seem like lazy evaluation of messages did not make it in there.

@sschuberth See LoggingEventBuilder and the fluent API.

@sschuberth See LoggingEventBuilder and the fluent API.

Ah, interesting approach, thanks for pointing it out @ceki!