Kong / ngx_wasm_module

Nginx + WebAssembly
Apache License 2.0
93 stars 8 forks source link

[CI] merge(*) host properties FFI getter/setter #449

Closed thibaultcha closed 11 months ago

thibaultcha commented 11 months ago

Changes atop #431:

Overall, the changes make the feature increase project coverage instead of decreasing it.

Plus an additional fix in dynamic builds:

Without this fix, the wasm_filter_module handlers execute before body_filter_by_lua, which isn't the desired behavior (i.e. same as static builds). This patch fixes FFI getter/setter body_filter_by_lua sanity tests, which were caught thanks to proper use of --- grep_error_log.

codecov[bot] commented 11 months ago

Codecov Report

Merging #449 (9c661a8) into main (db88b15) will increase coverage by 0.02525%. Report is 1 commits behind head on main. The diff coverage is 93.38843%.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/449/graphs/tree.svg?width=650&height=150&src=pr&token=T0PT2Q9IAN&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong)](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) ```diff @@ Coverage Diff @@ ## main #449 +/- ## =================================================== + Coverage 90.16374% 90.18899% +0.02524% =================================================== Files 46 46 Lines 8489 8572 +83 =================================================== + Hits 7654 7731 +77 - Misses 835 841 +6 ``` | [Files](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | Coverage Δ | | |---|---|---| | [src/common/proxy\_wasm/ngx\_proxy\_wasm.h](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2NvbW1vbi9wcm94eV93YXNtL25neF9wcm94eV93YXNtLmg=) | `92.30769% <ø> (ø)` | | | [src/common/proxy\_wasm/ngx\_proxy\_wasm\_host.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2NvbW1vbi9wcm94eV93YXNtL25neF9wcm94eV93YXNtX2hvc3QuYw==) | `93.75000% <100.00000%> (+0.12931%)` | :arrow_up: | | [src/http/ngx\_http\_wasm\_module.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2h0dHAvbmd4X2h0dHBfd2FzbV9tb2R1bGUuYw==) | `96.36872% <100.00000%> (ø)` | | | [src/wasm/ngx\_wasm\_core\_module.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL3dhc20vbmd4X3dhc21fY29yZV9tb2R1bGUuYw==) | `93.47826% <100.00000%> (+0.09591%)` | :arrow_up: | | [src/common/lua/ngx\_wasm\_lua\_ffi.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2NvbW1vbi9sdWEvbmd4X3dhc21fbHVhX2ZmaS5j) | `89.79592% <86.20690%> (-0.74462%)` | :arrow_down: | | [src/common/proxy\_wasm/ngx\_proxy\_wasm\_properties.c](https://app.codecov.io/gh/Kong/ngx_wasm_module/pull/449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#diff-c3JjL2NvbW1vbi9wcm94eV93YXNtL25neF9wcm94eV93YXNtX3Byb3BlcnRpZXMuYw==) | `93.51351% <94.44444%> (-0.02496%)` | :arrow_down: |
thibaultcha commented 11 months ago

In fact speaking about this API, I would even be in favor of merging the setter/getter functions into a single FFI call: set_host_properties_handlers, as I do not see the benefit of splitting the operation in two different calls, it feels unnecessarily bloated or "copy-pasted" for easy-mirroring.

@hishamhm I think I am going to make this change too; it would avoid an additional Lua/C round-trip on a hot code path, avoid duplicating error handling from the caller, and provide a smaller surface API which imho is always good, easier to document and grasp, etc (avoids the proliferation of functions exposed to the user). If the caller wishes to only set the setter or getter, they can call proxy_wasm.set_host_properties_handlers(getter, nil). Sounds good?

hishamhm commented 11 months ago

If the caller wishes to only set the setter or getter, they can call proxy_wasm.set_host_properties_handlers(getter, nil). Sounds good?

Sounds great!

hishamhm commented 11 months ago

Guard feature to OpenResty builds.

This is the only change I would argue against: the original design of host-defined properties kept the Lua and non-Lua sides well separated, and this was proven when I made three different implementations of the Lua side (FFI, Lua/C, Lua bridge APIs) and ngx_proxy_wasm_properties.c remained unchanged -- there was nothing FFI-specific (or even Lua-specific) about the additions there. My reasoning was that, as an Nginx module that can used with and without OpenResty, it is reasonable to think that a different non-Lua-driven Nginx application could still want to register a C function callback as a driver for dynamic host-defined properties.

I don't feel strongly about this, but I just wanted to point out the rationale of the design (otherwise the work of ngx_proxy_wasm_properties_set_ffi_getter/setter could simply be done directly in ngx_wasm_lua_ffi.c?) and the use-case.

thibaultcha commented 11 months ago

If that is ever a use-case the guards can be changed/removed and the struct members renamed (I doubt it ever will of course). In the meantime, the guards help the reader better mapping what parts of properties.c have to do with what other parts of the codebase, and what can be eliminated (from the mind) when working on vanilla Nginx builds - it's mostly reader-centric (maintainer-centric). properties.c isn't that easy to follow because it itself represents multiple concepts (Nginx-mapped variables, custom but hard-coded C handlers variables, and now custom FFI handlers variables, but only for "host properties", a getter and a setter symmetry for all properties, plus an rbtree to store them, etc...), so being able to eliminate parts of it for specific work is helpful imho.