Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Isolated classloader context for poly test is unusual/incorrect #245

Closed
seancorfield opened this issue Aug 11, 2022 · 1 comment
Closed
Projects

Comments

@seancorfield
Copy link
Sponsor Contributor

See https://github.com/seancorfield/poly-classloader-bug for repro case and instructions.

This is blocking our migration at work. Although we can add an explicit require to get around this initial failure, other tests subsequently fail for classloader-related issues so it seems like solving this initial problem might address some larger issues.

Although the generated project for the repro uses an older version of Polylith, this behavior continues to exist in the latest release.

@furkan3ayraktar
Copy link
Collaborator

I quickly checked couple of things and leaving them here for reference.

This is how Polylith creates a new ClassLoader:

(defn create-class-loader [paths color-mode]
(try
(let [url-array (into-array URL (map path->url paths))
^URLClassLoader class-loader (url-classloader url-array ext-classloader)]
(.loadClass class-loader "clojure.lang.RT")
(eval-in* class-loader '(require 'clojure.main))
class-loader)
(catch Exception e
(println (str (color/error color-mode "Couldn't create classloader for paths: ") paths " "
(color/error color-mode e))))))

As you can see, after loading clojure.lang.RT only (require 'clojure.main) is run.

I followed what Clojure actually does when it starts and found this:
https://github.com/clojure/clojure/blob/5ffe3833508495ca7c635d47ad7a1c8b820eab76/src/jvm/clojure/main.java#L37-L41

...

final static private Symbol CLOJURE_MAIN = Symbol.intern("clojure.main");
final static private Var REQUIRE = RT.var("clojure.core", "require");
final static private Var MAIN = RT.var("clojure.main", "main");

...

public static void main(String[] args) {
    RT.init();
    REQUIRE.invoke(CLOJURE_MAIN);
    MAIN.applyTo(RT.seq(args));
}

The difference is the RT.init() call before requiring clojure.main. Below is the entire init() and doInit() function it calls in the clojure.lang.RT class.

https://github.com/clojure/clojure/blob/5ffe3833508495ca7c635d47ad7a1c8b820eab76/src/jvm/clojure/lang/RT.java#L466-L501

...

static public void init() {
	doInit();
}

private static boolean INIT = false; // init guard
private synchronized static void doInit() {
	if(INIT) {return;} else {INIT=true;}


	Var.pushThreadBindings(
			RT.mapUniqueKeys(CURRENT_NS, CURRENT_NS.deref(),
			       WARN_ON_REFLECTION, WARN_ON_REFLECTION.deref()
					,RT.UNCHECKED_MATH, RT.UNCHECKED_MATH.deref()));
	try {
		Symbol USER = Symbol.intern("user");
		Symbol CLOJURE = Symbol.intern("clojure.core");


		Var in_ns = var("clojure.core", "in-ns");
		Var refer = var("clojure.core", "refer");
		in_ns.invoke(USER);
		refer.invoke(CLOJURE);
		maybeLoadResourceScript("user.clj");


		// start socket servers
		Var require = var("clojure.core", "require");
		Symbol SERVER = Symbol.intern("clojure.core.server");
		require.invoke(SERVER);
		Var start_servers = var("clojure.core.server", "start-servers");
		start_servers.invoke(System.getProperties());
	}
	catch(Exception e) {
		throw Util.sneakyThrow(e);
	}
	finally {
		Var.popThreadBindings();
	}
}

...

Among other things, doInit function requires clojure.core.server namespace, which is the missing namespace in this scenario.

Maybe invoking RT.init() before requiring clojure.main in the Polylith's ClassLoader creation could solve the issue. We could add (invoke-in class-loader clojure.lang.RT/init []) after loading clojure.lang.RT as following:

 (defn create-class-loader [paths color-mode] 
   (try 
     (let [url-array (into-array URL (map path->url paths)) 
           ^URLClassLoader class-loader (url-classloader url-array ext-classloader)] 
       (.loadClass class-loader "clojure.lang.RT")
       (invoke-in class-loader clojure.lang.RT/init []) ; <- new addition
       (eval-in* class-loader '(require 'clojure.main)) 
       class-loader) 
     (catch Exception e 
       (println (str (color/error color-mode "Couldn't create classloader for paths: ") paths " " 
                     (color/error color-mode e)))))) 

I haven't had time to test this yet. I'll do that when I have some time.

seancorfield added a commit to seancorfield/polylith that referenced this issue Aug 11, 2022
@tengstrand tengstrand added this to Done in Polylith Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

2 participants