Copilot commented on code in PR #7582:
URL: https://github.com/apache/shenyu/pull/7582#discussion_r4255420707
##########
shenyu-plugin/shenyu-plugin-wasm-api/src/main/java/org/apache/shenyu/plugin/wasm/api/loader/WasmLoader.java:
##########
@@ -282,17 +345,59 @@ 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 PooledInstance lease = acquire();
+ try {
+ return executeBound(lease.instance,
lease.instance.export(funcName), args);
+ } finally {
+ lease.lock.unlock();
+ }
+ }
+
+ // binds the thread to the instance so guest host callbacks resolve
getMemory() to it;
+ // restores the previously bound instance on exit (nested calls)
+ private long[] executeBound(final Instance instance, final ExportFunction
export, final long[] args) {
+ final Instance previous = CURRENT_INSTANCE.get();
+ CURRENT_INSTANCE.set(instance);
+ try {
+ return export.apply(args);
+ } finally {
+ if (Objects.nonNull(previous)) {
+ CURRENT_INSTANCE.set(previous);
+ } else {
+ CURRENT_INSTANCE.remove();
+ }
+ }
+ }
+
+ private PooledInstance acquire() {
+ final int start = Math.floorMod(nextIndex.getAndIncrement(),
pool.length);
+ for (int offset = 0; offset < pool.length; offset++) {
+ final PooledInstance candidate = pool[Math.floorMod(start +
offset, pool.length)];
+ if (candidate.lock.tryLock()) {
Review Comment:
`ReentrantLock.tryLock()` succeeds when the current owner calls it again. A
custom host callback that invokes another export can therefore re-lease its
already-active instance (always with pool size 1, and when other instances are
busy), re-entering the same Chicory machine while its outer stacks are live and
defeating the promised exclusive lease. Nested calls must use a different idle
instance or fail explicitly rather than re-enter the owned lock.
##########
shenyu-plugin/shenyu-plugin-wasm-api/src/main/java/org/apache/shenyu/plugin/wasm/api/loader/WasmLoader.java:
##########
@@ -38,35 +40,53 @@
import java.util.Objects;
import java.util.Optional;
import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReentrantLock;
import java.util.function.Consumer;
+import java.util.function.Function;
/**
* The WasmLoader aims to load wasm file and provide the wasm function to java,
* also provide the java function to wasm if we need.
*
* <p>Powered by <a href="https://chicory.dev/";>Chicory</a> — a pure-Java
WebAssembly
* runtime with zero native dependencies.
+ *
+ * <p>Concurrency model: a Chicory {@link Instance} is not thread-safe
(interpreter
+ * stacks, guest heap, linear memory), so every exported call leases one
pooled instance
+ * exclusively; host callbacks run inside the lease and {@link #getMemory()}
resolves to
+ * the instance executing on the calling thread.
*/
public class WasmLoader implements AutoCloseable {
private static final Logger LOG =
LoggerFactory.getLogger(WasmLoader.class);
private static final String IMPORT_WASM_MODULE_NAME = "shenyu";
+ // One instance per processor: matches event-loop parallelism; guest
memory is paid per instance.
+ private static final int DEFAULT_POOL_SIZE =
Runtime.getRuntime().availableProcessors();
+
+ // The instance executing on this thread; getMemory() in host callbacks
resolves against it.
+ private static final ThreadLocal<Instance> CURRENT_INSTANCE = new
ThreadLocal<>();
Review Comment:
This binding is shared by every `WasmLoader`. If loader A is executing and
its host callback accesses loader B, `loaderB.getMemory()` sees A's
thread-local instance and returns A's memory. Make the thread-local
loader-local so each loader can only resolve instances from its own pool.
##########
shenyu-plugin/shenyu-plugin-wasm-api/src/main/java/org/apache/shenyu/plugin/wasm/api/loader/WasmLoader.java:
##########
@@ -117,38 +150,57 @@ public WasmLoader(final Class<?> wasmClass, final
Consumer<Store> initializer) {
}
WasmModule module = Parser.parse(wasmBytes);
- // Try runtime compiler (WASM → JVM bytecode) for better execution
speed.
- // Falls back gracefully to interpreter if something goes wrong.
+ final ImportValues importValues = store.toImportValues();
+ // Try the runtime compiler (factory shared by all pooled
instances);
+ // fall back to the interpreter if compilation or first
instantiation fails.
+ Function<Instance, Machine> machineFactory;
+ Instance firstInstance;
try {
- this.instance = Instance.builder(module)
- .withImportValues(store.toImportValues())
-
.withMachineFactory(MachineFactoryCompiler.compile(module))
- .withStart(false)
- .build();
+ machineFactory = MachineFactoryCompiler.compile(module);
+ firstInstance = createInstance(module, importValues,
machineFactory);
Review Comment:
`createInstance` now executes the guest `_initialize`, so any runtime
failure from that guest export or one of its host callbacks is caught here as a
compiler failure and retried on a fresh interpreter instance. This can run
initialization side effects twice and can hide a transient initialization
failure; keep compiler/build fallback separate and invoke `_initialize` only
after the execution mode has been selected.
##########
shenyu-plugin/shenyu-plugin-wasm-api/src/test/java/org/apache/shenyu/plugin/wasm/api/WasmLoaderConcurrentTest.java:
##########
@@ -0,0 +1,171 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shenyu.plugin.wasm.api;
+
+import com.dylibso.chicory.runtime.ExportFunction;
+import org.apache.shenyu.plugin.wasm.api.loader.WasmLoader;
+import org.junit.jupiter.api.Test;
+
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Regression tests for issue #7558: pooled instances must isolate concurrent
calls
+ * while still allowing executions to overlap.
+ */
+public class WasmLoaderConcurrentTest {
+
+ private static final String EXPECTED_RESULT = "go result";
+
+ private static final Map<Long, String> RESULTS = new ConcurrentHashMap<>();
+
+ private static final AtomicLong IDS = new AtomicLong();
+
+ @Test
+ public void concurrentExportsAreIsolatedTest() throws Exception {
+ try (WasmLoader loader = new TestLoader(4, 0L, null, null)) {
Review Comment:
Both regression tests use the normal loader path, so on supported CI hosts
they exercise only the compiled machine. The linked regression scenario
explicitly requires the interpreter/fallback path too, which has separate
mutable interpreter state. Add a deterministic way to select each machine mode
and run the concurrency checks against both.
##########
shenyu-plugin/shenyu-plugin-wasm-api/src/test/java/org/apache/shenyu/plugin/wasm/api/WasmLoaderConcurrentTest.java:
##########
@@ -0,0 +1,171 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shenyu.plugin.wasm.api;
+
+import com.dylibso.chicory.runtime.ExportFunction;
+import org.apache.shenyu.plugin.wasm.api.loader.WasmLoader;
+import org.junit.jupiter.api.Test;
+
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Regression tests for issue #7558: pooled instances must isolate concurrent
calls
+ * while still allowing executions to overlap.
+ */
+public class WasmLoaderConcurrentTest {
+
+ private static final String EXPECTED_RESULT = "go result";
+
+ private static final Map<Long, String> RESULTS = new ConcurrentHashMap<>();
+
+ private static final AtomicLong IDS = new AtomicLong();
+
+ @Test
+ public void concurrentExportsAreIsolatedTest() throws Exception {
+ try (WasmLoader loader = new TestLoader(4, 0L, null, null)) {
+ final ExportFunction execute = loader.getWasmExtern("execute")
+ .orElseThrow(() -> new IllegalStateException("execute
export not found"));
+ final int threads = 8;
+ final int iterations = 100;
+ final ExecutorService pool = Executors.newFixedThreadPool(threads);
+ try {
+ final List<Future<List<String>>> futures = new ArrayList<>();
+ for (int i = 0; i < threads; i++) {
+ futures.add(pool.submit(() -> runIterations(execute,
iterations)));
+ }
+ final List<String> failures = new ArrayList<>();
+ for (Future<List<String>> future : futures) {
+ failures.addAll(future.get());
+ }
+ assertTrue(failures.isEmpty(), () ->
String.join(System.lineSeparator(), failures));
+ assertTrue(RESULTS.isEmpty(), "stray results indicate
cross-call payload leakage");
+ } finally {
+ pool.shutdownNow();
+ }
+ }
+ }
+
+ @Test
+ public void pooledInstancesExecuteInParallelTest() throws Exception {
+ final long blockedArgId = -1L;
+ final CountDownLatch blockEntered = new CountDownLatch(1);
+ final CountDownLatch releaseBlock = new CountDownLatch(1);
+ try (WasmLoader loader = new TestLoader(2, blockedArgId, blockEntered,
releaseBlock)) {
+ final ExportFunction execute = loader.getWasmExtern("execute")
+ .orElseThrow(() -> new IllegalStateException("execute
export not found"));
+ final ExecutorService pool = Executors.newFixedThreadPool(2);
+ try {
+ final Future<?> blockedCall = pool.submit(() ->
execute.apply(blockedArgId));
+ assertTrue(blockEntered.await(10, TimeUnit.SECONDS), "first
call never reached its host callback");
+
+ // must complete on a free instance, not queue behind the
parked one
+ final Future<String> secondCall = pool.submit(() -> {
+ final long argumentId = IDS.incrementAndGet();
+ execute.apply(argumentId);
+ return RESULTS.remove(argumentId);
+ });
+ assertEquals(EXPECTED_RESULT, secondCall.get(10,
TimeUnit.SECONDS),
+ "second call should complete on a free pooled
instance, not queue behind the parked one");
+
+ releaseBlock.countDown();
+ blockedCall.get(10, TimeUnit.SECONDS);
+ assertEquals(EXPECTED_RESULT, RESULTS.remove(blockedArgId));
+ assertTrue(RESULTS.isEmpty(), "stray results indicate
cross-call payload leakage");
+ } finally {
+ releaseBlock.countDown();
+ pool.shutdownNow();
+ }
+ }
+ }
+
+ private static List<String> runIterations(final ExportFunction execute,
final int iterations) {
+ final List<String> failures = new ArrayList<>();
+ for (int i = 0; i < iterations; i++) {
+ final long argumentId = IDS.incrementAndGet();
+ try {
+ execute.apply(argumentId);
+ final String result = RESULTS.remove(argumentId);
+ if (!EXPECTED_RESULT.equals(result)) {
+ failures.add(String.format("%s argId=%d expected '%s' but
got '%s'",
+ Thread.currentThread().getName(), argumentId,
EXPECTED_RESULT, result));
Review Comment:
This does not verify payload isolation: every call expects the same
constant, and the Go fixture ignores the distinct `hello from java <id>` input
and always returns `go result`. Swapping one request's input with another is
therefore invisible. Make the guest echo or derive its result from the
request-specific payload and assert that exact value.
--
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]
