Figure out what to do with our dynapath 1.0 compatibility problems

[bbatsov]

Long story short:

• We don't support the latest version of dynapath (1.0)
• The version we're using (0.2.5) is going to stop working with Java 10, as it relied on implementation details that are going to be changed

We need to come up with some solution for this soon, otherwise it's going to cause problems for many people.

More details here clojure-emacs/cider-nrepl#482

[tsulej]

Maybe it's related, I've just checked Java 10 and got such warning. nRepl and Cider seem to work.

nREPL server started on port 51299 on host 127.0.0.1 - nrepl://127.0.0.1:51299
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by mranderson048.orchard.v0v1v0_20180321v113210_28.dynapath.v0v2v5.dynapath.defaults$eval7485$fn__7486 to method java.net.URLClassLoader.addURL(java.net.URL)
WARNING: Please consider reporting this to the maintainers of mranderson048.orchard.v0v1v0_20180321v113210_28.dynapath.v0v2v5.dynapath.defaults$eval7485$fn__7486
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
REPL-y 0.3.7, nREPL 0.2.12
Clojure 1.9.0
Java HotSpot(TM) 64-Bit Server VM 10+46

[SevereOverfl0w]

Mirroring the boot approach seems difficult unless we control the start of the JVM very closely. I believe it needs to run prior to clojure.

[bbatsov]

Unfortunately I'm out of other ideas. I'm guessing something must be possible, but I simply don't have the time to do the research for it. The fact that dynapath decided to drop this functionality instead of fixing it leads me to believe probably there's no easy solution, though.

[SevereOverfl0w]

Inside of boot, I think we're okay, as we can use their compatible classloader.

What features does dynapath provide to us?

[bbatsov]

We use it only to add the JDK resources dynamically to the classpath, so afterwards things like source lookup and doc lookup will work for them. See

orchard/src/orchard/java.clj

Line 32 in 3934ea6

(defn add-classpath!

Unfortunately we're using exactly the damn method that the deprecation warnings are about. :-)

[tirkarthi]

A re-implementation of add-classpath (deprecated in Clojure core) that:
is a little more comprehensive than core's add-classpath — it should work as expected in more circumstances, and optionally uses Maven-resolver to add a Maven artifact (and all of its transitive dependencies) to your Clojure runtime's classpath dynamically.

I don't know how viable it is to use pomegranate but quick googling led me to this from a comment on clojuredocs

[SevereOverfl0w]

Pomegranate uses dynapath, https://github.com/cemerick/pomegranate/blob/master/README.md#urlclassloader-modifiability

…

On April 2, 2018 7:18:34 AM GMT+01:00, Xtreak ***@***.***> wrote: > A re-implementation of add-classpath (deprecated in Clojure core) that: > is a little more comprehensive than core's add-classpath — it should work as expected in more circumstances, and optionally uses Maven-resolver to add a Maven artifact (and all of its transitive dependencies) to your Clojure runtime's classpath dynamically. I don't know how viable it is to use pomegranate but quick googling led me to this from a [comment](https://clojuredocs.org/clojure.core/add-classpath#example-59d7d6d8e4b03026fe14ea57) on clojuredocs -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #20 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[SevereOverfl0w]

Pomegranate uses dynapath, it has the same problem. But seems to have some solution. https://github.com/cemerick/pomegranate/blob/master/README.md#urlclassloader-modifiability

…

On April 2, 2018 7:18:34 AM GMT+01:00, Xtreak ***@***.***> wrote: > A re-implementation of add-classpath (deprecated in Clojure core) that: > is a little more comprehensive than core's add-classpath — it should work as expected in more circumstances, and optionally uses Maven-resolver to add a Maven artifact (and all of its transitive dependencies) to your Clojure runtime's classpath dynamically. I don't know how viable it is to use pomegranate but quick googling led me to this from a [comment](https://clojuredocs.org/clojure.core/add-classpath#example-59d7d6d8e4b03026fe14ea57) on clojuredocs -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #20 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[bbatsov]

@SevereOverfl0w Not sure they have a real workaround.

If you are in a situation where you are using pomegranate but do not have a clojure.lang.DynamicClassLoader visible in your classloader hierarchy, you will need to explicitly enable the java.net.URLClassLoader support in dynapath (upon which pomegranate relies for such things).

This sounds like some flag to suppress the new behavior or something. And I don't see it in dynapath's readme.

[SevereOverfl0w]

Why are we not under clojure.lang.DynamicClassLoader when running?

[bbatsov]

No idea to be honest. I think @jeffvalk worked on this originally and we never had to change the code afterwards. Maybe he remembers something about this.

[jeffvalk]

For full Java source support, we need the JDK compiler classes in tools.jar, which is not on the classpath by default. It's also nice to have src.zip, so that the JDK class sources are available to the info middleware (jump-to, class/member docstrings, etc).

Dynapath was (at the time anyway...) the best option for loading these at runtime. And I believe dynapath does use a clojure.lang.DynamicClassLoader internally.

The use of dynapath wasn't very sophisticated; we just need to append those two resources to the classpath.

[bbatsov]

@jeffvalk Looking at the code it seems it's URLClassLoader, unless I'm misunderstanding something. But I guess we need to change this now (and this would also mean we don't need dynapath anymore).

[zcaudate]

I'm personally fine with the reflective warnings. are you sure java 10 is going to ban it completely?

if cider can modify the ns macro https://github.com/clojure/clojure/blob/010864f8ed828f8d261807b7345f1a539c5b20df/src/clj/clojure/core.clj#L5553

to provide a alternate classloader with the additional urls, then it might work.

[zcaudate]

Actually. I realised it's a slightly different problem (I was thinking in-editor evaluation), because ns doesn't have to be evalutated, it's a little easier to go around the dynapath thing.

Just write an alternate load method.

https://github.com/clojure/clojure/blob/010864f8ed828f8d261807b7345f1a539c5b20df/src/clj/clojure/core.clj#L5876-L5893

[jmingtan]

Hi, I'd like to summarize my understanding on this issue, and get some feedback from the maintainers if possible, especially to clear up any misconceptions :)

This is what I understand from reading the code, and from @jeffvalk's comments:

1. The sole namespace in orchard that depends on dynapath is orchard.java, which uses it to dynamically add the JDK's src.zip and tools.jar to the classpath
2. tools.jar is needed solely for access to the Javadoc classes, by orchard.java.parser whose job is to return source information (line numbers, docstrings) from a Java source file
3. src.zip is needed solely for source code lookup on JDK classes

Here are some minor problems posed by JDK version 9 and above:

• tools.jar is now removed, because the jdk classes are now present on the classpath
• java.home now returns the path to the jdk folder instead of the jre folder
• src.zip is moved into ${java.home}/lib

Here are the more major problems with JDK 9 and above:

• we're not able to "hack" java.net.URLClassLoader to invoke its protected methods anymore, making it harder to dynamically add new classpaths in the default class loader chain
• internally, src.zip has undergone a restructuring where each package is now under its corresponding module folder (e.g. "java.base/java/lang/String.java") which breaks the current lookup mechanism
• IMO the biggest problem is the Javadoc classes that parser.clj depends on can't be called because they are not being exported by the jdk.javadoc module. Trying to call their methods results in an IllegalAccessError.

I'm actually hoping I'm wrong on that last point, otherwise I think it means we need to rewrite parser.clj just to support JDK 9 and above - there's a project called javaparser that might help, but it would mean bringing in a new dependency. I guess my question is does this sound like a good approach? is there a strong preference not to bring in new dependencies?

The short-term approach here might just be to detect if JDK >= 9 and just not call add-classpath! in order to prevent any illegal access warnings. This would prevent source-info lookup, but I think that would be broken anyway due to not having access to jdk classes.

[jeffvalk]

Great summary, @jmingtan. This all sounds right to me at first read.

Regarding the big problem:

the biggest problem is the Javadoc classes that parser.clj depends on can't be called because they are not being exported by the jdk.javadoc module. Trying to call their methods results in an IllegalAccessError.

The com.sun.javadoc classes are now at jdk.javadoc.doclet (see Migration Guide). So far, so good.

The other internal API classes currently imported (those in com.sun.source.tree, com.sun.tools.javac.util, com.sun.tools.javadoc) are used only to set up the compiler in the fn parse-java. This fn exists only because the default Javadoc compiler in JDK7 required each source to be specified as a file path to a .java file. If the JDK9 version will accept either a jar resource path, or text content all of these imports can go away! This would be the first thing to look into.

[jeffvalk]

I had a quick go at this. I have a version of parser.clj that works on JDK9+ without referencing any internal classes. I'll clean it up when I have time (hopefully over the weekend).

[bbatsov]

@jeffvalk Fantastic news!

[jmingtan]

Thanks @jeffvalk! I had a go at implementing your suggestion too but I couldn’t figure out how to make the JavadocTool accept a zip file path, looking forward to seeing your implementation :)

[jeffvalk]

Update on this: The revised parser works fine with JDK9+ for all non-JDK Java sources. The sources to the JDK itself are tricky though -- partly due to the new Java module system, but mostly to a poor public API for the JDK compiler. The JDK sources are a primary use case; I'll continue to push on this to get it working.

[bbatsov]

@jeffvalk How much more time do you think you'll need? I'm thinking of cutting a new cider-nrepl release over the new few days.

[jeffvalk]

@bbatsov It's hard to estimate -- getting the public compiler API to work with the modular JDK sources involves a lot of digging into compiler internals to figure out why different types of access errors occur. I'm sure there's a common thread (or a few), but it's not obvious (yet) and it's certainly not well-documented.

What we could do in the meantime is release a version that works with most Java sources, but ignores the JDK sources under Java 9+. Then we can at least address the Dynapath dependency, which is what this issue was originally about.

I'll try to give this a go in the next day. If I don't manage to get it done in the next 24 hours, it will likely be another week before I can push anything useful. (Holidays and such, you know -- which, btw, happy holidays!)

[bbatsov]

What we could do in the meantime is release a version that works with most Java sources, but ignores the JDK sources under Java 9+. Then we can at least address the Dynapath dependency, which is what this issue was originally about.

I can live with this as a first step. :-)

I'll try to give this a go in the next day. If I don't manage to get it done in the next 24 hours, it will likely be another week before I can push anything useful. (Holidays and such, you know -- which, btw, happy holidays!)

Thanks! Happy Holidays to you as well!

As for the release - it's not that big of a deal. If we have to wait a bit for it, I guess we'll wait.

[jeffvalk]

@bbatsov How far into the future do you envision Java 8 support?

[bbatsov]

1-2 years at least, maybe more. It reaches its official EOL in December 2020. Given the massive changes in Java 9, I assume the migration from Java 8 is going to take quite a while for some organizations.

[jeffvalk]

Figured that was the case; makes sense. I'll write the 8 vs 9+ lib loading with that in mind.

Must say, if other people's experience with Java modules is like what I've had trying to get the compiler API running, Java 8 may be around for longer than anyone wants...

[bbatsov]

@jeffvalk When you click on a failed build there should be a button to re-run it.

[jeffvalk]

If there is one, it's not obvious where...

[jeffvalk]

Does it require a separate login to circleci?

[bbatsov]

That's what I see

I'm always logged in there with my GH account, though.

[jeffvalk]

I can reproduce the JDK8 errors by ensuring tools.jar exists, but does not contain the expected class files. Perhaps a bad symlink in the CI image?

[bbatsov]

Remove "fake" classpath references. It's either on the classpath or it isn't.

Btw, won't this break the lookup of resources in Boot? I think the resources were on the fake classpath, but I don't use Boot, so I don't quite remember what were they doing exactly.

[bbatsov]

Perhaps a bad symlink in the CI image?

Could be. I haven't checked the image myself - I just assumed Circle know what they are doing. :-)

[jeffvalk]

Could be. I haven't checked the image myself - I just assumed Circle know what they are doing. :-)

I'm hopeful. :-) Is there any way to check? Having tools.jar exist but be a bad file of some sort (content, permissions) reproduces the errors exactly, and it's the only way I've found. The tests all pass for me, and I ran all the configurations repeatedly in a loop (wanted to check for Heisenbugs).

[jeffvalk]

Btw, won't this break the lookup of resources in Boot?

The Boot logic seemed only to be appending tools.jar and src.zip onto the "fake" classpath. Since those are on the actual classpath whenever they're present, I don't see why anything fake is sensible. This said, I also don't use Boot.

[jeffvalk]

BTW I don't have the dropdown menu you showed above in CircleCI. I presume it requires being logged in.

[bbatsov]

I guess so. Btw, I see that on Java 11 there's some different problem:

lein test :only orchard.eldoc-test/test-eldoc

ERROR in (test-eldoc) (HashMap.java:1134)
arglist extraction
expected: (:eldoc (eldoc/eldoc (info/info (quote clojure.core) (quote .toString))))
  actual: java.util.ConcurrentModificationException: null
 at java.util.HashMap.computeIfAbsent (HashMap.java:1134)
    com.sun.tools.javac.model.JavacElements.unboundNameToSymbol (JavacElements.java:199)
    com.sun.tools.javac.model.JavacElements.doGetElement (JavacElements.java:182)
    com.sun.tools.javac.model.JavacElements.doGetTypeElement (JavacElements.java:172)
    com.sun.tools.javac.model.JavacElements.getTypeElement (JavacElements.java:160)
    com.sun.tools.javac.model.JavacElements.getTypeElement (JavacElements.java:87)
    orchard.java.parser$typesym.invokeStatic (parser.clj:180)

Doesn't happen all the time, but it's probably something worth looking into.

[jeffvalk]

Yeah, this is the Heisenbug I was referring to. Happened for me on 18 of 1113 (1.6%) test runs. Not sure why it's more frequent on the CI server, but that's useful!

This one appears to be a JDK bug in the Language Model API. ConcurrentModificationExceptions are thrown whenever an iterator's collection is modified by something other than the iterator. The parser code triggering this doesn't modify anything; it only reads the tree once it's fully parsed. From the stacktrace, it seems read calls are mutating the hash map. Sneaky mutability is ugly.

I'll find a workaround.

[jeffvalk]

Workaround applied.

The remaining CI build problems are related to this:

I can reproduce the JDK8 errors by ensuring tools.jar exists, but does not contain the expected class files. Perhaps a bad symlink in the CI image?

[shen-tian]

@shen-tian Is the wishlist a short-term or long-term one? 🙂 If the latter, is there another image available that includes it?

More likely a longer term thing... I'm sure better images exist, but nothing that's explicitly maintained for this purpose. Someone who's got good Dockerfile experience would be useful..

In the short term, we can probably just install src.zip using apt-get install openjdk-8-source etc. But gonna take some experimenting to get working.

[jeffvalk]

In the short term, we can probably just install src.zip using apt-get install openjdk-8-source etc. But gonna take some experimenting to get working.

This would be good.

One quick note, if the images are Debian-based as your note suggests: The Debian packages install the JDK sources in a directory that's not a child of the JDK directory (e.g. /usr/lib/jvm/openjdk-11/lib/src.zip vs /usr/lib/jvm/java-11-openjdk-amd64/). They'll need to be symlinked manually.

[birdspider]

The Debian packages install the JDK sources in a directory that's not a child of the JDK directory

FYI, I don't think this is debian specific but due to different java packaging guidelines? It seems (in linux world) java-8 is the last version with src.zip as child of JDK directory, otherwise within ./lib/

# ubuntu-bionic-18.04
# ubuntu-cosmic-18.10
/usr/lib/jvm/openjdk-8/src.zip

# archlinux
/usr/lib/jvm/java-12-adoptopenjdk/lib/src.zip
/usr/lib/jvm/java-11-openjdk/lib/src.zip

# ubuntu-bionic-18.04
# ubuntu-cosmic-18.10
# ubuntu-disco-19.04
/usr/lib/jvm/openjdk-11/lib/src.zip

# ubuntu-disco-19.04
/usr/lib/jvm/openjdk-12/lib/src.zip

[jeffvalk]

I don't think this is debian specific but due to different java packaging guidelines?

I have no insight here. If there's a specific guideline, I'd be interested in knowing what it is!

java-8 is the last version with src.zip as child of JDK directory, otherwise within ./lib/

Orchard will find src.zip in either of the two common locations -- in the root of the JDK or in lib. But the Debian/Ubuntu packages (and possibly other distros) install sources in a sibling directory to the JDK, hence the need to manually symlink src.zip:

/usr/lib/jvm/java-11-openjdk-amd64/   # openjdk-11-jdk
/usr/lib/jvm/openjdk-11/              # openjdk-11-source

[jeffvalk]

Created #50 to track the CI configuration issue.

[jeffvalk]

@bbatsov A quick note as you're thinking about getting rid of the dynapath dependency:

That code is easily replaced; dynapath itself is only a few lines. The value isn't the code, though; it's the protocol and compatibility with anything else that implements it. (This also has implications for rewriting/inlining the dependency.) Currently, any container, application server, etc that implements dynapath's protocol should "just work" with Orchard's runtime classloading. If Orchard doesn't reference that protocol, this might not be true. (The actual impact depends on the container behavior.)

Orchard doesn't do a lot of classloading (only tools.jar and src.zip), and I don't know how many users are running Clojure in runtime container of some sort. Maybe this isn't a use case worth supporting. Thought it was worth mentioning though.

[bbatsov]

That code is easily replaced; dynapath itself is only a few lines. The value isn't the code, though; it's the protocol and compatibility with anything else that implements it. (This also has implications for rewriting/inlining the dependency.) Currently, any container, application server, etc that implements dynapath's protocol should "just work" with Orchard's runtime classloading. If Orchard doesn't reference that protocol, this might not be true. (The actual impact depends on the container behavior.)

I wasn't aware there was any protocol to go with dynapath - for some reason I thought it was just a mechanism for dynamically loading code.

Orchard doesn't do a lot of classloading (only tools.jar and src.zip), and I don't know how many users are running Clojure in runtime container of some sort. Maybe this isn't a use case worth supporting. Thought it was worth mentioning though.

In general I was thinking that maybe loading some extra resources should be done by the clients, not by Orchard itself (e.g. cider-nrepl). dynapath is a relatively low-risk dependency (as it's unlikely that user code would depend on it), but still I'd be happy if we manage to get to the dream state of 0 runtime deps.

[jeffvalk]

In general I was thinking that maybe loading some extra resources should be done by the clients, not by Orchard itself ... I'd be happy if we manage to get to the dream state of 0 runtime deps.

Moving runtime classloading to the clients is definitely possible. Orchard could just check the classpath to see if specific resources are present and act accordingly.

[bbatsov]

Yep, exactly.

[jumarko]

This was linked from clojure-emacs/cider#2269 but it's unclear to me whether that issue is fixed or not. Is the javadoc & source code navigation supposed to work for JDK version > 8?