Conversation
扩展WASM ABI接口以支持数据注入、上游主机管理和指标获取变更文件
时序图Sequence diagram for new Wasm interface logic
participant WasmModule as WebAssembly Module
participant WasmRuntime as ProxyWasm Runtime
participant Context as ContextBase
WasmModule->>WasmRuntime: inject_encoded_data_to_filter_chain(body, endStream)
WasmRuntime->>Context: injectEncodedDataToFilterChain(body, endStream)
WasmRuntime-->>WasmModule: 返回WasmResult
WasmModule->>WasmRuntime: get_upstream_hosts(ptr, size)
WasmRuntime->>Context: getUpstreamHosts(pairs)
WasmRuntime->>WasmRuntime: 序列化pairs数据
WasmRuntime-->>WasmModule: 返回内存指针和大小
WasmModule->>WasmRuntime: set_upstream_override_host(address)
WasmRuntime->>Context: setUpstreamOverrideHost(address)
WasmRuntime-->>WasmModule: 返回执行结果
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
![lingma-agents[bot]](https://avatars.githubusercontent.com/in/1198481?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 1 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 2 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 1 | 次要问题，酌情优化。例如：代码格式不规范或注释缺失。 |
总计： 4 个问题
📋 评审意见详情
💡 单文件建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
🔧 include/proxy-wasm/context.h (1 💬)
- 新增方法未实现可能导致运行时错误 (L358-L370)
🔧 include/proxy-wasm/context_interface.h (1 💬)
- 注释拼写错误影响可读性 (L263)
📄 src/exports.cc (2 💬)
- get_upstream_hosts空列表处理逻辑错误 (L172-L184)
- 内存分配未处理释放逻辑 (L186-L189)
🚀 跨文件建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍 1. 新增接口未实现导致架构不一致
在include/proxy-wasm/context.h中新增的injectEncodedDataToFilterChain、getUpstreamHosts、setUpstreamOverrideHost等方法均直接返回unimplemented()，但这些接口在context_interface.h中被定义为纯虚函数强制实现。这种声明与实现的不一致违反了接口契约，可能导致调用方在不知情的情况下触发运行时错误。建议统一接口状态（完全实现或标记为可选），或移除未准备好的接口定义。
📌 关键代码：
WasmResult injectEncodedDataToFilterChain(...) override { return unimplemented(); }
WasmResult getUpstreamHosts(...) override { return unimplemented(); }
WasmResult setUpstreamOverrideHost(...) override { return unimplemented(); }
🔍 2. 跨文件内存管理逻辑存在不一致性
src/exports.cc中get_upstream_hosts函数在分配内存后未实现释放逻辑，而其他新增函数如inject_encoded_data_to_filter_chain在调用方未显式处理内存释放。这种不一致的内存管理策略可能引发内存泄漏或访问冲突，建议建立统一的内存管理规范并在接口文档中明确责任边界。
📌 关键代码：
char *buffer = context->wasm()->allocMemory(size, &ptr);
// 无对应free操作
🔍 3. 重复的错误处理逻辑未抽象化
在src/exports.cc的三个新增函数中（inject_encoded_data_to_filter_chain、get_upstream_hosts、set_upstream_override_host），均存在对getMemory结果的空值检查和错误返回的重复代码段，违反DRY原则。建议将此类通用逻辑封装为公共辅助函数以提升可维护性。
📌 关键代码：
auto body = context->wasmVm()->getMemory(body_ptr, body_size);
if (!body) { return WasmResult::InvalidMemoryAccess; }
🔍 4. 缺乏新增接口的单元测试覆盖
PR中新增的WASM接口（如get_upstream_hosts、injectEncodedDataToFilterChain等）未见配套的单元测试。新增功能缺乏验证可能导致隐藏缺陷，建议补充测试用例覆盖核心路径（成功/失败场景、边界条件等）。
🔍 5. StringPairs类型定义存在跨文件不一致性风险
在include/proxy-wasm/context_interface.h新增StringPairs类型定义，但未检查其他依赖文件是否已有类似定义（如PairsWithStringValues）。这种类型定义的重复可能导致后续开发中的命名冲突或数据转换问题，建议进行全局类型体系审查。
📌 关键代码：
using StringPairs = std::vector<std::pair<std::string, std::string>>;
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@Lingma-Agent 分析这个方法的性能瓶颈并提供优化建议。
-
@Lingma-Agent 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@Lingma-Agent 请总结上述讨论并提出解决方案。
-
@Lingma-Agent 请根据讨论内容生成优化代码。
| WasmResult injectEncodedDataToFilterChain(std::string_view /* body_text */, bool /* end_stream */) override { | ||
| return unimplemented(); | ||
| } | ||
|
|
||
| // Get all enpoint metrics of upstream cluster | ||
| WasmResult getUpstreamHosts(StringPairs * /* result */) override { | ||
| return unimplemented(); | ||
| } | ||
|
|
||
| // Override upstream host and bypass lb policy | ||
| WasmResult setUpstreamOverrideHost(std::string_view /* address */) override { | ||
| return unimplemented(); | ||
| } |
There was a problem hiding this comment.
新增方法未实现可能导致运行时错误
🟠 Critical | 🐞 Bugs
📋 问题详情
新增的三个方法（injectEncodedDataToFilterChain/getUpstreamHosts/setUpstreamOverrideHost）均直接返回unimplemented()，未提供实际逻辑。这会导致调用这些接口时返回未实现错误，影响功能完整性。
💡 解决方案
应确保所有新增接口提供基础实现逻辑，至少返回WasmResult::Ok或具体业务逻辑。建议：
- return unimplemented();
+ return WasmResult::Ok; // 或具体实现您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| // Inject encoded data to filter chain | ||
| virtual WasmResult injectEncodedDataToFilterChain(std::string_view /* body_text */, bool /* end_stream */) = 0; | ||
|
|
||
| // Get all enpoint metrics of upstream cluster |
There was a problem hiding this comment.
注释拼写错误影响可读性
🟢 Minor | 🧹 Code Smells
📋 问题详情
getUpstreamHosts注释中'enpoint'应为'endpoint'，拼写错误影响文档准确性。
💡 解决方案
修正拼写：
-enpoint
+endpoint🔧 建议代码
‼️ AI 生成代码 - 请在应用前检查逻辑、规范并测试
| // Get all enpoint metrics of upstream cluster | |
| // Get all endpoint metrics of upstream cluster |
您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| Word get_upstream_hosts(Word ptr_ptr, Word size_ptr) { | ||
| auto *context = contextOrEffectiveContext(); | ||
| StringPairs pairs; | ||
| auto result = context->getUpstreamHosts(&pairs); | ||
| if (result != WasmResult::Ok) { | ||
| return result; | ||
| } | ||
| if (pairs.empty()) { | ||
| if (!context->wasm()->copyToPointerSize("", ptr_ptr, size_ptr)) { | ||
| return WasmResult::InvalidMemoryAccess; | ||
| } | ||
| return WasmResult::Ok; | ||
| } |
There was a problem hiding this comment.
get_upstream_hosts空列表处理逻辑错误
🟡 Major | 🐞 Bugs
📋 问题详情
当pairs为空时，调用copyToPointerSize写入空字符串，但size_ptr应设为0而非写入内容，可能导致内存访问错误。
💡 解决方案
修改为空列表处理逻辑：
-copyToPointerSize("", ptr_ptr, size_ptr)
+setWord(size_ptr, 0)🔧 建议代码
‼️ AI 生成代码 - 请在应用前检查逻辑、规范并测试
| Word get_upstream_hosts(Word ptr_ptr, Word size_ptr) { | |
| auto *context = contextOrEffectiveContext(); | |
| StringPairs pairs; | |
| auto result = context->getUpstreamHosts(&pairs); | |
| if (result != WasmResult::Ok) { | |
| return result; | |
| } | |
| if (pairs.empty()) { | |
| if (!context->wasm()->copyToPointerSize("", ptr_ptr, size_ptr)) { | |
| return WasmResult::InvalidMemoryAccess; | |
| } | |
| return WasmResult::Ok; | |
| } | |
| if (pairs.empty()) { | |
| if (!context->wasmVm()->setWord(size_ptr, 0)) { | |
| return WasmResult::InvalidMemoryAccess; | |
| } | |
| return WasmResult::Ok; | |
| } | |
您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| uint64_t ptr = 0; | ||
| char *buffer = static_cast<char *>(context->wasm()->allocMemory(size, &ptr)); | ||
| if (buffer == nullptr) { | ||
| return WasmResult::InvalidMemoryAccess; |
There was a problem hiding this comment.
内存分配未处理释放逻辑
🟡 Major | 🧹 Code Smells
📋 问题详情
allocMemory分配的内存未在异常路径释放，可能导致内存泄漏。需添加错误处理时的释放逻辑。
💡 解决方案
添加释放保护：
+ // 在所有return前确保释放
+ if (!PairsUtil::marshalPairs(pairs, buffer, size)) {
+ context->wasm()->freeMemory(buffer);
+ return WasmResult::InvalidMemoryAccess;
+ }您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
1 participant
No description provided.