Skip to content

Set the ScriptEngineManager to use Context's loader#439

Merged
ctrueden merged 1 commit intomasterfrom
scriptEngineManager-classLoader
Jul 18, 2022
Merged

Set the ScriptEngineManager to use Context's loader#439
ctrueden merged 1 commit intomasterfrom
scriptEngineManager-classLoader

Conversation

@gselzer
Copy link
Member

@gselzer gselzer commented Jul 18, 2022

This change fixes the issue of ScriptLanguage plugin creation in imagej/napari-imagej#58.

The singular change changes the way in which AdaptedScriptLanguage's ScriptEngineManager is constructed. Previously, the no-args constructor was called. Now, we call the constructor taking a ClassLoader, and pass a Context.getClassLoader().

new ScriptEngineManager() does nothing more than call new ScriptEngineManager(Thread.currentThread().getContextClassLoader()).

Context.getClassLoader() is nothing more than this call with a fallback for a null ClassLoader

Thus, this change does nothing more than ensure that ScriptEngineManager has a ClassLoader, avoiding the internal fallbacks.

The only functionality we may lose is within ScriptEngineManager.getServiceLoader(ClassLoader c). Here is the method, decompiled in IntelliJ:

  private ServiceLoader<ScriptEngineFactory> getServiceLoader(ClassLoader var1) {
    return var1 != null ? ServiceLoader.load(ScriptEngineFactory.class, var1) : ServiceLoader.loadInstalled(ScriptEngineFactory.class);
  }

This PR will ensure that var1 is always non-null. Thus we lose any situations where we called ServiceLoader.loadInstalled. But, seeing as how this method [always ignores Service Providers in the application's classpath], I don't think we ever want that behavior here? When would we ever want to avoid loading a ScriptLanguage that is on the application's classpath?

Do you have any concerns @ctrueden?

@gselzer gselzer requested a review from ctrueden July 18, 2022 14:37
@gselzer gselzer self-assigned this Jul 18, 2022
@gselzer gselzer added the bug label Jul 18, 2022
@ctrueden ctrueden merged commit 401b6c5 into master Jul 18, 2022
@ctrueden ctrueden deleted the scriptEngineManager-classLoader branch July 18, 2022 18:03
@ctrueden
Copy link
Member

@gselzer Thank you! No concerns, I think this fix is exactly correct. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments