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

Can't get SLF4J logging to work #22

Closed
aelkayesh opened this issue Aug 20, 2018 · 6 comments
Closed

Can't get SLF4J logging to work #22

aelkayesh opened this issue Aug 20, 2018 · 6 comments
Labels

Comments

@aelkayesh
Copy link

aelkayesh commented Aug 20, 2018

I am trying to get slf4j to work within the notebooks because some 3rd party libs that I use need it. When I try to set jshell classpath directly through --class-path it works. If I set IJAVA_CLASSPATH in kernel.json to point to the logging jars or when I import them in the notebook, it does not work, it says SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".

%maven org.slf4j:slf4j-api:1.7.16
%maven ch.qos.logback:logback-classic:1.0.13    
 
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.slf4j.impl.StaticLoggerBinder;

Logger slf4jLogger = LoggerFactory.getLogger("T");
slf4jLogger.warn("Hi, {}", "Test");
@aelkayesh
Copy link
Author

aelkayesh commented Aug 20, 2018

Next thing I did was using the %jars magic like this:

%jars = "C:\\logback-classic-1.2.3.jar"
%jars = "C:\\logback-core-1.1.2.jar"
%jars = "C:\\slf4j-api-1.7.25.jar"

Same error..
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".

@SpencerPark
Copy link
Owner

Thank you for the report. I was able to reproduce and can confirm this is a bug.

Classpath related things are an ongoing issue with all the dynamic loading going on in this environment. I've been trying to find a fix but the closest I've gotten in with the newest beta version and manually loading things which still has some warnings but eventually does log something. I will continue to investigate and update when I get a chance.

In case you are curious this is what I am referring to is the following which essentially pulls out the code from slf4j that loads the logger.

%maven org.slf4j:slf4j-api:jar:1.8.0-beta2
%maven ch.qos.logback:logback-classic:jar:1.3.0-alpha4

import org.slf4j.spi.SLF4JServiceProvider;

ServiceLoader<SLF4JServiceProvider> serviceLoader = ServiceLoader.load(SLF4JServiceProvider.class);
List<SLF4JServiceProvider> providerList = new ArrayList<SLF4JServiceProvider>();
for (SLF4JServiceProvider provider : serviceLoader)
    providerList.add(provider);
    
providerList.get(0).initialize();
providerList.get(0).getLoggerFactory().getLogger("JupyterNotebook").info("HelloWorld")

@aelkayesh
Copy link
Author

Thank you for your swift reply and for your efforts in this project.

This could work well as a workaround to use logging explicitly in the cells. Unfortunately it won't work with 3rd party libs that use SLF4j default mechanism to find logging implementations. I will be waiting for a fix for this issue.

Thank you again

@aelkayesh
Copy link
Author

One more thing: I tried the scijava kernel and it works find with slf4j. It uses openjdk 8 and of course does not use jshell.

@SpencerPark
Copy link
Owner

Alright I think I've finally found the culprit. The classloader for adding modules at runtime is a child of the system classloader. The system classloader is what actually has slf4j dependency because it is used by the shrinkwrap library and therefore loads it at startup. This was an unfortunate (but necessary) side effect of switching to a single jvm rather than 2.

When the dependency is added via the configuration it is loaded by the same classloader as the slf4j and hence it is able to find the class (it asks the LoggerFactory class' classloader for the resource).

I think the best course of action is to use 2 different class loaders for the user and kernel spaces. Unfortunately this would be a fairly big breaking change as this removes the kernel from the user space. This is a pretty big change as there is a common runtime that the kernel and user space share which now needs duplicating. I'll see how far I get on a different branch and update if there is any news.

Temporarily I have added org.slf4j:slf4j-simple:1.7.22 as a dependency to go along with what is included with shrinkwrap. I believe this should allow any projects logging with slf4j to run which would fix this issue. The problem mentioned above is another issue but feel free to leave this open until then if you would like.

@aelkayesh
Copy link
Author

Thank you a million :-) It works as needed so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants