Clojurians Log v2

Hi Arne! Can you please explain why it breaks priority-classloader? Is it because the latter is installed on top of some other DCL in the classloader chain?

I know I might have broken something with that commit but I still fail to understand what is the right thing to do over there.

yes, exactly. See lambdaisland.classpath/install-priority-loader!. That sets the context classloader so that the classloader stack looks like this:

[priority-classloader
   DynamicClassLoader
   AppClassLoader
   PlatformClassLoader]

tbh I'm not sure I can really say what's the "right" thing to do, because the classloader stuff in clojure is brittle, confusing, and the design decisions aren't documented anywhere

this is from clojure.lang.RT:

static public ClassLoader makeClassLoader() {
	try {
		Var.pushThreadBindings(RT.map(USE_CONTEXT_CLASSLOADER, RT.T));
		return new DynamicClassLoader(baseLoader());
	} finally {
		Var.popThreadBindings();
	}
}

static public ClassLoader baseLoader(){
	if(Compiler.LOADER.isBound())
		return (ClassLoader) Compiler.LOADER.deref();
	else if(booleanCast(USE_CONTEXT_CLASSLOADER.deref()))
		return Thread.currentThread().getContextClassLoader();
	return Compiler.class.getClassLoader();
}

Compiler/eval starts with

public static Object eval(Object form, boolean freshLoader) {
	boolean createdLoader = false;
	if(true)//!LOADER.isBound())
		{
		Var.pushThreadBindings(RT.map(LOADER, RT.makeClassLoader()));
		createdLoader = true;
		}

so it always make a new DCL for each eval, based on that baseLoader (Compiler/LOADER or context classloader)

clearly at some point this was guarded, but it's been commented out like that since forever. Would be amazing to find out why that was.

should the session middleware always bind Compiler/LOADER given all this? there's probably a reason we do that, maybe for non-eval operations?

If I remember correctly, the decision to traverse the chain to the topmost DCL was made precisely to combat that uncontrolled new DCL proliferation of the stock eval.

But ever since we've owned the eval machinery in nrepl, it is no longer critical.

Having said that, it is important to understand if we want to allow the base scenario of user code (the code being eval'ed) setting a new wrapped classloader. Again, I think require will modify the classloaders for the current thread and not necessarily clean up after itself. I need to check.

Is there any other way you may install priority-classloader?

you tell me... it's heavy handed code cause that's the only way I got it to work reliably.

here's the code, there are a bunch of comments there about various considerations https://github.com/lambdaisland/classpath/blob/main/src/lambdaisland/classpath.clj#L355

I'd love for there to be an endorsed, reliable way for the user to set the classloader in a way that just works inside and outside of nrepl. The only way I've found is to set the context classloader on every thread

it should be ok if a PL gets wrapped by a DCL again, as long as its still in the stack above the application classloader

what were you trying to solve with that nrepl commit?

Yeah, I feel you. Even if we add a dedicated nREPL op to change the classloader, that would help for the non-nrepl case.

> what were you trying to solve with that nrepl commit? Nothing in particular. It just didn't feel right that we use getContextClassloader here when everywhere else we use (dynamic-classloader).

I'm going to merge your PR but I still have zero idea if it's a Good Thing® or not.

By the way, I don't think you need the future/sleep song and dance anymore, I don't see nrepl's ieval reverting the context classloader. I think we removed that.

the contextclassloader stuff itself seems to be an escape hatch added for some obscure reason in the depths of java history, which just happens to serve us well because Clojure mostly honors it

> I'm going to merge your PR but I still have zero idea if it's a Good Thing® or not. Thanks! much appreciated!

But don't take my word for it 🙂 There may be something inside Clojure itself that reverts the ccl.

I'll take my chances with this, I did test it locally with just that change and at least the hot-reload seemed to work again. It's worked for years, hopefully we're good for another few years.