<< Back to previous view

[CLJ-1741] deftype class literals and instances loaded from different classloaders when recompiling namespace Created: 30/May/15  Updated: 18/Jun/15

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.8

Type: Defect Priority: Major
Reporter: Stephen Nelson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: aot, classloader, compiler

Attachments: Text File 0001-CLJ-1714-Don-t-load-AOT-class-when-compiling-already.patch    
Patch: Code
Approval: Incomplete


Scenario: Given two files:


(ns dispatch.core (:require [dispatch.dispatch]))


(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
user=> (compile 'dispatch.dispatch)
T = (class t): false     ;; expected true

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.


(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.


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

Comment by Alex Miller [ 30/May/15 8:50 PM ]

Pulling into 1.7 for consideration.

Comment by Stephen Nelson [ 30/May/15 8:55 PM ]

I've added a debug flag to my example that causes type instance hashcodes and their class-loaders to be printed.

Compiling dispatch.core
deftype => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
defmethod => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
instance => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
dispatch:  :pass
Compiling dispatch.dispatch
deftype => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
defmethod => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
instance => 760357227 (sun.misc.Launcher$AppClassLoader@42a57993)
dispatch:  :fail
Comment by Nicola Mometto [ 01/Jun/15 7:23 AM ]

The compiler has weird loading rules when using `compile` and both a clj file and a class file are present in the classpath.

This bug happens because RT.load will load the AOT class file rebinding the ->Ctor to use the AOT deftype instance.

A fix for this would be making load "loaded libs" aware to avoid unnecessary/harmful reloadings.

Comment by Nicola Mometto [ 01/Jun/15 10:55 AM ]

The attached patch fixes this bug by keeping track of what has already been loaded and loading the AOT class only if necessary

Comment by Alex Miller [ 16/Jun/15 2:24 PM ]

Original description (since replaced):

Type-dispatching multimethods are defined using the wrong type instance

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:

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 CLJ-979: the type passed to the multimethod is retrieved using the wrong classloader. This suggests that it might have wider implications than AOT and multimethod dispatch.

Comment by Nicola Mometto [ 18/Jun/15 11:09 AM ]

I just realized this ticket is a duplicate of CLJ-1650

[CLJ-1663] DynamicClassLoader delegates to parent classloader before checking in its URL list Created: 18/Feb/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: classloader, regression

Attachments: Text File 0001-CLJ-1663-delegate-loadClass-to-super-classloader-bef.patch    
Patch: Code
Approval: Ok


See Cursive #748. Cursive calls into Leiningen in-process, and before doing that it creates a new DynamicClassLoader which uses an IntelliJ PluginClassLoader as its parent. This is throwing a CNFE, although the URL containing the class is present in the DynamicClassLoader URL list.

Cause: The patch for CLJ-979 added an implementation of loadClass that delegates to "getParent().loadClass()", which in this case delegates to PluginClassLoader.loadClass(). This was incorrect as the current implementation should have been to delegate to the loadClass method of the superclass, which will take care of delegating to the loadClass method of the parent class loader if necessary.

Approach: The proposed patch replaces the call to "getParent().loadClass()" with a call "super.loadClass()" fixing this issue.

Screened by: Alex Miller

Comment by Colin Fleming [ 18/Feb/15 6:25 AM ]

Unfortunately getClassLoadingLock(name) is only available from Java 1.7+ and I am targeting Java 1.6. However reverting that part of the patch to synchronize on "this" as previously does indeed fix the original problem.

Comment by Nicola Mometto [ 18/Feb/15 7:22 AM ]

I'll revert the getClassLoadingLock change then, it was actually out of scope for this ticket.

[CLJ-1550] Classes generated by deftype and defrecord don't play nice with .getPackage Created: 07/Oct/14  Updated: 22/Jul/15

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6, Release 1.7, Release 1.8
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 15
Labels: classloader, deftype

Attachments: Text File 0001-CLJ-1550-define-package-for-class-in-DynamicClassLoa.patch     Text File CLJ-1550-v2.patch    
Patch: Code and Test


Classes generated loaded by DynamicClassLoader return nil for .getPackage

(.getPackage String)
;; => #<Package package java.lang, Java Platform API Specification, version 1.7>
(deftype T [])
(.getPackage T)
;; => nil

This seems like a bug to me as it's not obvious why the class generated by deftype should exhibit different behaviour.

Patch: CLJ-1550-v2.patch

Comment by Alex Miller [ 07/Oct/14 8:54 AM ]

According to http://docs.oracle.com/javase/7/docs/api/java/lang/Class.html#getPackage() this method returns the package information found by the class loader or null if there is none. Its not clear to me that the current behavior is wrong per the spec. I would need to experiment more to see if this is unusual or not.

Comment by Bozhidar Batsov [ 07/Oct/14 9:05 AM ]

A bit of background for the issue. I'm no expert on the topic, but being able to procure all the class information except its package definitely looks strange to me.

Comment by Kevin Downey [ 07/Oct/14 11:46 AM ]

if you AOT compile(generate a class file on disk for a deftype), getPackage works fine, which suggests to me it is a jvm issue

Comment by Kevin Downey [ 07/Oct/14 11:49 AM ]

actually, it must just be that dynamicclassloader doesn't define a package for classes it loads

Comment by Alex Miller [ 07/Oct/14 12:13 PM ]

Yep, I believe that's correct.

Comment by Stuart Halloway [ 21/Jul/15 8:01 AM ]

There is no problem statement here. What is package information needed for?

Comment by Bozhidar Batsov [ 21/Jul/15 8:05 AM ]

I've linked the problem above. Basically tools like CIDER and vim-fireplace are relying on this information to implement things like completion hints.
This might not be problem when running your apps, but it's definitely a problem when inspecting their state...

Comment by Michael Blume [ 22/Jul/15 12:32 PM ]


[CLJ-1457] once the compiler pops the dynamic classloader from the stack, attempts to read record reader literals will fail Created: 30/Jun/14  Updated: 09/Jan/15  Resolved: 09/Jan/15

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Duplicate Votes: 6
Labels: classloader

Attachments: Text File 0001-CLJ-1457-ensure-Compiler.LOADER-is-bound-while-readi.patch    
Patch: Code
Approval: Triaged


reproduction case

java -jar target/clojure-1.7.0-master-SNAPSHOT.jar -e "(do (ns foo.bar) (defrecord Foo []) (defn -main [] (prn (->Foo)) (read-string \"#foo.bar.Foo[]\")))" -m foo.bar


Exception in thread "main" java.lang.ClassNotFoundException: foo.bar.Foo
	at java.net.URLClassLoader$1.run(URLClassLoader.java:372)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:361)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.net.URLClassLoader.findClass(URLClassLoader.java:360)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:308)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:340)
	at clojure.lang.RT.classForNameNonLoading(RT.java:2076)
	at clojure.lang.LispReader$CtorReader.readRecord(LispReader.java:1195)
	at clojure.lang.LispReader$CtorReader.invoke(LispReader.java:1164)
	at clojure.lang.LispReader$DispatchReader.invoke(LispReader.java:609)
	at clojure.lang.LispReader.read(LispReader.java:183)
	at clojure.lang.RT.readString(RT.java:1737)
	at clojure.core$read_string.invoke(core.clj:3497)
	at foo.bar$_main.invoke(NO_SOURCE_FILE:1)
	at clojure.lang.Var.invoke(Var.java:375)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.core$apply.invoke(core.clj:624)
	at clojure.main$main_opt.invoke(main.clj:315)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at clojure.lang.Var.invoke(Var.java:394)
	at clojure.lang.AFn.applyToHelper(AFn.java:165)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)

what happens is the evaluator pushes a dynamicclassloader, evaluates some code, then -m foo.bar causes foo.bar/-main to be called, which tries to read in a literal for the just defined record, but it fails because when foo.bar/-main is called clojure.lang.Compiler/LOADER is unbound so RT uses the sun.misc classloader to try and find the class, which it knows nothing about

Approaches: If the patch 0001-CLJ-979-make-clojure-resolve-to-the-correct-Class-in-v2.patch for CLJ-979 were to be committed, this issue would be automatically fixed aswell and the patch attached to this ticket would be unnecessary.
Alternatively, the attached patch (0001-CLJ-1457-ensure-Compiler.LOADER-is-bound-while-readi.patch) simply forces a DynamicClassLoader to be bound to clojure.lang.Compiler/LOADER during reading.

Comment by Kevin Downey [ 01/Jul/14 11:42 AM ]

this means that you cannot depend on ever being able to deserialize a record with read unless you are at the repl (the only place clojure.lang.Compiler/LOADER is guaranteed to be bound).

1. print/read support for records is broken
2. behavior is inconsistent between the repl and other environments
which will drive people crazy when the try to figure out why their code isn't working

Comment by Alex Miller [ 01/Jul/14 4:43 PM ]

I would appreciate more understanding about how this affects code run in a more normal scenario (than calling clojure.main with -e and -m).

Comment by Kevin Downey [ 22/Aug/14 4:24 PM ]

https://gist.githubusercontent.com/anonymous/bafde69c99e0be63988d/raw/736d14d98030f48b6a65ca0bfdc3c81fb44e1789/gistfile1.txt is an hour long irc log where someone was having a problem after they switched their app from aot compilation to launch via -m, which I tracked down to this issue.

Comment by David Pidcock [ 27/Oct/14 8:24 PM ]

This also affects me running
lein ring server

I threw this test code in my handler.clj

(defrecord Test [id])

(def example-test (pr-str (->Test 1)))

(defroutes app-routes
(GET "/test" [] (read-string example-test)))

When I run this in the REPL, it works.
But I get ClassNotFound when I browse to "/test".

I thought I'd done something stupid, until I found this bug. (Well - I could still be doing something stupid, of course).

Comment by Michael Fogleman [ 03/Nov/14 11:04 AM ]

Nicola Mometto helpfully pointed out that CLJ-1413 seems to be the same issue as this.


Comment by Michael Fogleman [ 13/Nov/14 8:29 AM ]

In the other issue, Lars Bohl has a reproducible example of a very simple or even simplest possible case.

Comment by Alex Miller [ 04/Dec/14 12:46 PM ]

Possibly related: CLJ-1544

Comment by Nicola Mometto [ 04/Dec/14 2:31 PM ]

The patch 0001-CLJ-979-make-clojure-resolve-to-the-correct-Class-in-v2.patch from CLJ-979 contains a workaround that fixes this bug.

Comment by Nicola Mometto [ 06/Dec/14 6:26 PM ]

For completeness here's an alternative patch to the one proposed for CLJ-979 that fixes this issue by ensuring Compiler/LOADER is bound to a DynamicClassLoader while reading

Comment by Alex Miller [ 09/Jan/15 10:54 AM ]

Since CLJ-979 is headed into 1.7.0-alpha5, I'm duping this to that.

[CLJ-979] Clojure resolves to wrong deftype classes when AOT compiling or reloading Created: 03/May/12  Updated: 18/Feb/15  Resolved: 10/Jan/15

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3, Release 1.4, Release 1.5, Release 1.6, Release 1.7
Fix Version/s: Release 1.7

Type: Defect Priority: Critical
Reporter: Edmund Jackson Assignee: Unassigned
Resolution: Completed Votes: 14
Labels: aot, classloader, compiler

Attachments: Text File CLJ-979.patch     Text File clj-979-symptoms.patch     Text File CLJ-979-v2.patch     Text File CLJ-979-v3.patch     Text File CLJ-979-v4.patch     Text File CLJ-979-v5.patch     Text File CLJ-979-v6.patch     Text File CLJ-979-v7.patch    
Patch: Code and Test
Approval: Ok


Compiling a class via `deftype` during AOT compilation gives different results for the different constructors. These hashes should be identical.

user=> (binding [*compile-files* true] (eval '(deftype Abc [])))
user=> (hash Abc)
user=> (hash (class (->Abc)))
31966239 ;; should be 16446700

This also means that whenever there's a stale AOT compiled deftype class in the classpath, that class will be used rather then the JIT compiled one, breaking repl interaction.

Another demonstration of this classloader issue (from CLJ-1495) when reloading deftypes (no AOT) :

user> (defrecord Foo [bar])
user> (= (->Foo 42) #user.Foo{:bar 42}) ;;expect this to evaluate to true
user> (defrecord Foo [bar])
user> (= (->Foo 42) #user.Foo{:bar 42}) ;;expect this to evaluate to true also -- but it doesn't!

This bug also affects AOT compilation of multimethods that dispatch on a class, this affected core.match for years see http://dev.clojure.org/jira/browse/MATCH-86, http://dev.clojure.org/jira/browse/MATCH-98. David had to work-around this issue by using a bunch of protocols instead of multimethods.

Cause of the bug: currently clojure uses Class.forName to resolve a class from a class name, which ignores the class cache from DynamicClassLoader thus reloading deftypes or mixing AOT compilation at the repl with deftypes breaks, resolving to the wrong class.

Approach: the current patch (CLJ-979-v7.patch) addresses this issue in multiple ways:

  • it makes RT.classForName/classForNameNonLoading look in the class cache before delegating to Class/forName if the current classloader is not a DynamicClassLoader (this incidentally addresses also CLJ-1457)
  • it makes clojure use RT.classForName/classForNameNonLoading instead of Class/forName
  • it overrides Classloader/loadClass so that it's class cache aware – this method is used by the jvm to load classes
  • it changes gen-interface to always emit an in-memory interface along with the [optional] on-disk interface so that the in-memory class is always updated.

Patch: CLJ-979-v7.patch

Screened by: Alex Miller

Comment by Scott Lowe [ 12/May/12 9:05 PM ]

I can't reproduce this under Clojure 1.3 or 1.4, and Leiningen 1.7.1 on either Java 1.7.0-jdk7u4-b21 OpenJDK 64-Bit or Java 1.6.0_31 Java HotSpot 64-Bit. OS is Mac OS X 10.7.

Edmund, how are you running this AOT code? I wrapped your code in a main function and built an uberjar from it.

Comment by Edmund Jackson [ 13/May/12 2:20 AM ]

Hi Scott,


I have two use cases
1. AOT compile and call from repl.
My steps: git clone, lein compile, lein repl, (use 'aots.death), (in-ns 'aots.death), (= (class (Dontwork. nil)) (class (map->Dontwork {:a 1}))) => false

2. My original use case, which I've minimised here, is an AOT ns, producing a genclass that is called instantiated from other Java (no main). This produces the same error. I will produce an example of this and post it too.

Comment by Edmund Jackson [ 13/May/12 4:23 AM ]

Hi Scott,

Here is an example of it failing in the interop case: https://github.com/ejackson/aotquestion2
The steps I'm following to compile this all up are

git clone git@github.com:ejackson/aotquestion2.git
cd aotquestion2/cljside/
lein uberjar
lein install
cd ../javaside/
mvn package
java -jar ./target/aotquestion-1.0-SNAPSHOT.jar

and it dies with this:

Exception in thread "main" java.lang.ClassCastException: cljside.core.Dontwork cannot be cast to cljside.core.Dontwork
at cljside.MyClass.makeDontwork(Unknown Source)
at aotquestion.App.main(App.java:8)

The error message is really confusing (to me, anyway), but I think its the same root problem as for the REPL case.

What do you see when you run the above ?

Comment by Scott Lowe [ 13/May/12 8:41 AM ]

Ah, yes, looks like my initial attempt to reproduce was too simplistic. I used your second git repo, and can now confirm that it's failing for me with the same error.

Comment by Scott Lowe [ 13/May/12 10:35 PM ]

I looked into this a little further and the AOT generated code looks correct, in the sense that both code paths appear to be returning the same type.

However, I wonder if this is really a ClassLoader issue, whereby two definitions of the same class are being loaded at different times, because that would cause the x.y.Class cannot be cast to x.y.Class exception that we're seeing here.

Comment by Steve Miner [ 03/Sep/13 9:54 AM ]

This could be related to CLJ-1157 which deals with a ClassLoader issue with AOT compiled code.

Comment by Ambrose Bonnaire-Sergeant [ 29/Mar/14 1:11 PM ]

I've tried this patch attached to CLJ-1157 and it did not solve this issue.

Comment by Ambrose Bonnaire-Sergeant [ 29/Mar/14 2:27 PM ]

This bug seems to be rooted in different behaviour for do/let under compilation. Attached a patch showing these symptoms in the hope it helps people find the cause.

Comment by Peter Taoussanis [ 22/Sep/14 3:12 AM ]

Just a quick note to confirm that this still seems to be around as of Clojure 1.7.0-alpha2. Don't have any useful input on possible solutions, sorry.

Comment by Alex Miller [ 04/Dec/14 1:12 PM ]

Duplicates - CLJ-1495, CLJ-1132

Comment by Nicola Mometto [ 04/Dec/14 1:50 PM ]

The attached patch fixes the classloader issues by routing RT.classForName & variants through the DynamicClassLoader class cache before invoking Class.forName

Comment by Nicola Mometto [ 04/Dec/14 1:59 PM ]

Re-adding triaged status added by Alex Miller that got accidentaly nuked by a race-condition between my edits to the ticket description and Alex's ones

Comment by Nicola Mometto [ 04/Dec/14 2:30 PM ]

0001-CLJ-979-make-clojure-resolve-to-the-correct-Class-in-v2.patch is the same as 0001-CLJ-979-make-clojure-resolve-to-the-correct-Class-in.patch except it unconditionally looks for classes in the class cache of DynamicClassLoader, even if baseLoader() is not a DynamicClassLoader.
This fixes the bug of CLJ-1457 but might just be a workaround

Comment by Michael Blume [ 11/Dec/14 3:29 PM ]

Current patch blows up my Clojure build


Comment by Nicola Mometto [ 11/Dec/14 3:45 PM ]

Michael: the current patch builds clojure fine for me, I'll try to reproduce. Which jvm version are you using?

Comment by Michael Blume [ 11/Dec/14 4:26 PM ]

[14:24][michael.blume@tcc-michael-4:~/workspace/clojure((0fc43db...))]$ java -version
java version "1.8.0_25"
Java(TM) SE Runtime Environment (build 1.8.0_25-b17)
Java HotSpot(TM) 64-Bit Server VM (build 25.25-b02, mixed mode)
[14:24][michael.blume@tcc-michael-4:~/workspace/clojure((0fc43db...))]$ mvn -version
Apache Maven 3.2.3 (33f8c3e1027c3ddde99d3cdebad2656a31e8fdf4; 2014-08-11T13:58:10-07:00)
Maven home: /usr/local/Cellar/maven/3.2.3/libexec
Java version: 1.8.0_25, vendor: Oracle Corporation
Java home: /Library/Java/JavaVirtualMachines/jdk1.8.0_25.jdk/Contents/Home/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "10.10.1", arch: "x86_64", family: "mac"

build was after I applied the patch to the current master branch of the clojure github repo

Comment by Andy Fingerhut [ 11/Dec/14 5:34 PM ]

I am seeing a similar compilation error as Michael Blume, with both JDK 1.7 and 1.8 on Mac OS X 10.9.5.

By accident I found that if I take latest Clojure master and do 'mvn package', then apply the patch CLJ-979.patch dated Dec 11 2014, then do 'mvn package' again without 'mvn clean', it compiles with no errors. If I do 'mvn clean' then 'mvn package' in a patched tree, I get the error every time I've tried.

Comment by Nicola Mometto [ 12/Dec/14 5:50 AM ]

The updated patch fixes the LinkageError Andy and Michael were getting.

Andy, Michael, can you confirm?

Comment by Nicola Mometto [ 12/Dec/14 9:38 AM ]

Added more testcases to new patch

Comment by Nicola Mometto [ 12/Dec/14 10:09 AM ]

Cleaned up the patch from whitespace changes

Comment by Andy Fingerhut [ 12/Dec/14 12:32 PM ]

I tried latest Clojure master plus patch CLJ-979-v4.patch, dated 12 Dec 2014, with Mac OS X 10.9.5 + JDK7, and Ubuntu Linux 14.04 with JDKs 6 through 9, and 'mvn clean' followed by 'mvn package' built and passed tests successfully with all of them.

I did notice that some files were created in the test directory that were not cleaned up by the end of the test, which you can use 'git status .' to see. Not sure if that is considered a bad thing for Clojure tests.

Comment by Nicola Mometto [ 12/Dec/14 1:07 PM ]

Thanks Andy, I've updated the patch and it now should remove all temporary classes created by the test.
It's probably not the best way to do it but I couldn't figure out how to do it another way.

Comment by Michael Blume [ 12/Dec/14 2:34 PM ]

Yep, looks good to me =)

Comment by Alex Miller [ 15/Dec/14 4:01 PM ]

Thanks first to Nicola for all his work so far on this!

Some feedback:
1) While the ticket itself isn't bad, I would really like to focus the title and description on a crisp statement of the real problem now that we understand it more. I'd like help on making sure we capture that correctly - how is this for a title: "Uses of Class.forName() miss classes in DynamicClassLoader cache?" ?

Similarly, the description should focus on the problem and show some examples. The defrecord one is good. The first example works for me before the patch and fails after?

2) The crux of this whole thing is the change in loading order in DCL.loadClass() - changing this is a big deal. We really need broader testing with things likely to be affected - off the top of my head: Immutant, Pomegranate, Leiningen, or anything else that monkeys with classloader stuff. Maybe something with Eclipse/OSGi if there is something testable out there.

3) DynamicClassLoader comments:
a) loadClass(String name) - I believe this is identical to the super impl, so can be removed.
b) findClass(String name) - now that we are hijacking loadClass(), I'm not sure it's even necessary to implement this or to call super.findClass() - if you get to the super.findClass(), I would expect that to always throw CNFE. Potentially, this method could even be removed (but this might do bad things if there are subclasses of DCL out there in the wild).
c) loadClass(String name, ...) - instead of calling findClass() and using the CNFE for control flow, you could just directly call findInMemoryClass(), then use a returned null as the decision factor. Again, this is possibly bad if there are DCL subclasses, so I'm on the fence about it.

4) Is the change in gen-interface something that should be a separate ticket? Seems like it could be separable.

5) I don't like the test changes with respect to set up and cleanup. The build already supports compiling a subset of test ns'es (like clojure.test-clojure.genclass.examples). I'd prefer to use that existing mechanism if at all possible. Check the build.xml for the hard-coded ns list.

6) What are the performance implications? I'm not expecting they are significant but we just made a bunch of changes for compilation performance and I'd hate to undo them all. Could findInMemoryClass be smarter about avoiding checks that can't succeed (avoiding "java.*" for example?).

Comment by Nicola Mometto [ 15/Dec/14 5:43 PM ]

1) It's not really about Class.forName() specifically, it's about DynamicClassLoader not being class cache aware in the loadClass method. The JVM uses the classloader loadClass method for resolving all kind of class usages,
including but not limited to Class.forName() (i.e. when loading some bytecode containing a "new" instruction, that class reference will be resolved via a call to loadClass)
I'll try to make the documentation a bit more clear, the first example is an exhibit of the bugged behaviour, the two calls should output the same hash.

2,4) So, there are 3 approaches to how DynamicClassLoader could go at it:

  • Prefer in-disk classes over in-memory classes, roughly the current approach (sometimes it will pick the in-memory class over the in-disk one causing weird bugs like Foo. and ->Foo constructing different classes), has the
    negative effect of breaking interaction between AOT compilation and JIT loading, which has created all sorts of troubles with redefinig deftypes/defprotocols in repls while having stale classfiles in disk.
  • Always pick the most-updated class, this has the advangate of being always correct but has several disadvantages that make it inpracticable in my opinion: we'd have to keep track of the timestamp in which a dynamic class
    is defined, and make the loadClass implementation such that if there a class is both in-memory and in-disk, it compares the timestamps and select the most updated one. This would complicate the implementation a lot and we'd
    likely have to pay a substantial performance hit.
  • Prefer in-memory classes over in-disk classes, the approach proposed in the current patches. It has the advantage of being almost always correct, make repl interaction & jit/aot mixing work fine and the implementation is
    mostly straightforward. The downside is that in cases like gen-class where an AOT class can actually be the most updated version, the in-memory version will be used. In clojure all the forms that do bytecode emission either
    only do AOT compilation or do AOT compilation on demand and always load the class in memory, except gen-interface that doesn't load the class in memory if it's being AOT compiled. Changing its semantics to behave like the
    other jit/aot compiling forms (deftype/defrecord/reify) is the only way to make this approach work so I don't think this should go in another ticket.

5) I don't like the previous testing strategy either but couldn't figure out a better way. Thanks for the pointer on the already in-place infrastructure, I'll check it out and update the patch

In the meantime I've uploaded a new patch addressing 3 and 6. Specifically:
3) I removed the unnecessary loadClass(String) arity, I've made loadClass(String, boolean) use findInMemoryClass(String) directly rather than relying on findClass(String) since nowhere in the documentation it guarantess that
findClass will be used by loadClass. However I've left the findClass(String) implementation in place in case there's code out there that relies on this.
6) I haven't done any serious testing but I haven't noticed any significant difference in compile times for any of my tools.* contrib libraries with the current patch. Filtering "java.*" class names before the inMemory check
didn't seem to produce any difference so it's not included in the updated patch. However I'll probably include an alternative patch with that filtering to do more performance testings and see if it can actually help a bit.

All this said, I'm afraid that I won't have time to personally do an in-depth benchmarking & cross project testing of this patch. I've been spending almost all the free time I had in the past weeks working through a bunch of tickets (mostly this one) but now because of school and other commitments I can't promise I will be able to do anything more than maintaining the current patch & answering to any questions about the bug. Any help in moving this ticket further would be appreciated, in particular to address points 2 and 6.

Comment by Alex Miller [ 16/Dec/14 8:33 AM ]

Thanks Nicola. I'll certainly take over sheparding the bug and appeal to the greater community for help in broad testing when I think we're ready for that.

Comment by Nicola Mometto [ 16/Dec/14 12:50 PM ]

Updated patch with better tests, addressing Alex Miller's comments.

Comment by Michael Blume [ 06/Jan/15 4:20 PM ]

New in-the-wild case: https://groups.google.com/forum/#!topic/clojure/WH6lJwH1vMg

Comment by Colin Fleming [ 17/Feb/15 10:30 PM ]

I believe I have a case which is now broken because of this patch. I'm not 100% clear on the details so I might be wrong, but I can't see any other explanation for what I'm seeing. The bug I'm looking at is Cursive #748. Cursive calls into Leiningen in-process, and before doing that I create a new DynamicClassLoader which uses an IntelliJ PluginClassLoader as its parent. I believe that what is happening is that PluginClassLoader.loadClass() delegates to various parent classloaders then throws CNFE if it can't find the class in question. Am I correct in thinking that after this patch DCL now delegates to the parent classloader before checking its URL list, whereas previously it did not?

Comment by Nicola Mometto [ 18/Feb/15 4:21 AM ]

Colin, I opened CLJ-1663 with a patch that should fix the issue. Can try it and comment on that ticket if it does?

[CLJ-127] DynamicClassLoader's call to ClassLoader.getSystemClassLoader is prohibited in some environments Created: 18/Jun/09  Updated: 18/Apr/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: classloader


Currently, clojure.lang.DynamicClassLoader's constructor has the
following call to super():

      (Thread.currentThread().getContextClassLoader() == null ||
        Thread.currentThread().getContextClassLoader() == ClassLoader.getSystemClassLoader()) ?
          Compiler.class.getClassLoader() : Thread.currentThread().getContextClassLoader());

That call to ClassLoader.getSystemClassLoader() is forbidden by Google
AppEngine's security policies. That restricts you from being able to
load any resources from the classpath that haven't been AOT-compiled.
I've verified that just removing that removing the " ||
Thread.currentThread().getContextClassLoader() ==
ClassLoader.getSystemClassLoader()" does in fact result in something
that works in GAE (as far as my needs go). Unfortunately, I'm not sure
whether that breaks anything, which, presumably, it does.

Comment by Assembla Importer [ 24/Aug/10 3:45 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/127

Comment by Assembla Importer [ 24/Aug/10 3:45 AM ]

jmcconnell said: I'd be happy to take this up with the GAE folks if it winds up looking like this is something they should probably allow or if we need any further information from them on their policies.

Comment by Assembla Importer [ 24/Aug/10 3:45 AM ]

richhickey said: Updating tickets (#127, #128, #129, #130)

Comment by Assembla Importer [ 24/Aug/10 3:45 AM ]

mikehinchey said: GAE made some changes a few weeks ago, maybe changed this because I'm able to load from .clj files now (not the servlet, of course, which must be gen-class).

Comment by Assembla Importer [ 24/Aug/10 3:45 AM ]

richhickey said: Updating tickets (#8, #42, #113, #2, #20, #94, #96, #104, #119, #124, #127, #149, #162)

Comment by Assembla Importer [ 24/Aug/10 3:45 AM ]

mattrevelle said: J. McConnell, was this issue resolved by a change in GAE policy?

Comment by Kevin Downey [ 18/Apr/14 12:05 AM ]

hard to say if this is still an issue, but I have been able to run clojure code on GAE in the past

Generated at Mon Jul 27 20:42:11 CDT 2015 using JIRA 4.4#649-r158309.