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

[jsscripting] Fix memory leak on script execution failure #16578

Merged

Conversation

florian-h05
Copy link
Contributor

Add-on part of the fix for #16462.

Add-on part of the fix for openhab#16462.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05 florian-h05 changed the title [jsscripting] Fix memory leak on script invocation failure [jsscripting] Fix memory leak on script execution failure Mar 26, 2024
@florian-h05 florian-h05 requested a review from jlaur March 26, 2024 00:28
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Make engineIdentifier a instance field to ease debugging.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Copy link
Contributor Author

florian-h05 commented Mar 26, 2024

@jlaur I have now pushed my improvement. It basically stringifies the first 5 elements of the stack trace (that should be enough for the JS part of the stack trace in most cases) so we still have a part of the stack trace, but Pax Logging does not hold a ref to the exception. I have also checked this change again with VisualVM, it does not re-introduce the memory leak.

FYI I also made the engine identifier an instance field for OpenhabGraalJSScriptEngine because it really really eases debugging.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Copy link
Contributor Author

florian-h05 commented Mar 26, 2024

The log message now looks like:

11:46:55.591 [ERROR] [.automation.script.javascript.abcd.js] - Failed to execute script: SyntaxError: abcd.js:10:97 Expected } but found eof
{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{};{
                                                                                                 ^
        at org.graalvm.polyglot.Context.eval(Context.java:399)
        at com.oracle.truffle.js.scriptengine.GraalJSScriptEngine.eval(GraalJSScriptEngine.java:458)
        at com.oracle.truffle.js.scriptengine.GraalJSScriptEngine.eval(GraalJSScriptEngine.java:400)
        at java.scripting/javax.script.AbstractScriptEngine.eval(AbstractScriptEngine.java:247)
        at org.openhab.automation.jsscripting.internal.scriptengine.DelegatingScriptEngineWithInvocableAndAutocloseable.eval(DelegatingScriptEngineWithInvocableAndAutocloseable.java:57)
        ... 18 more

And if there is a JS stack trace:

11:49:13.424 [ERROR] [tomation.script.javascript.a299db5278] - Failed to execute script: Error
        at <js>.getItem(/etc/openhab/automation/js/node_modules/openhab/src/items/items.js:589)
        at <js>.:program(<eval>:1)
        at org.graalvm.polyglot.Context.eval(Context.java:399)
        at com.oracle.truffle.js.scriptengine.GraalJSScriptEngine.eval(GraalJSScriptEngine.java:458)
        at com.oracle.truffle.js.scriptengine.GraalJSScriptEngine.eval(GraalJSScriptEngine.java:426)
        ... 76 more

This reverts commit 4bb4977.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05 florian-h05 requested a review from jlaur March 26, 2024 22:34
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@lolodomo lolodomo added the bug An unexpected problem or unintended behavior of an add-on label Mar 27, 2024
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Many thanks for fixing this nasty memory leak!

@jlaur jlaur merged commit 66e8d7d into openhab:main Mar 27, 2024
3 checks passed
@jlaur jlaur added this to the 4.2 milestone Mar 27, 2024
@florian-h05 florian-h05 deleted the jsscripting-scriptexception-memory-leak branch March 27, 2024 21:29
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 an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants