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

Prevent compilation-time jetty log initialization #468

Closed
Invertisment opened this issue Jul 28, 2022 · 6 comments
Closed

Prevent compilation-time jetty log initialization #468

Invertisment opened this issue Jul 28, 2022 · 6 comments

Comments

@Invertisment
Copy link

Invertisment commented Jul 28, 2022

The commit cb28ec5 introcuces a new import of KeyStoreScanner into the namespace. That class has LOG instance which can be seen here: https://github.com/eclipse/jetty.project/blob/cb127793e5d8b5c5730b964392a9a905ba49191d/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/KeyStoreScanner.java#L39

This log instance is initialized at build time and this message is printed when I run ring uberjar on my Clojure app:

22-07-28 08:40:21 _ INFO [org.eclipse.jetty.util.log:170] - Logging initialized @15339ms to org.eclipse.jetty.util.log.Slf4jLog

The timestamp 15339ms is the time since the JVM has started and doesn't say much but what it does it initialize the logging at build time.

And this logging initialization prevents me from running graalvm's native-image command because this message appears:

> native-image -jar ./target/uberjar/app.jar
...
Caused by: com.oracle.graal.pointsto.constraints.UnsupportedFeatureException: No instances of com.github.fzakaria.slf4j.timbre.TimbreLoggerAdapter are allowed in the image heap as this class should be initialized at image runtime. Object has been initialized by the io.netty.channel.nio.AbstractNioChannel class initializer with a trace:`
...

That means that by requring namespace ring.adapter.jetty in any way (I tried to require run-jetty and :as but it still loads that class which I don't use at all (because my SSL is managed by my cloud provider)) I can't build graalvm executable.
My version: [ring/ring-jetty-adapter "1.9.5"]

@weavejester
Copy link
Member

weavejester commented Jul 28, 2022

Thanks for the report. A PR to fix this would be welcome.

@Invertisment
Copy link
Author

Invertisment commented Jul 28, 2022

I found that I can use 1.8.0 version and it compiles fine. But that's not a good workaround.

One fix that I can think of is to split the namespace so that the imports would be located in two separate namespaces and the builder methods would be separated as well.
I think this would require a major version bump (then the non-ssl namespace would be graalvm compilable and the other not).

One alternative to it would be to dynamically load the class by its title when the ssl method is called. But this would mean that if the JAR that contains that class changes then the build will break (but it would be known at build time at least).
Example: https://github.com/clojure/clojure/blob/e6fce5a42ba78fadcde00186c0b0c3cd00f45435/src/clj/clojure/core.clj#L6837

In any way the ssl function is not useful in graalvm because it loads the offending class. Do you think it's a good idea to support graalvm? I reported because I had an issue with it but I don't know what are the goals of this lib.

There could also be a hybrid approach where we would still have two namespaces but we would load the ssl namespace lazily on method call. This way we would be able to keep the current interface but still separate the namespaces. Then eventually the method could be marked with ^:deprecated.

I don't want to remove the functionality because somebody may depend on it (it would be unfortunate if suddenly somebody would find out that their webpage is HTTP-only because some kind of scanner didn't load).

Edit: I thought that ssl-connector is public. I'll make the lazy-namespace PR to review. Or maybe you'll have a better idea.

@weavejester
Copy link
Member

I think supporting GraalVM is a good idea, but if we want to make the support official it'll need tests to prevent regressions from classes like this. I'm fine with lazy loading the offending piece of code, with an eye to looking to add some GraalVM tests in future to prevent this sort of thing from happening in future.

@Invertisment
Copy link
Author

Invertisment commented Jul 28, 2022

My chat with @borkdude showed that I have no idea where the bug is. I tried to create a minimum repro and I think I can't find it. I'll try to import more things.
But on my large project it's consistently not failing with version 1.8.0 and failing with 1.8.1. So I missed something in my repro.

@Invertisment
Copy link
Author

I tried with my minimal repro and when I add [com.google.firebase/firebase-admin "8.1.0"] then my minimal repro fails no matter what. So it fails even if it's the only dependency.
But it gets more interesting.
There is version 9.0.0 which doesn't fail on minimal repro and even on the version with all of the other dependencies. But it fails when I use it in my large project.
So um... sorry for wasting time and accusing this project when it's not its fault. But at this point I actually don't really know what fails in my codebase. Something somehow binds together. But probably it's incorrect timing of the logger initialization on the google's side.
But currently I don't know and I'll close this issue. Minimal repro with this project and logger worked alright. So GraalVM works for 1.9.1. So at least somebody verified it.

@weavejester
Copy link
Member

Thanks for the investigation and verifying it works with 1.9.1. I hope you find the cause of your issue.

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

Successfully merging a pull request may close this issue.

2 participants