Copilot commented on code in PR #6356:
URL: https://github.com/apache/shenyu/pull/6356#discussion_r3355759765
##########
shenyu-plugin/shenyu-plugin-wasm-api/src/main/java/org/apache/shenyu/plugin/wasm/api/loader/WasmLoader.java:
##########
@@ -93,91 +92,273 @@ public WasmLoader(final Class<?> wasmClass, final
Function<Store<Void>, Map<Stri
if (Objects.isNull(resource)) {
throw new ShenyuWasmInitException("Can't find wasm file: " +
wasmName);
}
- // Reads the WebAssembly module as bytes.
- byte[] wasmBytes = Files.readAllBytes(Paths.get(resource.toURI()));
// Instantiates the WebAssembly module.
+
+ this.store = new Store();
+
+ // Register WASI preview1 stubs. Both Go and Rust modules need
these,
+ // but we implement our own no-op versions instead of using
Chicory's
+ // WasiPreview1 to avoid WasiExitException from proc_exit(0).
+ registerWasiStubs(store);
+
+ // Allow subclasses to register custom host functions
(get_args/put_result).
if (Objects.nonNull(initializer)) {
- Map<String, Func> wasmFunctionMap = initializer.apply(store);
- if (Objects.nonNull(wasmFunctionMap) &&
!wasmFunctionMap.isEmpty()) {
- wasmCallJavaFuncMap.putAll(wasmFunctionMap);
- }
+ initializer.accept(store);
+ } else {
+ registerBuiltinHostFunctions(store);
}
- Map<String, Func> wasmFunctionMap = initWasmCallJavaFunc(store);
- if (Objects.nonNull(wasmFunctionMap) &&
!wasmFunctionMap.isEmpty()) {
- wasmCallJavaFuncMap.putAll(wasmFunctionMap);
+
+ // Allow WasmLoader subclasses to override and register additional
host functions.
+ // Only called when not using Consumer initializer, to avoid
double invocation.
+ if (Objects.isNull(initializer)) {
+ initWasmCallJavaFunc(store);
}
- this.module = Module.fromBinary(store.engine(), wasmBytes);
- WasiCtx.addToLinker(linker);
- // maybe need define many functions
- if (!wasmCallJavaFuncMap.isEmpty()) {
- wasmCallJavaFuncMap.forEach((funcName, wasmCallJavaFunc) ->
- linker.define(store, IMPORT_WASM_MODULE_NAME,
funcName, Extern.fromFunc(wasmCallJavaFunc)));
+
+ // Reads the WebAssembly module as bytes.
+ byte[] wasmBytes = Files.readAllBytes(Paths.get(resource.toURI()));
+ WasmModule module = Parser.parse(wasmBytes);
+
+ // Try runtime compiler (WASM → JVM bytecode) for better execution
speed.
+ // Falls back gracefully to interpreter if something goes wrong.
+ try {
+ this.instance = Instance.builder(module)
+ .withImportValues(store.toImportValues())
+
.withMachineFactory(MachineFactoryCompiler.compile(module))
+ .withStart(false)
+ .build();
+ LOG.debug("Using runtime compiler for {}", wasmName);
+ } catch (LinkageError | RuntimeException compilerError) {
+ LOG.warn("Runtime compiler unavailable, falling back to
interpreter for {}: {}",
+ wasmName, compilerError.getMessage());
+ this.instance = Instance.builder(module)
+ .withImportValues(store.toImportValues())
+ .withStart(false)
+ .build();
}
- linker.module(store, "", module);
- // Let the `wasmCallJavaFunc` function to refer this as a
placeholder of Memory because
- // we have to add the function as import before loading the module
exporting Memory.
- Optional<Extern> extern = this.getWasmExtern(MEMORY_METHOD_NAME);
- if (!extern.isPresent()) {
- throw new ShenyuWasmInitException(MEMORY_METHOD_NAME + "
function not find in wasm file: " + wasmName);
+
+ // Call _initialize if present (required by TinyGo -target
wasm-unknown).
+ try {
+ ExportFunction initFn = instance.export("_initialize");
+ if (Objects.nonNull(initFn)) {
+ initFn.apply();
+ LOG.debug("Called _initialize for {}", wasmName);
+ }
+ } catch (com.dylibso.chicory.wasm.InvalidException e) {
+ LOG.debug("No _initialize export in {}", wasmName);
+ }
+
+ if (Objects.isNull(instance.memory())) {
+ throw new ShenyuWasmInitException("memory not available in
wasm file: " + wasmName);
}
- this.memRef = extern.get().memory();
+
Runtime.getRuntime().addShutdownHook(new Thread(this::close));
} catch (URISyntaxException | IOException e) {
throw new ShenyuWasmInitException(e);
}
}
-
- protected Map<String, Func> initWasmCallJavaFunc(final Store<Void> store) {
- return null;
+
+ private void registerWasiStubs(final Store store) {
+ // proc_exit(code: i32) -> ()
+ store.addFunction(new HostFunction(
+ "wasi_snapshot_preview1", "proc_exit",
+ FunctionType.of(List.of(ValType.I32), List.of()),
+ (inst, args) -> new long[0]
Review Comment:
WasmLoader currently registers only a small subset of WASI imports
(proc_exit/random_get/fd_write/environ_*). This is a behavioral regression from
the previous Wasmtime-based implementation that provided full WASI; any plugin
that imports other WASI functions (e.g., clock_time_get, fd_read, path_open,
etc.) will now fail to instantiate.
##########
shenyu-plugin/shenyu-plugin-wasm-base/src/test/java/org/apache/shenyu/plugin/wasm/base/handler/AbstractWasmPluginDataHandlerTest.java:
##########
@@ -39,128 +43,95 @@
@ExtendWith(MockitoExtension.class)
@MockitoSettings(strictness = Strictness.LENIENT)
public final class AbstractWasmPluginDataHandlerTest {
-
- private RuleData ruleData;
-
+
private PluginData pluginData;
-
- private SelectorData selectorData;
-
- private SimpleTestHandler testWasmPluginDataHandler;
-
- private PluginDataHandler pluginDataHandler;
-
+
@BeforeEach
public void setUp() {
- this.ruleData = mock(RuleData.class);
this.pluginData = mock(PluginData.class);
- this.selectorData = mock(SelectorData.class);
- // Use a simple test handler instead of WebAssembly-dependent handler
- this.testWasmPluginDataHandler = new SimpleTestHandler();
- this.pluginDataHandler = () -> "SHENYU";
- when(ruleData.getId()).thenReturn("SHENYU");
when(pluginData.getId()).thenReturn("SHENYU");
- when(selectorData.getId()).thenReturn("SHENYU");
- }
-
- /**
- * The handler plugin test.
- */
- @Test
- public void handlerPluginTest() {
- pluginDataHandler.handlerPlugin(pluginData);
- testWasmPluginDataHandler.handlerPlugin(pluginData);
- }
-
- /**
- * The remove plugin test.
- */
- @Test
- public void removePluginTest() {
- pluginDataHandler.removePlugin(pluginData);
- testWasmPluginDataHandler.handlerPlugin(pluginData);
- testWasmPluginDataHandler.removePlugin(pluginData);
- }
-
- /**
- * The handler selector test.
- */
- @Test
- public void handlerSelectorTest() {
- pluginDataHandler.handlerSelector(selectorData);
- testWasmPluginDataHandler.handlerSelector(selectorData);
}
-
- /**
- * The remove selector test.
- */
- @Test
- public void removeSelectorTest() {
- pluginDataHandler.removeSelector(selectorData);
- testWasmPluginDataHandler.handlerSelector(selectorData);
- testWasmPluginDataHandler.removeSelector(selectorData);
- }
-
- /**
- * The handler rule test.
- */
- @Test
- public void handlerRuleTest() {
- pluginDataHandler.handlerRule(ruleData);
- testWasmPluginDataHandler.handlerRule(ruleData);
- }
-
- /**
- * The remove rule test.
- */
+
+ /** Go WASM test. */
@Test
- public void removeRuleTest() {
- pluginDataHandler.removeRule(ruleData);
- testWasmPluginDataHandler.handlerRule(ruleData);
- testWasmPluginDataHandler.removeRule(ruleData);
+ public void goHandlerPluginTest() {
+ final TestGoWasmPluginDataHandler goHandler = new
TestGoWasmPluginDataHandler("go result");
+ goHandler.handlerPlugin(pluginData);
}
Review Comment:
goHandlerPluginTest only exercises handlerPlugin(), but
AbstractWasmPluginDataHandler also routes
removePlugin/handlerSelector/removeSelector/handlerRule/removeRule through WASM
exports. Without calling them here, a mismatched export name/signature in the
Go .wasm could slip through CI.
##########
shenyu-plugin/shenyu-plugin-wasm-base/src/test/pkg/wasmabi/abi.go:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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 wasmabi provides helper functions for communicating with the Java
+// ShenYu host via shared memory. TinyGo WASM plugins should use GetArgs and
+// PutResult instead of calling the raw host imports directly.
+package wasmabi
Review Comment:
The package comment says this is an ABI helper that TinyGo WASM plugins
“should use”, but the implementation lives under src/test (not
shipped/published). This is either a mismatch with the PR description’s “new
shenyu/wasmabi Go package” claim, or the file should be moved to a non-test
location so external plugin authors can actually depend on it.
##########
shenyu-plugin/shenyu-plugin-wasm-base/src/test/java/org/apache/shenyu/plugin/wasm/base/AbstractShenyuWasmPluginTest.java:
##########
@@ -62,236 +70,158 @@ public final class AbstractShenyuWasmPluginTest {
private ConditionData conditionData;
private ServerWebExchange exchange;
-
- private ShenyuPlugin testShenyuWasmPlugin;
-
+
private ShenyuPluginChain shenyuPluginChain;
@BeforeEach
public void setUp() {
mockShenyuConfig();
this.ruleData = RuleData.builder()
- .id("1")
- .pluginName("SHENYU")
- .selectorId("1")
- .enabled(true)
- .loged(true)
- .matchRestful(false)
- .sort(1).build();
+ .id("1").pluginName("SHENYU").selectorId("1")
+ .enabled(true).loged(true).matchRestful(false).sort(1).build();
this.conditionData = new ConditionData();
this.conditionData.setOperator("match");
this.conditionData.setParamName("/");
this.conditionData.setParamType("uri");
this.conditionData.setParamValue("/http/**");
this.shenyuPluginChain = mock(ShenyuPluginChain.class);
- this.pluginData = PluginData.builder()
- .name("SHENYU")
- .enabled(true).build();
+ this.pluginData =
PluginData.builder().name("SHENYU").enabled(true).build();
this.selectorData = SelectorData.builder()
- .id("1").pluginName("SHENYU")
- .enabled(true)
- .matchRestful(false)
- .type(SelectorTypeEnum.CUSTOM_FLOW.getCode()).build();
- // Use a simple test plugin instead of WebAssembly-dependent plugin
- this.testShenyuWasmPlugin = spy(new SimpleTestPlugin());
- this.exchange =
MockServerWebExchange.from(MockServerHttpRequest.get("/http/SHENYU/SHENYU")
- .build());
+ .id("1").pluginName("SHENYU").enabled(true)
+
.matchRestful(false).type(SelectorTypeEnum.CUSTOM_FLOW.getCode()).build();
+ this.exchange =
MockServerWebExchange.from(MockServerHttpRequest.get("/http/SHENYU/SHENYU").build());
ShenyuContext context = mock(ShenyuContext.class);
exchange.getAttributes().put(Constants.CONTEXT, context);
clearCache();
when(shenyuPluginChain.execute(exchange)).thenReturn(Mono.empty());
}
-
- /**
- * The plugin is null test.
- */
- @Test
- public void executePluginIsNullTest() {
- StepVerifier.create(testShenyuWasmPlugin.execute(exchange,
shenyuPluginChain)).expectSubscription().verifyComplete();
- verify(shenyuPluginChain).execute(exchange);
- }
-
- /**
- * The selector is null test.
- */
- @Test
- public void executeSelectorIsNullTest() {
- BaseDataCache.getInstance().cachePluginData(pluginData);
- StepVerifier.create(testShenyuWasmPlugin.execute(exchange,
shenyuPluginChain)).expectSubscription().verifyComplete();
- verify(shenyuPluginChain).execute(exchange);
- }
-
- /**
- * The selector data is null test.
- */
+
+ /** Go WASM test with selector/rule matching. */
@Test
- public void executeSelectorDataIsNullTest() {
- BaseDataCache.getInstance().cachePluginData(pluginData);
- BaseDataCache.getInstance().cacheSelectData(selectorData);
- StepVerifier.create(testShenyuWasmPlugin.execute(exchange,
shenyuPluginChain)).expectSubscription().verifyComplete();
+ public void executeGoWasmPluginTest() {
+ setupCacheData();
+ final TestGoShenyuWasmPlugin goPlugin = new TestGoShenyuWasmPlugin("go
result");
+ StepVerifier.create(goPlugin.execute(exchange,
shenyuPluginChain)).expectSubscription().verifyComplete();
verify(shenyuPluginChain).execute(exchange);
}
-
- /**
- * The rule is null test.
- */
+
+ /** Rust WASM test with selector/rule matching. */
@Test
- public void executeRuleIsNullTest() {
- List<ConditionData> conditionDataList =
Collections.singletonList(conditionData);
- this.selectorData.setMatchMode(0);
- this.selectorData.setLogged(true);
- this.selectorData.setMatchRestful(false);
- this.selectorData.setConditionList(conditionDataList);
- BaseDataCache.getInstance().cachePluginData(pluginData);
- BaseDataCache.getInstance().cacheSelectData(selectorData);
- StepVerifier.create(testShenyuWasmPlugin.execute(exchange,
shenyuPluginChain)).expectSubscription().verifyComplete();
+ public void executeRustWasmPluginTest() {
+ setupCacheData();
+ final TestShenyuWasmPlugin rustPlugin = new TestShenyuWasmPlugin("rust
result");
+ StepVerifier.create(rustPlugin.execute(exchange,
shenyuPluginChain)).expectSubscription().verifyComplete();
verify(shenyuPluginChain).execute(exchange);
}
Review Comment:
This test class used to cover several control-flow branches (e.g.,
plugin/selector/rule cache missing, and multi-match selector/rule scenarios).
After the refactor it only validates the happy path with one selector + one
rule, which reduces regression coverage for AbstractShenyuWasmPlugin's
matching/dispatch behavior.
--
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]
