Details
-
Type:
Defect
-
Status:
Open
-
Priority:
Major
-
Resolution: Unresolved
-
Affects Version/s: Release 1.7
-
Fix Version/s: Release 1.10
-
Component/s: None
-
Labels:
-
Patch:Code
-
Approval:Incomplete
Description
Scenario: Given two files:
src/dispatch/core.clj:
(ns dispatch.core (:require [dispatch.dispatch]))
src/dispatch/dispatch.clj:
(ns dispatch.dispatch)
(deftype T [])
(def t (->T))
(println "T = (class t):" (= T (class t)))
Compile first core, then dispatch:
java -cp src:target/classes:clojure.jar -Dclojure.compile.path=target/classes clojure.main user=> (compile 'dispatch.core) T = (class t): true dispatch.core user=> (compile 'dispatch.dispatch) T = (class t): false ;; expected true dispatch.dispatch
This scenario more commonly occurs in a leiningen project with :aot :all. Files are compiled in alphabetical order with :all. In this case, dispatch.core will be compiled first, then dispatch.dispatch.
Cause:
(compile 'dispatch.core)
- transitively compiles dispatch.dispatch
- writes .class files to compile-path (which is on the classpath)
- assertion passes
(compile 'dispatch.dispatch)
- due to prior compile, load dispatch.dispatch__init is loaded via the appclassloader
- ->T constructor will use new bytecode to instantiate a T instance - this uses appclassloader, loaded from compiled T on disk
- however, T class literals are resolved with RT.classForName, which checks the dynamic classloader cache, so uses old runtime version of T, instead of on-disk version
In 1.6, RT.classForName() did not check dynamic classloader cache, so loaded T from disk as with instances. This was changed in CLJ-979 to support other redefinition and AOT mixing usages.
Approaches:
1) Compile in reverse dependency order to avoid compiling twice.
Either swap the order of compilation in the first example or specify the order in project.clj:
:aot [dispatch.dispatch dispatch.core]
This is a short-term workaround.
2) Move the deftype into a separate namespace from where it is used so it is not redefined on the second compile. This is another short-term workaround.
3) Do not put compile-path on the classpath (this violates current expectations, but avoids loading dispatch__init)
(set! *compile-path* "foo")
(compile 'dispatch.core)
(compile 'dispatch.dispatch)
This is not easy to set up via Leiningen currently.
4) Compile each file with an independent Clojure runtime - avoids using cached classes in DCL for class literals.
Probably too annoying to actually do right now in Leiningen or otherwise.
5) Make compilation non-transitive. This is in the ballpark of CLJ-322, which is another can of worms. Also possibly where we should be headed though.
Screening: I do not believe the proposed patch is a good idea - it papers over the symptom without addressing the root cause. I think we need to re-evaluate how compilation works with regard to compile-path (#3) and transitivity (CLJ-322) (#5), but I think we should do this after 1.7. - Alex
See also: CLJ-1650
Attachments
Activity
Field | Original Value | New Value |
---|---|---|
Fix Version/s | Release 1.7 [ 10250 ] | |
Approval | Vetted [ 10003 ] |
Labels | classloader compiler, |
Labels | classloader compiler, | classloader compiler |
Approval | Vetted [ 10003 ] | Incomplete [ 10006 ] |
Labels | classloader compiler | aot classloader compiler |
Attachment | 0001-CLJ-1714-Don-t-load-AOT-class-when-compiling-already.patch [ 14218 ] | |
Patch | Code [ 10001 ] |
Attachment | 0001-CLJ-1714-Don-t-load-AOT-class-when-compiling-already-v2.patch [ 14219 ] |
Attachment |
0001- |
Comment | [ Alternative patch that uses Classloader.findLoadeClass rather than keeping a set of loaded class names ] |
Approval | Incomplete [ 10006 ] | Vetted [ 10003 ] |
Approval | Vetted [ 10003 ] | Incomplete [ 10006 ] |
Summary | Type-dispatching multimethods are defined using the wrong type instance | deftype class literals and instances loaded from different classloaders when recompiling namespace |
Description |
When using a multimethod that dispatches on types, such as print-dup/print-method, the type reference passed to {{addMethod}} in the presence of aot is incorrect on the second load of the namespace. This means that if the namespace has already been loaded as a dependency of another file, the second load when the namespace is loaded for aot compilation will produce a multimethod that fails to dispatch correctly.
I've created an example repository: https://github.com/sfnelson/clj-mm-dispatch To reproduce independently, create a namespace that contains a deftype and a multimethod dispatching on the type, and a second namespace that requires the first and sorts alphabetically before the first. Aot-compile both namespaces. When the type-defining namespace is loaded via {{require}} it produces a class file for the deftype. When it is loaded the second time for aot-compilation, the type corresponding to the existing class file is given to the defmethod, instead of the new class constructed by loading the namespace. This causes the multimethod it fail to dispatch correctly. To me this issue seems similar to |
*Scenario:* Given two files:
src/dispatch/core.clj: {code} (ns dispatch.core (:require [dispatch.dispatch])) {code} src/dispatch/dispatch.clj: {code} (ns dispatch.dispatch) (deftype T []) (def t (->T)) (println "T = (class t):" (= T (class t))) {code} Compile first core, then dispatch: {code} java -cp src:target/classes:clojure.jar -Dclojure.compile.path=target/classes clojure.main user=> (compile 'dispatch.core) T = (class t): true dispatch.core user=> (compile 'dispatch.dispatch) T = (class t): false ;; expected true dispatch.dispatch {code} This scenario more commonly occurs in a leiningen project with {{:aot :all}}. Files are compiled in alphabetical order with :all. In this case, dispatch.core will be compiled first, then dispatch.dispatch. *Cause:* (compile 'dispatch.core) - transitively compiles dispatch.dispatch - writes .class files to *compile-path* (which is on the classpath) - assertion passes (compile 'dispatch.dispatch) - due to prior compile, load dispatch.dispatch__init is loaded via the appclassloader - ->T constructor will use new bytecode to instantiate a T instance - this uses appclassloader, loaded from compiled T on disk - however, T class literals are resolved with RT.classForName, which checks the dynamic classloader cache, so uses old runtime version of T, instead of on-disk version In 1.6, RT.classForName() did not check dynamic classloader cache, so loaded T from disk as with instances. This was changed in *Approaches:* 1) Compile in reverse dependency order to avoid compiling twice. Either swap the order of compilation in the first example or specify the order in project.clj: {code} :aot [dispatch.dispatch dispatch.core] {code} This is a short-term workaround. 2) Do not put compile-path on the classpath (this violates current expectations, but avoids loading dispatch__init) {code} (set! *compile-path* "foo") (compile 'dispatch.core) (compile 'dispatch.dispatch) {code} This is not easy to set up via Leiningen currently. 3) Compile each file with an independent Clojure runtime - avoids using cached classes in DCL for class literals. Probably too annoying to actually do right now in Leiningen or otherwise. 4) Make compilation non-transitive. This is in the ballpark of *Screening:* I do not believe the proposed patch is a good idea - it papers over the symptom without addressing the root cause. I think we need to re-evaluate how compilation works with regard to compile-path (#2) and transitivity ( |
Comment |
[ Summarizing and simplifying some of this a bit more (mostly for my own use):
h3. The setup dispatch.core {code} (ns dispatch.core (:require [dispatch.dispatch])) {code} dispatch.dispatch {code} (ns dispatch.dispatch) (deftype T []) (def t (->T)) (assert (= T (class t))) ;; pass or fail? {code} h3. What happens (compile 'dispatch.core) - transitively compiles dispatch.dispatch - writes .class files to *compile-path* (which is on the classpath) - assertion passes (compile 'dispatch.dispatch) - due to prior compile, load dispatch.dispatch__init is loaded via the appclassloader - ->T constructor will use new bytecode to instantiate a T instance - this uses appclassloader, loaded from compiled T on disk - T class literals are resolved with RT.classForName - in 1.6: RT.classForName() did not check dynamic classloader cache, so loads new compiled T from disk and matches the instance - in 1.7: RT.classForName() *does* check the dynamic classloader cache, so uses old runtime version of T, instead of on-disk version h3. Workarounds 1) Compile in reverse dependency order to avoid compiling twice {code} :aot [dispatch.dispatch dispatch.core] ;; in project.clj {code} This is a valid short-term workaround. 2) Do not put compile-path on the classpath (this violates current instructions, but avoids loading dispatch__init) {code} (set! *compile-path* "foo") (compile 'dispatch.core) (compile 'dispatch.dispatch) {code} I am of the mind that this is the direction we should be headed (eventually). This is not easy to set up via Leiningen currently. 3) Compile each file with an independent Clojure runtime - avoids using cached classes in DCL for class literals. Probably too annoying to do right now. 4) Make compile non-transitive. This is in the ballpark of ] |
Description |
*Scenario:* Given two files:
src/dispatch/core.clj: {code} (ns dispatch.core (:require [dispatch.dispatch])) {code} src/dispatch/dispatch.clj: {code} (ns dispatch.dispatch) (deftype T []) (def t (->T)) (println "T = (class t):" (= T (class t))) {code} Compile first core, then dispatch: {code} java -cp src:target/classes:clojure.jar -Dclojure.compile.path=target/classes clojure.main user=> (compile 'dispatch.core) T = (class t): true dispatch.core user=> (compile 'dispatch.dispatch) T = (class t): false ;; expected true dispatch.dispatch {code} This scenario more commonly occurs in a leiningen project with {{:aot :all}}. Files are compiled in alphabetical order with :all. In this case, dispatch.core will be compiled first, then dispatch.dispatch. *Cause:* (compile 'dispatch.core) - transitively compiles dispatch.dispatch - writes .class files to *compile-path* (which is on the classpath) - assertion passes (compile 'dispatch.dispatch) - due to prior compile, load dispatch.dispatch__init is loaded via the appclassloader - ->T constructor will use new bytecode to instantiate a T instance - this uses appclassloader, loaded from compiled T on disk - however, T class literals are resolved with RT.classForName, which checks the dynamic classloader cache, so uses old runtime version of T, instead of on-disk version In 1.6, RT.classForName() did not check dynamic classloader cache, so loaded T from disk as with instances. This was changed in *Approaches:* 1) Compile in reverse dependency order to avoid compiling twice. Either swap the order of compilation in the first example or specify the order in project.clj: {code} :aot [dispatch.dispatch dispatch.core] {code} This is a short-term workaround. 2) Do not put compile-path on the classpath (this violates current expectations, but avoids loading dispatch__init) {code} (set! *compile-path* "foo") (compile 'dispatch.core) (compile 'dispatch.dispatch) {code} This is not easy to set up via Leiningen currently. 3) Compile each file with an independent Clojure runtime - avoids using cached classes in DCL for class literals. Probably too annoying to actually do right now in Leiningen or otherwise. 4) Make compilation non-transitive. This is in the ballpark of *Screening:* I do not believe the proposed patch is a good idea - it papers over the symptom without addressing the root cause. I think we need to re-evaluate how compilation works with regard to compile-path (#2) and transitivity ( |
*Scenario:* Given two files:
src/dispatch/core.clj: {code} (ns dispatch.core (:require [dispatch.dispatch])) {code} src/dispatch/dispatch.clj: {code} (ns dispatch.dispatch) (deftype T []) (def t (->T)) (println "T = (class t):" (= T (class t))) {code} Compile first core, then dispatch: {code} java -cp src:target/classes:clojure.jar -Dclojure.compile.path=target/classes clojure.main user=> (compile 'dispatch.core) T = (class t): true dispatch.core user=> (compile 'dispatch.dispatch) T = (class t): false ;; expected true dispatch.dispatch {code} This scenario more commonly occurs in a leiningen project with {{:aot :all}}. Files are compiled in alphabetical order with :all. In this case, dispatch.core will be compiled first, then dispatch.dispatch. *Cause:* (compile 'dispatch.core) - transitively compiles dispatch.dispatch - writes .class files to *compile-path* (which is on the classpath) - assertion passes (compile 'dispatch.dispatch) - due to prior compile, load dispatch.dispatch__init is loaded via the appclassloader - ->T constructor will use new bytecode to instantiate a T instance - this uses appclassloader, loaded from compiled T on disk - however, T class literals are resolved with RT.classForName, which checks the dynamic classloader cache, so uses old runtime version of T, instead of on-disk version In 1.6, RT.classForName() did not check dynamic classloader cache, so loaded T from disk as with instances. This was changed in *Approaches:* 1) Compile in reverse dependency order to avoid compiling twice. Either swap the order of compilation in the first example or specify the order in project.clj: {code} :aot [dispatch.dispatch dispatch.core] {code} This is a short-term workaround. 2) Move the deftype into a separate namespace from where it is used so it is not redefined on the second compile. This is another short-term workaround. 3) Do not put compile-path on the classpath (this violates current expectations, but avoids loading dispatch__init) {code} (set! *compile-path* "foo") (compile 'dispatch.core) (compile 'dispatch.dispatch) {code} This is not easy to set up via Leiningen currently. 4) Compile each file with an independent Clojure runtime - avoids using cached classes in DCL for class literals. Probably too annoying to actually do right now in Leiningen or otherwise. 5) Make compilation non-transitive. This is in the ballpark of *Screening:* I do not believe the proposed patch is a good idea - it papers over the symptom without addressing the root cause. I think we need to re-evaluate how compilation works with regard to compile-path (#3) and transitivity ( |
Fix Version/s | Release 1.8 [ 10254 ] | |
Fix Version/s | Release 1.7 [ 10250 ] |
Description |
*Scenario:* Given two files:
src/dispatch/core.clj: {code} (ns dispatch.core (:require [dispatch.dispatch])) {code} src/dispatch/dispatch.clj: {code} (ns dispatch.dispatch) (deftype T []) (def t (->T)) (println "T = (class t):" (= T (class t))) {code} Compile first core, then dispatch: {code} java -cp src:target/classes:clojure.jar -Dclojure.compile.path=target/classes clojure.main user=> (compile 'dispatch.core) T = (class t): true dispatch.core user=> (compile 'dispatch.dispatch) T = (class t): false ;; expected true dispatch.dispatch {code} This scenario more commonly occurs in a leiningen project with {{:aot :all}}. Files are compiled in alphabetical order with :all. In this case, dispatch.core will be compiled first, then dispatch.dispatch. *Cause:* (compile 'dispatch.core) - transitively compiles dispatch.dispatch - writes .class files to *compile-path* (which is on the classpath) - assertion passes (compile 'dispatch.dispatch) - due to prior compile, load dispatch.dispatch__init is loaded via the appclassloader - ->T constructor will use new bytecode to instantiate a T instance - this uses appclassloader, loaded from compiled T on disk - however, T class literals are resolved with RT.classForName, which checks the dynamic classloader cache, so uses old runtime version of T, instead of on-disk version In 1.6, RT.classForName() did not check dynamic classloader cache, so loaded T from disk as with instances. This was changed in *Approaches:* 1) Compile in reverse dependency order to avoid compiling twice. Either swap the order of compilation in the first example or specify the order in project.clj: {code} :aot [dispatch.dispatch dispatch.core] {code} This is a short-term workaround. 2) Move the deftype into a separate namespace from where it is used so it is not redefined on the second compile. This is another short-term workaround. 3) Do not put compile-path on the classpath (this violates current expectations, but avoids loading dispatch__init) {code} (set! *compile-path* "foo") (compile 'dispatch.core) (compile 'dispatch.dispatch) {code} This is not easy to set up via Leiningen currently. 4) Compile each file with an independent Clojure runtime - avoids using cached classes in DCL for class literals. Probably too annoying to actually do right now in Leiningen or otherwise. 5) Make compilation non-transitive. This is in the ballpark of *Screening:* I do not believe the proposed patch is a good idea - it papers over the symptom without addressing the root cause. I think we need to re-evaluate how compilation works with regard to compile-path (#3) and transitivity ( |
*Scenario:* Given two files:
src/dispatch/core.clj: {code} (ns dispatch.core (:require [dispatch.dispatch])) {code} src/dispatch/dispatch.clj: {code} (ns dispatch.dispatch) (deftype T []) (def t (->T)) (println "T = (class t):" (= T (class t))) {code} Compile first core, then dispatch: {code} java -cp src:target/classes:clojure.jar -Dclojure.compile.path=target/classes clojure.main user=> (compile 'dispatch.core) T = (class t): true dispatch.core user=> (compile 'dispatch.dispatch) T = (class t): false ;; expected true dispatch.dispatch {code} This scenario more commonly occurs in a leiningen project with {{:aot :all}}. Files are compiled in alphabetical order with :all. In this case, dispatch.core will be compiled first, then dispatch.dispatch. *Cause:* (compile 'dispatch.core) - transitively compiles dispatch.dispatch - writes .class files to *compile-path* (which is on the classpath) - assertion passes (compile 'dispatch.dispatch) - due to prior compile, load dispatch.dispatch__init is loaded via the appclassloader - ->T constructor will use new bytecode to instantiate a T instance - this uses appclassloader, loaded from compiled T on disk - however, T class literals are resolved with RT.classForName, which checks the dynamic classloader cache, so uses old runtime version of T, instead of on-disk version In 1.6, RT.classForName() did not check dynamic classloader cache, so loaded T from disk as with instances. This was changed in *Approaches:* 1) Compile in reverse dependency order to avoid compiling twice. Either swap the order of compilation in the first example or specify the order in project.clj: {code} :aot [dispatch.dispatch dispatch.core] {code} This is a short-term workaround. 2) Move the deftype into a separate namespace from where it is used so it is not redefined on the second compile. This is another short-term workaround. 3) Do not put compile-path on the classpath (this violates current expectations, but avoids loading dispatch__init) {code} (set! *compile-path* "foo") (compile 'dispatch.core) (compile 'dispatch.dispatch) {code} This is not easy to set up via Leiningen currently. 4) Compile each file with an independent Clojure runtime - avoids using cached classes in DCL for class literals. Probably too annoying to actually do right now in Leiningen or otherwise. 5) Make compilation non-transitive. This is in the ballpark of *Screening:* I do not believe the proposed patch is a good idea - it papers over the symptom without addressing the root cause. I think we need to re-evaluate how compilation works with regard to compile-path (#3) and transitivity ( See also: |
Fix Version/s | Release 1.9 [ 10750 ] | |
Fix Version/s | Release 1.8 [ 10254 ] |
Fix Version/s | Release 1.9 [ 10750 ] | |
Fix Version/s | Release Next [ 11451 ] |
Pulling into 1.7 for consideration.