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

[automation] Script engines now unloaded if replaced, and closed if AutoCloseable #2681

Merged
merged 1 commit into from
Jan 15, 2022

Conversation

jpg0
Copy link
Contributor

@jpg0 jpg0 commented Jan 11, 2022

This change:

  • Prevents scripts (and their associated engines) from replacing themselves without proper unloading
  • Closes any engines with implement AutoCloseable

(Required to fix openhab/openhab-addons#11952)

Signed-off-by: Jonathan Gilbert jpg@trillica.com

Signed-off-by: Jonathan Gilbert <jpg@trillica.com>
@jpg0 jpg0 changed the title Script engines now unloaded if replaced, and closed if AutoCloseable [automation] Script engines now unloaded if replaced, and closed if AutoCloseable Jan 11, 2022
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

It's nice to be able to close script engine resources without having to restart the runtime. 👍

@wborn wborn merged commit 22c2859 into openhab:main Jan 15, 2022
@wborn wborn added this to the 3.3 milestone Jan 15, 2022
@wborn wborn added the bug An unexpected problem or unintended behavior of the Core label Jan 15, 2022
@digitaldan
Copy link
Contributor

@jpg0 i mentioned this in openhab/openhab-addons#11830 , but after digging into the error, i think its caused by this change, as mentioned in the other PR, I'm getting this error anytime a rule file is updated

19:37:25.062 [INFO ] [.rulesupport.loader.ScriptFileWatcher] - Loading script '/Users/daniel/openhab-main/openhab-3.3.0-SNAPSHOT/conf/automation/js/test.js'
19:37:25.062 [WARN ] [ommon.WrappedScheduledExecutorService] - Scheduled runnable ended with an exception:
java.lang.IllegalStateException: The Context is already closed.
	at com.oracle.truffle.polyglot.PolyglotEngineException.illegalState(PolyglotEngineException.java:129) ~[?:?]
	at com.oracle.truffle.polyglot.PolyglotContextImpl.checkClosed(PolyglotContextImpl.java:1025) ~[?:?]
	at com.oracle.truffle.polyglot.PolyglotContextImpl.enterThreadChanged(PolyglotContextImpl.java:608) ~[?:?]
	at com.oracle.truffle.polyglot.PolyglotEngineImpl.enterCached(PolyglotEngineImpl.java:1885) ~[?:?]
	at com.oracle.truffle.polyglot.PolyglotEngineImpl.enterIfNeeded(PolyglotEngineImpl.java:1813) ~[?:?]
	at com.oracle.truffle.polyglot.PolyglotValueDispatch.hostEnter(PolyglotValueDispatch.java:1228) ~[?:?]
	at com.oracle.truffle.polyglot.PolyglotContextImpl.getBindings(PolyglotContextImpl.java:966) ~[?:?]
	at com.oracle.truffle.polyglot.PolyglotContextDispatch.getBindings(PolyglotContextDispatch.java:97) ~[?:?]
	at org.graalvm.polyglot.Context.getBindings(Context.java:540) ~[?:?]
	at com.oracle.truffle.js.scriptengine.GraalJSScriptEngine.invokeFunction(GraalJSScriptEngine.java:548) ~[?:?]
	at org.openhab.automation.jsscripting.internal.scriptengine.DelegatingScriptEngineWithInvocableAndAutocloseable.invokeFunction(DelegatingScriptEngineWithInvocableAndAutocloseable.java:121) ~[?:?]
	at org.openhab.automation.jsscripting.internal.scriptengine.InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable.invokeFunction(InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable.java:119) ~[?:?]
	at org.openhab.automation.jsscripting.internal.scriptengine.DelegatingScriptEngineWithInvocableAndAutocloseable.invokeFunction(DelegatingScriptEngineWithInvocableAndAutocloseable.java:121) ~[?:?]
	at org.openhab.automation.jsscripting.internal.scriptengine.InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable.invokeFunction(InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable.java:119) ~[?:?]
	at org.openhab.core.automation.module.script.internal.ScriptEngineManagerImpl.removeEngine(ScriptEngineManagerImpl.java:207) ~[?:?]
	at org.openhab.core.automation.module.script.internal.ScriptEngineManagerImpl.createScriptEngine(ScriptEngineManagerImpl.java:127) ~[?:?]
	at org.openhab.core.automation.module.script.rulesupport.loader.ScriptFileWatcher.createAndLoad(ScriptFileWatcher.java:229) ~[?:?]
	at org.openhab.automation.jsscripting.internal.fs.watch.JSScriptFileWatcher.createAndLoad(JSScriptFileWatcher.java:55) ~[?:?]
	at org.openhab.core.automation.module.script.rulesupport.loader.ScriptFileWatcher.importFile(ScriptFileWatcher.java:213) ~[?:?]
	at org.openhab.core.automation.module.script.rulesupport.loader.ScriptFileWatcher.lambda$2(ScriptFileWatcher.java:205) ~[?:?]
	at java.util.Optional.ifPresent(Optional.java:183) ~[?:?]
	at org.openhab.core.automation.module.script.rulesupport.loader.ScriptFileWatcher.importFileWhenReady(ScriptFileWatcher.java:203) ~[?:?]
	at org.openhab.core.automation.module.script.rulesupport.loader.ScriptFileWatcher.processWatchEvent(ScriptFileWatcher.java:180) ~[?:?]
	at org.openhab.automation.jsscripting.internal.fs.watch.JSScriptFileWatcher.processWatchEvent(JSScriptFileWatcher.java:49) ~[?:?]
	at org.openhab.core.service.WatchQueueReader.lambda$3(WatchQueueReader.java:319) ~[?:?]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515) ~[?:?]
	at java.util.concurrent.FutureTask.run(FutureTask.java:264) ~[?:?]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) [?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) [?:?]
	at java.lang.Thread.run(Thread.java:829) [?:?]

I see this PR is closing the script engine (delegate), but without digging in more, it seems like this is not cleaning it up correctly and the closed script engine is continued to be used?

@jpg0
Copy link
Contributor Author

jpg0 commented Jan 24, 2022

@digitaldan I've submitted a (trivial, tbh) PR to remove them here: #2706

My limited testing seemed to confirm that this fixed the issue.

@jpg0 jpg0 deleted the remove-close-script-engines branch January 24, 2022 14:45
wborn pushed a commit to wborn/openhab-core that referenced this pull request Apr 6, 2022
@wborn wborn added the patch A PR that has been cherry-picked to a patch release branch label Apr 6, 2022
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
…penhab#2681)

Signed-off-by: Jonathan Gilbert <jpg@trillica.com>
GitOrigin-RevId: 22c2859
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of the Core patch A PR that has been cherry-picked to a patch release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[JS scripting engine] Memory Leak
4 participants