Copilot commented on code in PR #7582: URL: https://github.com/apache/shenyu/pull/7582#discussion_r4265727308
########## shenyu-plugin/shenyu-plugin-wasm-api/src/main/java/org/apache/shenyu/plugin/wasm/api/loader/WasmLoader.java: ########## @@ -34,39 +36,59 @@ import java.io.IOException; import java.net.URL; +import java.util.ArrayDeque; import java.util.List; 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(); Review Comment: Defaulting the pool size to `availableProcessors()` can create a large number of WASM instances on high-core machines (or misreported CPU counts), significantly increasing memory footprint and start-up time compared to the previous single-instance behavior. Consider adding an upper cap and/or making the default configurable (e.g., system property/env var), while still allowing explicit `poolSize` via the constructor. ########## shenyu-plugin/shenyu-plugin-wasm-api/src/main/java/org/apache/shenyu/plugin/wasm/api/loader/WasmLoader.java: ########## @@ -282,17 +364,68 @@ 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; + // 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 PooledInstance acquire() { + final ArrayDeque<Instance> active = bindings.get(); + final int start = Math.floorMod(nextIndex.getAndIncrement(), pool.length); + PooledInstance fallback = null; + for (int offset = 0; offset < pool.length; offset++) { + final PooledInstance candidate = pool[Math.floorMod(start + offset, pool.length)]; + if (active.contains(candidate.instance)) { + // never re-enter an instance this thread is already executing on, at any nesting depth + continue; + } + if (Objects.isNull(fallback)) { + fallback = candidate; + } + if (candidate.lock.tryLock()) { + return candidate; + } + } + if (Objects.isNull(fallback)) { + // nesting deeper than the pool: no instance left to lease + throw new IllegalStateException( + "Nested export calls exceed the WASM instance pool size for " + wasmName); + } + // every other instance is busy: queue on the round-robin candidate + fallback.lock.lock(); + return fallback; + } Review Comment: When all instances are busy, `acquire()` blocks on a single chosen `fallback` lock. This can cause head-of-line blocking (waiting on a long-running instance) even if a different instance becomes available sooner, reducing throughput under contention. A more scalable approach is to manage leases with a proper pool structure (e.g., a `BlockingQueue` of free instances) so callers wait for *any* instance to become available rather than a specific lock. ########## shenyu-plugin/shenyu-plugin-wasm-api/src/test/java/org/apache/shenyu/plugin/wasm/api/WasmLoaderConcurrentTest.java: ########## @@ -0,0 +1,284 @@ +/* + * 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 com.dylibso.chicory.runtime.Instance; +import com.dylibso.chicory.runtime.Machine; +import com.dylibso.chicory.wasm.WasmModule; +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 java.util.function.Function; +import java.util.function.LongFunction; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +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(); Review Comment: The test class uses shared static state (`RESULTS`, `IDS`) across multiple `@Test` methods. If the build enables JUnit parallel test execution (or if a failure leaves residue), this can make results order-dependent and flaky. Prefer clearing `RESULTS` (and optionally resetting `IDS`) in a `@BeforeEach`/`@AfterEach`, or make them instance fields scoped per test to ensure isolation. -- 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]
