Copilot commented on code in PR #7582:
URL: https://github.com/apache/shenyu/pull/7582#discussion_r4273213328
##########
shenyu-plugin/shenyu-plugin-wasm-api/src/main/java/org/apache/shenyu/plugin/wasm/api/loader/WasmLoader.java:
##########
@@ -282,17 +378,74 @@ private Optional<ExportFunction> findWasmExtern(final
String funcName) {
return Optional.of(instance.export(funcName));
}
}
- LOG.debug("WASM export function '{}' not found in {}", funcName,
wasmName);
return Optional.empty();
}
-
+
+ private long[] executeOnPool(final String funcName, final long[] args) {
+ final Instance lease = acquire();
+ try {
+ return executeBound(lease, lease.export(funcName), args);
+ } finally {
+ release(lease);
+ }
+ }
+
+ // binds the thread to the instance so guest host callbacks resolve
getMemory() to it;
+ // pops on exit (nested calls each push their own entry)
+ private long[] executeBound(final Instance instance, final ExportFunction
export, final long[] args) {
+ final ArrayDeque<Instance> stack = bindings.get();
+ stack.push(instance);
+ try {
+ return export.apply(args);
+ } finally {
+ stack.pop();
+ }
+ }
+
+ private Instance acquire() {
+ if (!ready) {
+ // reached only from a host callback during construction (e.g. a
nested
+ // export call from _initialize): no initialized instance is free
yet,
+ // so waiting could never succeed
+ throw new IllegalStateException(
+ "Nested export call during construction of " + wasmName
+ + "; the instance pool is not ready yet");
+ }
+ final ArrayDeque<Instance> active = bindings.get();
+ if (active.size() >= poolSize) {
+ // nesting deeper than the pool: this thread already leases every
instance,
+ // so waiting could never succeed
+ throw new IllegalStateException(
+ "Nested export calls exceed the WASM instance pool size
for " + wasmName);
+ }
+ try {
+ return freeInstances.take();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new IllegalStateException(
+ "Interrupted while waiting for a pooled WASM instance of "
+ wasmName, e);
+ }
Review Comment:
`freeInstances.take()` can block indefinitely. In production this can
translate into hung requests or deadlocks (e.g., multiple threads hold
instances and each performs a nested export that waits for another lease).
Consider using a bounded wait (e.g., `poll(timeout, unit)`) and failing fast
with a diagnostic exception message (pool size, currently-bound depth, and
optionally the export name) to avoid unbounded stalls.
##########
shenyu-plugin/shenyu-plugin-wasm-api/src/main/java/org/apache/shenyu/plugin/wasm/api/loader/WasmLoader.java:
##########
@@ -282,17 +378,74 @@ private Optional<ExportFunction> findWasmExtern(final
String funcName) {
return Optional.of(instance.export(funcName));
}
}
- LOG.debug("WASM export function '{}' not found in {}", funcName,
wasmName);
return Optional.empty();
}
-
+
+ private long[] executeOnPool(final String funcName, final long[] args) {
+ final Instance lease = acquire();
+ try {
+ return executeBound(lease, lease.export(funcName), args);
+ } finally {
+ release(lease);
Review Comment:
If `executeBound(...)` throws, and `release(lease)` also throws, the release
failure will mask the original exception from the WASM export. To preserve
debuggability, capture the original throwable, attempt to release in a separate
try/catch, and attach any release failure via `addSuppressed(...)` (or log it)
before rethrowing the original.
##########
shenyu-plugin/shenyu-plugin-wasm-api/src/main/java/org/apache/shenyu/plugin/wasm/api/loader/WasmLoader.java:
##########
@@ -282,17 +378,74 @@ private Optional<ExportFunction> findWasmExtern(final
String funcName) {
return Optional.of(instance.export(funcName));
}
}
- LOG.debug("WASM export function '{}' not found in {}", funcName,
wasmName);
return Optional.empty();
}
-
+
+ private long[] executeOnPool(final String funcName, final long[] args) {
+ final Instance lease = acquire();
+ try {
+ return executeBound(lease, lease.export(funcName), args);
+ } finally {
+ release(lease);
+ }
+ }
+
+ // binds the thread to the instance so guest host callbacks resolve
getMemory() to it;
+ // pops on exit (nested calls each push their own entry)
+ private long[] executeBound(final Instance instance, final ExportFunction
export, final long[] args) {
+ final ArrayDeque<Instance> stack = bindings.get();
+ stack.push(instance);
+ try {
+ return export.apply(args);
+ } finally {
+ stack.pop();
+ }
+ }
+
+ private Instance acquire() {
+ if (!ready) {
+ // reached only from a host callback during construction (e.g. a
nested
+ // export call from _initialize): no initialized instance is free
yet,
+ // so waiting could never succeed
+ throw new IllegalStateException(
+ "Nested export call during construction of " + wasmName
+ + "; the instance pool is not ready yet");
+ }
Review Comment:
This introduces a new hard failure mode for nested export calls during
construction (e.g., if a host callback triggered by `_initialize` calls another
export). Previously (single instance) that pattern could succeed. If this is
intended, it should be clearly documented as a constraint of the API; otherwise
consider executing nested calls against the currently-bound instance during
initialization (when concurrency isn’t a concern yet) to preserve prior
behavior while still avoiding deadlock.
##########
shenyu-plugin/shenyu-plugin-wasm-api/src/main/java/org/apache/shenyu/plugin/wasm/api/loader/WasmLoader.java:
##########
@@ -282,17 +378,74 @@ private Optional<ExportFunction> findWasmExtern(final
String funcName) {
return Optional.of(instance.export(funcName));
}
}
- LOG.debug("WASM export function '{}' not found in {}", funcName,
wasmName);
return Optional.empty();
}
-
+
+ private long[] executeOnPool(final String funcName, final long[] args) {
+ final Instance lease = acquire();
+ try {
+ return executeBound(lease, lease.export(funcName), args);
+ } finally {
+ release(lease);
+ }
+ }
+
+ // binds the thread to the instance so guest host callbacks resolve
getMemory() to it;
+ // pops on exit (nested calls each push their own entry)
+ private long[] executeBound(final Instance instance, final ExportFunction
export, final long[] args) {
+ final ArrayDeque<Instance> stack = bindings.get();
+ stack.push(instance);
+ try {
+ return export.apply(args);
+ } finally {
+ stack.pop();
+ }
+ }
+
+ private Instance acquire() {
+ if (!ready) {
+ // reached only from a host callback during construction (e.g. a
nested
+ // export call from _initialize): no initialized instance is free
yet,
+ // so waiting could never succeed
+ throw new IllegalStateException(
+ "Nested export call during construction of " + wasmName
+ + "; the instance pool is not ready yet");
+ }
+ final ArrayDeque<Instance> active = bindings.get();
+ if (active.size() >= poolSize) {
+ // nesting deeper than the pool: this thread already leases every
instance,
+ // so waiting could never succeed
+ throw new IllegalStateException(
+ "Nested export calls exceed the WASM instance pool size
for " + wasmName);
+ }
+ try {
+ return freeInstances.take();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new IllegalStateException(
+ "Interrupted while waiting for a pooled WASM instance of "
+ wasmName, e);
+ }
+ }
+
+ private void release(final Instance lease) {
+ if (!freeInstances.offer(lease)) {
+ throw new IllegalStateException(
+ "Failed to return pooled WASM instance for " + wasmName
+ + "; queue is full, which indicates a
double-release or pool corruption");
Review Comment:
If `executeBound(...)` throws, and `release(lease)` also throws, the release
failure will mask the original exception from the WASM export. To preserve
debuggability, capture the original throwable, attempt to release in a separate
try/catch, and attach any release failure via `addSuppressed(...)` (or log it)
before rethrowing the original.
##########
shenyu-plugin/shenyu-plugin-wasm-api/src/main/java/org/apache/shenyu/plugin/wasm/api/loader/WasmLoader.java:
##########
@@ -282,17 +378,74 @@ private Optional<ExportFunction> findWasmExtern(final
String funcName) {
return Optional.of(instance.export(funcName));
}
}
- LOG.debug("WASM export function '{}' not found in {}", funcName,
wasmName);
return Optional.empty();
}
-
+
+ private long[] executeOnPool(final String funcName, final long[] args) {
+ final Instance lease = acquire();
+ try {
+ return executeBound(lease, lease.export(funcName), args);
+ } finally {
+ release(lease);
+ }
+ }
+
+ // binds the thread to the instance so guest host callbacks resolve
getMemory() to it;
+ // pops on exit (nested calls each push their own entry)
+ private long[] executeBound(final Instance instance, final ExportFunction
export, final long[] args) {
+ final ArrayDeque<Instance> stack = bindings.get();
+ stack.push(instance);
+ try {
+ return export.apply(args);
+ } finally {
+ stack.pop();
+ }
+ }
+
+ private Instance acquire() {
+ if (!ready) {
+ // reached only from a host callback during construction (e.g. a
nested
+ // export call from _initialize): no initialized instance is free
yet,
+ // so waiting could never succeed
+ throw new IllegalStateException(
+ "Nested export call during construction of " + wasmName
+ + "; the instance pool is not ready yet");
+ }
+ final ArrayDeque<Instance> active = bindings.get();
+ if (active.size() >= poolSize) {
+ // nesting deeper than the pool: this thread already leases every
instance,
+ // so waiting could never succeed
+ throw new IllegalStateException(
+ "Nested export calls exceed the WASM instance pool size
for " + wasmName);
+ }
+ try {
+ return freeInstances.take();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new IllegalStateException(
+ "Interrupted while waiting for a pooled WASM instance of "
+ wasmName, e);
+ }
+ }
+
+ private void release(final Instance lease) {
+ if (!freeInstances.offer(lease)) {
+ throw new IllegalStateException(
+ "Failed to return pooled WASM instance for " + wasmName
+ + "; queue is full, which indicates a
double-release or pool corruption");
+ }
+ }
+
/**
- * Get the WASM module's linear memory.
+ * Get the WASM module's linear memory for use in host functions.
+ *
+ * <p>During an exported call (including host callbacks) this returns the
memory of
+ * the instance executing on the current thread; otherwise the primary
instance's.
*
* @return the WASM linear memory
*/
public Memory getMemory() {
- return instance.memory();
+ final Instance current = bindings.get().peek();
+ return Objects.nonNull(current) ? current.memory() : primary.memory();
}
Review Comment:
With pooled instances, returning `primary.memory()` when no instance is
bound can be unsafe under concurrency (callers outside an export/host-callback
may read/write memory that is currently leased/executing on another thread). A
safer contract is to fail fast when unbound (e.g., throw with a clear message),
or to expose two methods: one strictly for \"current call\" memory (bound-only)
and an explicitly documented, not-thread-safe \"primary\" memory accessor if
needed.
--
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]
