davsclaus commented on code in PR #26308:
URL: https://github.com/apache/camel/pull/26308#discussion_r3988687839
##########
components/camel-quickjs/src/main/java/org/apache/camel/language/quickjs/QuickjsLanguage.java:
##########
@@ -130,17 +140,62 @@ private Object eval(String script, Map<String, Object>
bindings) {
EngineState state = currentEngine();
state.stderr.reset();
try {
- return state.engine.invokeGuestFunction(
+ Object result = state.engine.invokeGuestFunction(
QuickjsHelper.MODULE_NAME,
QuickjsHelper.FUNCTION_NAME,
List.of(bindings, script),
QuickjsHelper.EVAL_WRAPPER);
+ if (state.exhausted(engineMaxMemory, engineMaxEvaluations)) {
+ discard(state);
+ }
+ return result;
} finally {
// Drop this evaluation's WASI stderr so a reused Engine cannot
accumulate it.
state.stderr.reset();
}
Review Comment:
**The engine is never recycled when a script throws.**
`invokeGuestFunction` throwing skips `exhausted(...)` entirely, so neither
the evaluation counter nor the memory sample is ever taken — while QuickJS has
already evaluated (and kept) the module. Verified on this branch with
`engineMaxEvaluations = 10` and 500 throwing scripts: memory went 1,310,720 ->
4,128,768 bytes and `trackedEngineCount()` stayed at 1.
Moving the check into the `finally` covers both paths:
```suggestion
return result;
} finally {
// Drop this evaluation's WASI stderr so a reused Engine cannot
accumulate it.
state.stderr.reset();
if (state.exhausted(engineMaxMemory, engineMaxEvaluations)) {
discard(state);
}
}
```
A test evaluating a throwing script until the limit and asserting
`trackedEngineCount()` is zero would guard this.
##########
components/camel-quickjs/src/main/java/org/apache/camel/language/quickjs/QuickjsLanguage.java:
##########
@@ -47,6 +48,15 @@
@Language("quickjs")
public class QuickjsLanguage extends TypedLanguageSupport implements
ScriptingLanguage, Service {
+ /**
+ * Every evaluation executes a module in the QuickJS runtime, and QuickJS
keeps evaluated modules until its context
+ * is freed, so an engine grows with every evaluation (about 12 KB each).
An engine is therefore recycled once its
+ * WebAssembly memory exceeds {@link #getEngineMaxMemory()} or it has run
{@link #getEngineMaxEvaluations()}
+ * evaluations: it is closed and the thread creates a fresh one on its
next evaluation.
+ */
+ private long engineMaxMemory = 64L * 1024 * 1024;
+ private int engineMaxEvaluations = 50_000;
Review Comment:
Minor: these are written by the thread that configures the language and read
by every worker thread, with no happens-before between them. In practice they
are set before routes start, but `volatile` costs nothing on a field read once
per evaluation and makes a later runtime change (JMX, a management console)
actually take effect:
```suggestion
private volatile long engineMaxMemory = 64L * 1024 * 1024;
private volatile int engineMaxEvaluations = 50_000;
```
##########
components/camel-quickjs/src/main/java/org/apache/camel/language/quickjs/QuickjsLanguage.java:
##########
@@ -130,17 +140,62 @@ private Object eval(String script, Map<String, Object>
bindings) {
EngineState state = currentEngine();
state.stderr.reset();
try {
- return state.engine.invokeGuestFunction(
+ Object result = state.engine.invokeGuestFunction(
QuickjsHelper.MODULE_NAME,
QuickjsHelper.FUNCTION_NAME,
List.of(bindings, script),
QuickjsHelper.EVAL_WRAPPER);
+ if (state.exhausted(engineMaxMemory, engineMaxEvaluations)) {
+ discard(state);
+ }
+ return result;
} finally {
// Drop this evaluation's WASI stderr so a reused Engine cannot
accumulate it.
state.stderr.reset();
}
}
+ /**
+ * Closes the calling thread's engine; the next evaluation on this thread
creates a fresh one.
+ */
+ private void discard(EngineState state) {
+ if (engine.get() == state) {
+ engine.remove();
+ }
+ engines.remove(state.engine);
Review Comment:
Minor: `discard(...)` runs without `engineLock`, so it can race a concurrent
`stop()` that has already polled this engine off the queue — the engine then
gets closed twice. `Engine.close()` is effectively idempotent (the streams are
`ByteArrayOutputStream`s), so this is benign today, but gating the close on the
removal makes ownership explicit and survives a future `close()` that is not
idempotent:
```suggestion
if (engines.remove(state.engine)) {
closeUnpublished(state.engine);
}
```
--
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]
