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

Wildcard imports of built-in packages don't work #4994

Closed
rbasralian opened this issue Dec 29, 2023 · 2 comments · Fixed by #5117
Closed

Wildcard imports of built-in packages don't work #4994

rbasralian opened this issue Dec 29, 2023 · 2 comments · Fixed by #5117
Assignees
Labels
bug Something isn't working core Core development tasks groovy

Comments

@rbasralian
Copy link
Contributor

A wildcard import like:

import java.util.concurrent.*

throws:

com.illumon.iris.controller.exception.ScriptException: java.lang.RuntimeException: Attempting to import a path that does not exist: import java.util.concurrent.*;
	at io.deephaven.engine.util.GroovyDeephavenSession.updateScriptImports(GroovyDeephavenSession.java:582)
	at io.deephaven.engine.util.GroovyDeephavenSession.grepScriptImports(GroovyDeephavenSession.java:592)
	at io.deephaven.engine.util.GroovyDeephavenSession.evaluate(GroovyDeephavenSession.java:307)
	at io.deephaven.engine.util.AbstractScriptSession.lambda$evaluateScript$0(AbstractScriptSession.java:147)
	at io.deephaven.engine.context.ExecutionContext.lambda$apply$0(ExecutionContext.java:149)
	at io.deephaven.engine.context.ExecutionContext.apply(ExecutionContext.java:160)
	at io.deephaven.engine.context.ExecutionContext.apply(ExecutionContext.java:148)

This is basically because groovyShell.getClassLoader().getDefinedPackage("java.util.concurrent") returns null. (Using .getPackage(), which is deprecated, does work. It eventually calls jdk.internal.loader.BootLoader.getDefinedPackage())

@rbasralian rbasralian added bug Something isn't working triage labels Dec 29, 2023
@rcaudy
Copy link
Member

rcaudy commented Dec 29, 2023

Eww. Why do we still have things that grep script imports?

Regardless, I'm pretty confident that getPackage is internally racy and unreliable.

@rcaudy rcaudy added core Core development tasks groovy and removed triage labels Dec 29, 2023
@rcaudy rcaudy added this to the December 2023 milestone Dec 29, 2023
@niloc132
Copy link
Member

niloc132 commented Feb 6, 2024

ClassLoader.getDefinedPackage is expected to return null in these cases for the first import - we have a unit test that validates that null must be returned before trying to run a wildcard import:

@Test
public void testUnloadedWildcardPackageImport() {
// it's unlikely we have loaded anything from this groovy package
final String packageString = "groovy.time";
if (this.getClass().getClassLoader().getDefinedPackage(packageString) != null) {
Assert.fail("Package '" + packageString + "' is already loaded, test with a more obscure package.");
}
session.evaluateScript("import " + packageString + ".*").throwIfError();
}

However, changing groovy.time to java.util.concurrent (or java.util) in that test produces a failure, as this bug describes (contrast with io.deephaven.engine.util, which will not return null from that test on line 136, and so the test cannot run).

In GroovyDeephavenSession.createClassImport, after getDefinedPackage fails, ClassGraph is used to look up the class. ClassGraph has a specific config that must be applied to be able to scan system modules/packages - enabling that appears to resolve this.

niloc132 added a commit to niloc132/deephaven-core that referenced this issue Feb 6, 2024
Also updates our classgraph build, in case we were affected by Java 17
issues.

Fixes deephaven#4994
niloc132 added a commit to niloc132/deephaven-core that referenced this issue Feb 6, 2024
Also updates our classgraph build, in case we were affected by Java 17
issues.

Fixes deephaven#4994
niloc132 added a commit that referenced this issue Feb 6, 2024
Also updates our classgraph build, in case we were affected by Java 17
issues.

Fixes #4994
devinrsmith pushed a commit that referenced this issue Feb 12, 2024
Also updates our classgraph build, in case we were affected by Java 17
issues.

Fixes #4994
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core Core development tasks groovy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants