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.

Sign up for GitHub

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

[REPL] Stop old classpath calculation on the base classloader #4363

Closed
wants to merge 1 commit into from
Closed

[REPL] Stop old classpath calculation on the base classloader #4363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libraries/scripting/jvm/src/kotlin/script/experimental/jvm/BasicJvmReplEvaluator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class BasicJvmReplEvaluator(val scriptEvaluator: ScriptEvaluator = BasicJvmScrip
}
if (lastSnippetClass != null) {
jvm {
baseClassLoader(lastSnippetClass.java.classLoader)
lastSnippetClassLoader(lastSnippetClass.java.classLoader)
}
}
}
Expand Down
11 changes: 6 additions & 5 deletions libraries/scripting/jvm/src/kotlin/script/experimental/jvm/impl/KJvmCompiledScript.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,14 @@ fun KJvmCompiledScript.getOrCreateActualClassloader(evaluationConfiguration: Scr
val module = compiledModule
?: throw IllegalStateException("Illegal call sequence, actualClassloader should be set before calling function on the class without module")
val baseClassLoader = evaluationConfiguration[ScriptEvaluationConfiguration.jvm.baseClassLoader]
val lastClassLoader = evaluationConfiguration[ScriptEvaluationConfiguration.jvm.lastSnippetClassLoader] ?: baseClassLoader
val classLoaderWithDeps =
if (evaluationConfiguration[ScriptEvaluationConfiguration.jvm.loadDependencies] == false) baseClassLoader
else makeClassLoaderFromDependencies(baseClassLoader)
else makeClassLoaderFromDependencies(baseClassLoader, lastClassLoader)
return module.createClassLoader(classLoaderWithDeps)
}

private fun CompiledScript.makeClassLoaderFromDependencies(baseClassLoader: ClassLoader?): ClassLoader? {
private fun CompiledScript.makeClassLoaderFromDependencies(baseClassLoader: ClassLoader?, lastClassLoader: ClassLoader?): ClassLoader? {
val processedScripts = mutableSetOf<CompiledScript>()
fun recursiveScriptsSeq(res: Sequence<CompiledScript>, script: CompiledScript): Sequence<CompiledScript> =
if (processedScripts.add(script)) script.otherScripts.asSequence().fold(res + script, ::recursiveScriptsSeq)
Expand All @@ -139,17 +140,17 @@ private fun CompiledScript.makeClassLoaderFromDependencies(baseClassLoader: Clas
val processedClasspathElements = mutableSetOf<URL>()
fun recursiveClassPath(res: Sequence<URL>, classLoader: ClassLoader?): Sequence<URL> =
when (classLoader) {
null -> res
null, baseClassLoader -> res
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still process the null case, e.g. generating an error if we've got null.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it should be after baseClassLoader check, since it could be null, and it is a valid situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ligee I'm not sure that fallbackClassloader of DualClassLoader should necessarily have base classloader as a parent. Is it really so?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I can see from usages, it is always a thread context classloader that is a base classloader at the same time, so there is no appropriate test

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say it should convert to the baseClassLoader, but since it is possible to replace it, it is safer to leave the code as you wrote it.
About the tests - there are some tests with baseClassLoader set to null, and that's probably it.

is DualClassLoader -> recursiveClassPath(res, classLoader.parent) +
recursiveClassPath(emptySequence(), classLoader.fallbackClassLoader)
is URLClassLoader -> recursiveClassPath(res + classLoader.urLs, classLoader.parent)
else -> recursiveClassPath(res, classLoader.parent)
}
recursiveClassPath(emptySequence(), baseClassLoader).forEach { processedClasspathElements.add(it) }
recursiveClassPath(emptySequence(), lastClassLoader).forEach { processedClasspathElements.add(it) }

val processedClassloaders = mutableSetOf<ClassLoader>()

return dependenciesWithConfigurations.fold(baseClassLoader) { parentClassLoader, (compilationConfiguration, scriptDependency) ->
return dependenciesWithConfigurations.fold(lastClassLoader) { parentClassLoader, (compilationConfiguration, scriptDependency) ->
when (scriptDependency) {
is JvmDependency -> {
scriptDependency.classpath.mapNotNull {
Expand Down
5 changes: 5 additions & 0 deletions libraries/scripting/jvm/src/kotlin/script/experimental/jvm/jvmScriptEvaluation.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ val JvmScriptEvaluationConfigurationKeys.baseClassLoader by PropertiesCollection
isTransient = true
)

/**
* Classloader of the last snippet (supposed to be used in REPL)
*/
val JvmScriptEvaluationConfigurationKeys.lastSnippetClassLoader by PropertiesCollection.key<ClassLoader?>(isTransient = true)

/**
* Load script dependencies before evaluation, true by default
* If false, it is assumed that the all dependencies will be provided via baseClassLoader
Expand Down