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

Undeclared runtime dependency on de.unkrig.jdiasm.Disassembler #13

Closed
rktoomey opened this Issue Jan 4, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@rktoomey
Copy link

rktoomey commented Jan 4, 2017

Digging into this runtime error from a Logback configurator, I discovered that janino has an undeclared runtime dependency on de.unkrig.jdiasm:

java.lang.ClassNotFoundException: de.unkrig.jdisasm.Disassembler
        at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:331)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
        at java.lang.Class.forName0(Native Method)
        at java.lang.Class.forName(Class.java:264)
        at org.codehaus.janino.UnitCompiler.disassembleToStdout(UnitCompiler.java:11913)
        at org.codehaus.janino.UnitCompiler.addClassFile(UnitCompiler.java:847)
        at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:841)
        at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:431)
        at org.codehaus.janino.UnitCompiler.access$400(UnitCompiler.java:209)
        at org.codehaus.janino.UnitCompiler$2.visitPackageMemberClassDeclaration(UnitCompiler.java:385)
        at org.codehaus.janino.UnitCompiler$2.visitPackageMemberClassDeclaration(UnitCompiler.java:380)
        at org.codehaus.janino.Java$PackageMemberClassDeclaration.accept(Java.java:1405)
        at org.codehaus.janino.UnitCompiler.compile(UnitCompiler.java:380)
        at org.codehaus.janino.UnitCompiler.compileUnit(UnitCompiler.java:354)
        at org.codehaus.janino.SimpleCompiler.compileToClassLoader(SimpleCompiler.java:413)
        at org.codehaus.janino.ClassBodyEvaluator.compileToClass(ClassBodyEvaluator.java:318)
        at org.codehaus.janino.ClassBodyEvaluator.cook(ClassBodyEvaluator.java:235)
        at org.codehaus.janino.SimpleCompiler.cook(SimpleCompiler.java:200)
        at org.codehaus.commons.compiler.Cookable.cook(Cookable.java:76)
        at org.codehaus.commons.compiler.Cookable.cook(Cookable.java:71)
        at ch.qos.logback.core.joran.conditional.PropertyEvalScriptBuilder.build(PropertyEvalScriptBuilder.java:47)
        at ch.qos.logback.core.joran.conditional.IfAction.begin(IfAction.java:65)
        at ch.qos.logback.core.joran.spi.Interpreter.callBeginAction(Interpreter.java:269)
        at ch.qos.logback.core.joran.spi.Interpreter.startElement(Interpreter.java:145)
        at ch.qos.logback.core.joran.spi.Interpreter.startElement(Interpreter.java:128)
        at ch.qos.logback.core.joran.spi.EventPlayer.play(EventPlayer.java:50)
        at ch.qos.logback.core.joran.GenericConfigurator.doConfigure(GenericConfigurator.java:158)
        at ch.qos.logback.core.joran.GenericConfigurator.doConfigure(GenericConfigurator.java:145)
        at ch.qos.logback.core.joran.GenericConfigurator.doConfigure(GenericConfigurator.java:103)
        at ch.qos.logback.core.joran.GenericConfigurator.doConfigure(GenericConfigurator.java:53)
        at play.api.libs.logback.LogbackLoggerConfigurator.configure(LogbackLoggerConfigurator.scala:94)

Which is right there in the code but it's not in the pom:

https://github.com/janino-compiler/janino/blob/master/janino/src/main/java/org/codehaus/janino/UnitCompiler.java#L11952-L11967

Please add a runtime dependency to your pom.xml to make this explicit so that it will show up properly in dependency management tools.

rktoomey added a commit to rktoomey/janino that referenced this issue Jan 4, 2017

rktoomey added a commit to rktoomey/janino that referenced this issue Jan 4, 2017

aunkrig added a commit that referenced this issue Jan 11, 2017

#13
Undeclared runtime dependency on de.unkrig.jdiasm.Disassembler
@aunkrig

This comment has been minimized.

Copy link
Member

aunkrig commented Jan 11, 2017

Hello Rose,

JDISASM is only used when I debug JANINO, that's why it is adressed through REFLECTION and not invoked directly. In other words: JANINO does not (strictly) depend on JDISASM.

An attempt to access JDISASM is only made when the Unitcompiler.LOGGER ist set to level FINEST (which is what you obviously did). If, in that case, JDISASM cannot be found on the CLASSPATH, JANINO prints a stack trace to indicate the fact, but continues otherwise.

To make the point clearer, I replaced the printing of the stack trace with a log message:

UnitCompiler.LOGGER.log(Level.FINEST, (
    "Notice: Could not disassemble class file for logging because "
    + "\"de.unkrig.jdisasm.Disassembler\" is not on the classpath. "
    + "If you are interested in disassemblies of class files generated by JANINO, "
    + "get de.unkrig.jdisasm and put it on the classpath."
));

Please let me know if this solution is acceptable for you.

@aunkrig aunkrig closed this Jan 11, 2017

aunkrig added a commit that referenced this issue Jan 11, 2017

#13
Undeclared runtime dependency on de.unkrig.jdiasm.Disassembler
@rktoomey

This comment has been minimized.

Copy link

rktoomey commented Jan 11, 2017

Hello Arno,

It's certainly better than blowing up.

But there's another issue you should be aware of! A project that uses an slf4j to JUL logging bridge with the default setup experiences this exception regardless of whether FINEST logging is enabled for com.codehaus.janino because:

http://www.slf4j.org/apidocs/org/slf4j/bridge/SLF4JBridgeHandler.html

Please note that translating a java.util.logging event into SLF4J incurs the cost of constructing LogRecord instance regardless of whether the SLF4J logger is disabled for the given level.

So the application blows up anyway because the logging bridge is eagerly evaluating the log record statement 🎉

I understand this unfortunate collection of circumstances is outside your control! I'm documenting it here because I hope the next person who runs into this will find this useful.

One possible solution is to setup a LevelChangePropagator in the logback config:

    <contextListener class="ch.qos.logback.classic.jul.LevelChangePropagator"/>
    <logger name="org.codehaus.janino" level="WARN"/>

However, since the majority of Java projects these days use a logging bridge for JUL, please consider using a -Djdiasm.debug=TRUE property to turn this functionality on instead of using the log level as a proxy to indicate you want to debug.

Thanks,
Rose

@rktoomey

This comment has been minimized.

Copy link

rktoomey commented Jan 25, 2017

@aunkrig would you be open to a PR switching your logging implementation from JUL to SLF4J?

Very few projects primarily use JUL any longer, and I see another user has encountered the issue caused by eager evaluation of JUL log events. As logback users upgrade and bring in the new version of your library, I believe more users will encounter this error.

If you would be open to the change, I would be happy to contribute a PR that converts your JUL logging to SLF4J:

  1. adds slf4j-api as a compile dependency
  2. adds slf4j-simple or similar as a test dependency
  3. adds a test scoped logging config that would show your asm output as TRACE (which is the slf4j equivalent of FINEST)
  4. straightforward conversion of log statements to the new syntax

Please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment