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
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
*/
package org.openhab.automation.jsscripting.internal;

import java.util.Arrays;
import java.util.stream.Collectors;

import javax.script.Invocable;
import javax.script.ScriptContext;
import javax.script.ScriptEngine;
Expand All @@ -26,11 +29,13 @@
* Wraps ScriptEngines provided by Graal to provide error messages and stack traces for scripts.
*
* @author Jonathan Gilbert - Initial contribution
* @author Florian Hotze - Improve logger name, Fix memory leak caused by exception logging
*/
class DebuggingGraalScriptEngine<T extends ScriptEngine & Invocable & AutoCloseable>
extends InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable<T> {

private static final String SCRIPT_TRANSFORMATION_ENGINE_IDENTIFIER = "openhab-transformation-script-";
private static final int STACK_TRACE_LENGTH = 5;

private @Nullable Logger logger;

Expand All @@ -49,15 +54,26 @@ protected void beforeInvocation() {
@Override
public Exception afterThrowsInvocation(Exception e) {
Throwable cause = e.getCause();
// OPS4J Pax Logging holds a reference to the exception, which causes the OpenhabGraalJSScriptEngine to not be
// removed from heap by garbage collection and causing a memory leak.
// Therefore, don't pass the exceptions itself to the logger, but only their message!
if (cause instanceof IllegalArgumentException) {
logger.error("Failed to execute script:", e);
}
if (cause instanceof PolyglotException) {
logger.error("Failed to execute script:", cause);
logger.error("Failed to execute script: {}", stringifyThrowable(cause));
} else if (cause instanceof PolyglotException) {
logger.error("Failed to execute script: {}", stringifyThrowable(cause));
}
return e;
}

private String stringifyThrowable(Throwable throwable) {
String message = throwable.getMessage();
StackTraceElement[] stackTraceElements = throwable.getStackTrace();
String stackTrace = Arrays.stream(stackTraceElements).limit(STACK_TRACE_LENGTH)
.map(t -> " at " + t.toString()).collect(Collectors.joining(System.lineSeparator()))
+ System.lineSeparator() + " ... " + stackTraceElements.length + " more";
return (message != null) ? message + System.lineSeparator() + stackTrace : stackTrace;
}

/**
* Initializes the logger.
* This cannot be done on script engine creation because the context variables are not yet initialized.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ public class OpenhabGraalJSScriptEngine

// these fields start as null because they are populated on first use
private @Nullable Consumer<String> scriptDependencyListener;
private String engineIdentifier;
jlaur marked this conversation as resolved.
Show resolved Hide resolved

private boolean initialized = false;
private final boolean injectionEnabled;
Expand Down Expand Up @@ -242,6 +243,7 @@ protected void beforeInvocation() {
if (localEngineIdentifier == null) {
throw new IllegalStateException("Failed to retrieve engine identifier from engine bindings");
}
this.engineIdentifier = localEngineIdentifier;

ScriptExtensionAccessor scriptExtensionAccessor = (ScriptExtensionAccessor) ctx
.getAttribute(CONTEXT_KEY_EXTENSION_ACCESSOR);
Expand All @@ -250,7 +252,7 @@ protected void beforeInvocation() {
}

Consumer<String> localScriptDependencyListener = (Consumer<String>) ctx
.getAttribute("oh.dependency-listener"/* CONTEXT_KEY_DEPENDENCY_LISTENER */);
.getAttribute(CONTEXT_KEY_DEPENDENCY_LISTENER);
if (localScriptDependencyListener == null) {
LOGGER.warn(
"Failed to retrieve script script dependency listener from engine bindings. Script dependency tracking will be disabled.");
Expand Down