vyommani commented on code in PR #1267:
URL: https://github.com/apache/ranger/pull/1267#discussion_r4141222232
##########
agents-common/src/main/java/org/apache/ranger/plugin/util/RangerRequestExprResolver.java:
##########
@@ -212,24 +212,29 @@ public String resolveExpressions(RangerAccessRequest
request) {
String ret = str;
if (hasTokens) {
- ScriptEngine scriptEngine =
ScriptEngineUtil.createScriptEngine(serviceType);
- RangerRequestScriptEvaluator scriptEvaluator = new
RangerRequestScriptEvaluator(request, scriptEngine,
RangerRequestScriptEvaluator.needsJsonCtxEnabled(str));
- StringBuffer sb = new StringBuffer();
- Matcher matcher =
PATTERN.matcher(str);
+ ScriptEngine scriptEngine =
ScriptEngineUtil.createScriptEngine(serviceType);
- while (matcher.find()) {
- String expr = matcher.group(REGEX_GROUP_EXPR);
- Object oVal = scriptEvaluator.evaluateScript(expr);
- String val = oVal == null ? "" : Objects.toString(oVal);
+ try {
+ RangerRequestScriptEvaluator scriptEvaluator = new
RangerRequestScriptEvaluator(request, scriptEngine,
RangerRequestScriptEvaluator.needsJsonCtxEnabled(str));
+ StringBuffer sb = new
StringBuffer();
+ Matcher matcher =
PATTERN.matcher(str);
- matcher.appendReplacement(sb, val);
- }
+ while (matcher.find()) {
+ String expr = matcher.group(REGEX_GROUP_EXPR);
+ Object oVal = scriptEvaluator.evaluateScript(expr);
+ String val = oVal == null ? "" : Objects.toString(oVal);
- matcher.appendTail(sb);
+ matcher.appendReplacement(sb, val);
+ }
- ret = sb.toString();
+ matcher.appendTail(sb);
- LOG.debug("RangerRequestExprResolver.processExpressions({}):
ret={}", str, ret);
+ ret = sb.toString();
+
+ LOG.debug("RangerRequestExprResolver.processExpressions({}):
ret={}", str, ret);
+ } finally {
+ ScriptEngineUtil.closeScriptEngine(scriptEngine);
Review Comment:
The finally around evaluation is the right place to release the engine, but
ScriptEngine.close() does not free the Graal resources this path allocates.
GraalScriptEngineCreator builds each engine with
GraalJSScriptEngine.create(null, sharedBuilder). A null engine makes Graal
allocate a new org.graalvm.polyglot.Engine. RangerRequestScriptEvaluator then
calls createBindings() and eval(script, bindings), and those bindings own a
separate polyglot Context. On js-scriptengine 25.1.3,
GraalJSScriptEngine.close() only closes the engine's default ScriptContext. It
does not close the bindings context that eval used, and it does not close
getPolyglotEngine(). Each ${{...}} resolution still leaks that context and its
private engine. Calling close() also builds a second context on the shared
builder just to close the unused default one.
Closing getPolyglotEngine() for every ScriptEngine is not safe either. An
engine from ScriptEngineManager shares one factory engine, and closing that
would break later engines. Engines from GraalJSScriptEngine.create(null,
builder) own a private engine, and Engine.close() also closes contexts created
with it. GraalJSBindings is AutoCloseable, and closing those bindings closes
the context eval actually used.
The new tests mock closeScriptEngine and a plain ScriptEngine, so they stay
green when the real Graal context and engine stay open. A test against a real
GraalJSScriptEngine should assert that the polyglot engine and the bindings
context are closed, including when evaluation throws.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
