Clojure

Clojure can't be loaded from the boot classpath under java 9

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.9
  • Fix Version/s: Release 1.9
  • Component/s: None
  • Labels:
  • Patch:
    Code
  • Approval:
    Ok

Description

As part of the changes for the jigsaw module system in Java 9, the
java packages available to the boot classloader are now a subset of
the the full java distribution. This means that classes loaded via the
boot classloader cannot access any classes outside of that subset.

In the boot classloader only the java.base module is available. For a complete list of module/package listings see http://cr.openjdk.java.net/~mr/jigsaw/ea/module-summary.html

Clojure itself uses java.sql.Timestamp in clojure.instant to provide print-method and print-dup implementations for java.sql.Timestamp.

This can be seen with (using Clojure 1.4.0 or higher, and a early-access build
of Java 9, most recently tested with 9-ea+147):

java -Xbootclasspath/a:clojure.jar clojure.main -e "(require 'clojure.instant)"

This affects any clojure-based tool that puts itself on the boot
classpath in order to gain a startup time boost (both lein
and boot are affected currently).

Proposed: If java.sql.Timestamp is not available, do not load instant.clj or install it in the default data readers.

Patch: clj-2077-4.patch

Screener Notes: This looks correct and does not break normal usage. Should be tested in the bootclasspath scenarios people have problems with.

  1. CLJ-2077.patch
    17/Aug/17 6:48 PM
    3 kB
    Ghadi Shayban
  2. clj-2077-2.patch
    06/Sep/17 3:50 PM
    3 kB
    Alex Miller
  3. clj-2077-3.patch
    08/Sep/17 10:19 AM
    2 kB
    Alex Miller
  4. clj-2077-4.patch
    08/Sep/17 12:51 PM
    2 kB
    Alex Miller

Activity

Hide
Toby Crawley added a comment -

More details on the underlying change that is triggering this are available at http://openjdk.java.net/jeps/261 (search for java.sql to find the relevant section).

Show
Toby Crawley added a comment - More details on the underlying change that is triggering this are available at http://openjdk.java.net/jeps/261 (search for java.sql to find the relevant section).
Hide
Alex Miller added a comment -

Does this need to be a ticket here? Or is this really an issue for build tools?

Show
Alex Miller added a comment - Does this need to be a ticket here? Or is this really an issue for build tools?
Hide
Toby Crawley added a comment -

That depends on if we want using Clojure from the boot classpath to be an acceptable use case. If not, then I agree, it is just an issue for tooling.

Show
Toby Crawley added a comment - That depends on if we want using Clojure from the boot classpath to be an acceptable use case. If not, then I agree, it is just an issue for tooling.
Hide
Toby Crawley added a comment -

I realized today that this issue doesn't actually affect boot, since it doesn't use the bootclasspath. So lein is the only tooling I know of that is affected by this.

Show
Toby Crawley added a comment - I realized today that this issue doesn't actually affect boot, since it doesn't use the bootclasspath. So lein is the only tooling I know of that is affected by this.
Hide
Ivan Pierre added a comment -

https://docs.oracle.com/javase/8/docs/api/java/sql/Timestamp.html
Would it be possible to use java.util.Date instead. Alas it's not possible to downcast :
Due to the differences between the Timestamp class and the java.util.Date class mentioned above, it is recommended that code not views Timestamp values generically as an instance of java.util.Date. The inheritance relationship between Timestamp and java.util.Date really denotes implementation inheritance, and not type inheritance.

Show
Ivan Pierre added a comment - https://docs.oracle.com/javase/8/docs/api/java/sql/Timestamp.html Would it be possible to use java.util.Date instead. Alas it's not possible to downcast : Due to the differences between the Timestamp class and the java.util.Date class mentioned above, it is recommended that code not views Timestamp values generically as an instance of java.util.Date. The inheritance relationship between Timestamp and java.util.Date really denotes implementation inheritance, and not type inheritance.
Hide
Ivan Pierre added a comment - - edited

The problem with Date is that it doesn't deal with nanoseconds. But Timestamp is created by a long giving the TimeDate value in milliseconds.
The use of setNano and getNano are only to store the SQL TIMESTAMP. Wouldn't it be better to deal with this value in another way?

The other way is to take just what we need from TimeStamp, and it's just a little thing, I'll try to compile with that to see if some other thing comes after...

Test code : https://gist.github.com/ivanpierre/b0ea937dac97d910a7c3c1e5774028e0

Show
Ivan Pierre added a comment - - edited The problem with Date is that it doesn't deal with nanoseconds. But Timestamp is created by a long giving the TimeDate value in milliseconds. The use of setNano and getNano are only to store the SQL TIMESTAMP. Wouldn't it be better to deal with this value in another way? The other way is to take just what we need from TimeStamp, and it's just a little thing, I'll try to compile with that to see if some other thing comes after... Test code : https://gist.github.com/ivanpierre/b0ea937dac97d910a7c3c1e5774028e0
Hide
Ivan Pierre added a comment - - edited

Ok, I pass to the GNU version of Timestamp. The code is neater. I mixed some of Sun's for more consistency. I dropped the string management of dates as Clojure will do it in clojure.instant.

It still works. I had a doubt...

If I type (clojure.lang.TimeStamp. 3678141) the response will be :
==> #inst "1970-01-01T01:01:18.141000000-00:00" with a nano of
141000000

But is if set nano to 1 : (.setNanos (clojure.lang.TimeStamp. 3678141) 1) the response is : #inst "1970-01-01T01:01:18.000000001-00:00"

This is correct, but it's a little disturbing to see my nice .141 disappear...

I put a fork on my GitHub. Last commit : https://github.com/ivanpierre/clojure/commit/749a0184ee7409290dad9ff353605fcaabd64f69

So, good, now pass to Leinigen...

Show
Ivan Pierre added a comment - - edited Ok, I pass to the GNU version of Timestamp. The code is neater. I mixed some of Sun's for more consistency. I dropped the string management of dates as Clojure will do it in clojure.instant. It still works. I had a doubt... If I type (clojure.lang.TimeStamp. 3678141) the response will be : ==> #inst "1970-01-01T01:01:18.141000000-00:00" with a nano of 141000000 But is if set nano to 1 : (.setNanos (clojure.lang.TimeStamp. 3678141) 1) the response is : #inst "1970-01-01T01:01:18.000000001-00:00" This is correct, but it's a little disturbing to see my nice .141 disappear... I put a fork on my GitHub. Last commit : https://github.com/ivanpierre/clojure/commit/749a0184ee7409290dad9ff353605fcaabd64f69 So, good, now pass to Leinigen...
Hide
Alex Miller added a comment -

I think the first question here is: do we expect that Clojure should be loadable from the bootclasspath?

While I realize this is a hack people use, my initial answer would be no (that was never a design constraint afaik). But will need to defer to Rich on that.

Show
Alex Miller added a comment - I think the first question here is: do we expect that Clojure should be loadable from the bootclasspath? While I realize this is a hack people use, my initial answer would be no (that was never a design constraint afaik). But will need to defer to Rich on that.
Hide
Ivan Pierre added a comment - - edited

Well Leiningen do it, even with the lein repl. I test to see if TimeStamp is the only thing.
Alas Leinigen is not 1.9 compatible, so I have to go down to version 1.8 to make the tests. (problem of conflict between clojure.spec and hara library)
A funny thing would be to pass the whole Clojure test in bootstrap so we would know...

Show
Ivan Pierre added a comment - - edited Well Leiningen do it, even with the lein repl. I test to see if TimeStamp is the only thing. Alas Leinigen is not 1.9 compatible, so I have to go down to version 1.8 to make the tests. (problem of conflict between clojure.spec and hara library) A funny thing would be to pass the whole Clojure test in bootstrap so we would know...
Hide
Ivan Pierre added a comment - - edited

The java Timestamp could be directly integrated into clojure.instant as it's a new datatype. So no need to worry about copyright stuff, and integer it in a complete manner to accept SQL TIMESTAMP, and some correct protocols.
The worst is that looking across the DBs documentation doesn't help a lot and some are quite contradictory.

Show
Ivan Pierre added a comment - - edited The java Timestamp could be directly integrated into clojure.instant as it's a new datatype. So no need to worry about copyright stuff, and integer it in a complete manner to accept SQL TIMESTAMP, and some correct protocols. The worst is that looking across the DBs documentation doesn't help a lot and some are quite contradictory.
Hide
Phil Hagelberg added a comment -

> I think the first question here is: do we expect that Clojure should be loadable from the bootclasspath?

I don't know if it's the right thing for Clojure to be able to be loaded from the bootclasspath, but as an additional point of data: Leiningen takes 2.5 seconds to load on my machine from the bootclasspath on Java 8. Without the bootclasspath, it takes 3.4 seconds on Java 8 and 4.5 seconds on Java 9. (This is just for one Clojure instance, not launching a project subprocess.) So in this case it nearly doubles the time, which is consistently in the top 5 complaints about working with Clojure.

I would not be surprised if other Clojure-based tools (IDEs/editors perhaps) would want to use the bootclasspath but I don't have any data on that. I know there are people using the bootclasspath for production servers, but they probably wouldn't be as upset about adding a few seconds as people using it on their own machine.

I would be happy to write the few lines of code needed to defer all references to the inaccessible classes until runtime if it's decided that's the way to go.

Show
Phil Hagelberg added a comment - > I think the first question here is: do we expect that Clojure should be loadable from the bootclasspath? I don't know if it's the right thing for Clojure to be able to be loaded from the bootclasspath, but as an additional point of data: Leiningen takes 2.5 seconds to load on my machine from the bootclasspath on Java 8. Without the bootclasspath, it takes 3.4 seconds on Java 8 and 4.5 seconds on Java 9. (This is just for one Clojure instance, not launching a project subprocess.) So in this case it nearly doubles the time, which is consistently in the top 5 complaints about working with Clojure. I would not be surprised if other Clojure-based tools (IDEs/editors perhaps) would want to use the bootclasspath but I don't have any data on that. I know there are people using the bootclasspath for production servers, but they probably wouldn't be as upset about adding a few seconds as people using it on their own machine. I would be happy to write the few lines of code needed to defer all references to the inaccessible classes until runtime if it's decided that's the way to go.
Hide
Ghadi Shayban added a comment -

Phil - would you mind sharing your environment and testing code? I see a slowdown without bootclasspath but not nearly as dramatic as what you see. The Java 9 builds you are using probably have extra debugging enabled.

Show
Ghadi Shayban added a comment - Phil - would you mind sharing your environment and testing code? I see a slowdown without bootclasspath but not nearly as dramatic as what you see. The Java 9 builds you are using probably have extra debugging enabled.
Hide
Phil Hagelberg added a comment -

Ghadi: I'm running on a Zulu build of Java 9 since I wasn't able to find binaries for Debian of the official OpenJDK ones. It's possible there's extra debugging overhead, but it's more likely it's just because I'm running on old hardware.

$ java -version
openjdk version "9-ea"
OpenJDK Runtime Environment (Zulu 9.0.0.10-linux64) (build 9-ea+155-pre-release-eval)
OpenJDK 64-Bit Server VM (Zulu 9.0.0.10-linux64) (build 9-ea+155, mixed mode, pre-release-eval)

Show
Phil Hagelberg added a comment - Ghadi: I'm running on a Zulu build of Java 9 since I wasn't able to find binaries for Debian of the official OpenJDK ones. It's possible there's extra debugging overhead, but it's more likely it's just because I'm running on old hardware. $ java -version openjdk version "9-ea" OpenJDK Runtime Environment (Zulu 9.0.0.10-linux64) (build 9-ea+155-pre-release-eval) OpenJDK 64-Bit Server VM (Zulu 9.0.0.10-linux64) (build 9-ea+155, mixed mode, pre-release-eval)
Hide
Alex Miller added a comment -

I'm moving this into Clojure 1.9. Not implying any decisions, just so we look at it before any 1.9 release decisions.

Show
Alex Miller added a comment - I'm moving this into Clojure 1.9. Not implying any decisions, just so we look at it before any 1.9 release decisions.
Hide
Alex Miller added a comment -

Would like to see a patch for this that makes loading/definition of things not in java.base conditional such that this would not fail. If someone wants to work on that, would be happy to see it. If not, I will look at it eventually.

Show
Alex Miller added a comment - Would like to see a patch for this that makes loading/definition of things not in java.base conditional such that this would not fail. If someone wants to work on that, would be happy to see it. If not, I will look at it eventually.
Hide
Ghadi Shayban added a comment - - edited

Adding a patch that conditionally loads clojure.instant

test with:

java -Xbootclasspath/a:$PWD/target/clojure-1.9.0-master-SNAPSHOT.jar:$HOME/.m2/repository/org/clojure/spec.alpha/0.1.123/spec.alpha-0.1.123.jar:$HOME/.m2/repository/org/clojure/core.specs.alpha/0.1.10/core.specs.alpha-0.1.10.jar clojure.main
Show
Ghadi Shayban added a comment - - edited Adding a patch that conditionally loads clojure.instant test with:
java -Xbootclasspath/a:$PWD/target/clojure-1.9.0-master-SNAPSHOT.jar:$HOME/.m2/repository/org/clojure/spec.alpha/0.1.123/spec.alpha-0.1.123.jar:$HOME/.m2/repository/org/clojure/core.specs.alpha/0.1.10/core.specs.alpha-0.1.10.jar clojure.main
Hide
Rich Hickey added a comment -

Could we please get a function or macro that encapsulates the check/catch so we don't have N of these to maintain should there eventually be a better way?

Show
Rich Hickey added a comment - Could we please get a function or macro that encapsulates the check/catch so we don't have N of these to maintain should there eventually be a better way?
Hide
Alex Miller added a comment -

Updated patch to encapsulate class existence check in "when-class" (and changed one existing usage of same pattern).

Show
Alex Miller added a comment - Updated patch to encapsulate class existence check in "when-class" (and changed one existing usage of same pattern).
Hide
Alex Miller added a comment -

Added -3 patch that applies cleanly to latest master.

Show
Alex Miller added a comment - Added -3 patch that applies cleanly to latest master.
Hide
Alex Miller added a comment -

whoops, swapped the data readers in -3. fixed in -4.

Show
Alex Miller added a comment - whoops, swapped the data readers in -3. fixed in -4.
Hide
Andrew Rosa added a comment -

Please correct me if I'm wrong, but as far as I can understand the issue is only related to code}}java.sql.Timestamp{{/code. The path removes the whole "instant.clj" file, but only a small portion of it is related to the problematic class.

Wouldn't be better to split it apart and conditionally load only the code}}java.sql.Timestamp{{/code portion of the code, like is already done with 1.8's code}}java.time.Instant{{/code on "code_instant18.clj"?

Show
Andrew Rosa added a comment - Please correct me if I'm wrong, but as far as I can understand the issue is only related to code}}java.sql.Timestamp{{/code. The path removes the whole "instant.clj" file, but only a small portion of it is related to the problematic class. Wouldn't be better to split it apart and conditionally load only the code}}java.sql.Timestamp{{/code portion of the code, like is already done with 1.8's code}}java.time.Instant{{/code on "code_instant18.clj"?
Hide
Alex Miller added a comment -

Hey Andrew, this is possible, and I went down this road a bit, but in the end I did not find the complexity of splitting it to be worth the effort. While you can work around the constructor-side of the reader parts as Timestamp is not used by default there, the Timestamp use is squarely in the middle of print support. A tagged literal that you can read but not print is of limited use so it seemed best to just omit the entire tag.

Show
Alex Miller added a comment - Hey Andrew, this is possible, and I went down this road a bit, but in the end I did not find the complexity of splitting it to be worth the effort. While you can work around the constructor-side of the reader parts as Timestamp is not used by default there, the Timestamp use is squarely in the middle of print support. A tagged literal that you can read but not print is of limited use so it seemed best to just omit the entire tag.
Hide
Alex Miller added a comment -

Released in 1.9.0-beta1.

Show
Alex Miller added a comment - Released in 1.9.0-beta1.

People

Vote (6)
Watch (7)

Dates

  • Created:
    Updated:
    Resolved: