Skip to content

Commit

Permalink
refactor(lua-bridge) rewrite for full yielding support
Browse files Browse the repository at this point in the history
Major refactor of the Lua bridge to support multiple concurrent yielding
Lua threads. The old implementation would break down when scheduling
more than one yielding Lua thread at a time.

The new implementation "tricks" OpenResty by scheduling uthreads via C
and passing these threads to the OpenResty runloop as if they were
created from Lua (via `ngx.thread`). Because all uthreads must resume
their "parent thread" when finished (as per OpenResty's implementation),
we schedule a stub "entry thread" whenever we are trying to use the Lua
bridge. This entry thread itself does nothing and is collected at
request pool cleanup.

List of significant changes for this refactor:

- **Breaking:** the `proxy_wasm.start()` FFI function is **removed**.
  Only `proxy_wasm.attach()` is now necessary, and the filter chain is
  only resumed once the ngx_http_wasm_module `rewrite` or `access`
  phases are entered. Prior, `proxy_wasm.start()` would resume the
  filter chain during the ngx_http_lua_module phase handlers, which was
  incompatible with Lua threads yielding.
- In ngx_wasm_socket_tcp, the `sock->env` member is now a pointer to the
  request's `env` instead of a copy so as to manipulate the `env->state`
  control variable.
- The `wasm_call` directive can now yield, which allows for sanity
  testing of the Lua bridge yielding functionality.
- A new `rctx->resume_handler` pointer holds the resume entry point back
  from yielding facilities into `ngx_http_core_run_phases`. For now,
  only the Lua bridge uses it, but other yielding facilities should be
  refactored to use it so as to factorize our resuming code.

Fix #524
  • Loading branch information
thibaultcha committed May 10, 2024
1 parent 5d8fed2 commit 8fd1fcb
Show file tree
Hide file tree
Showing 39 changed files with 1,367 additions and 859 deletions.
3 changes: 2 additions & 1 deletion .github/actions/setup-httpbin-server/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ runs:
--no-resolv \
--port=${{ inputs.dns_port }} \
--server=${{ inputs.upstream_dns_server }} \
--address=/httpbin.org/127.0.0.1
--address=/httpbin.org/127.0.0.1 \
--address=/example.com/127.0.0.1
- name: Start httpbin proxy + server
shell: bash
run: |
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/job-unit-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ jobs:
sudo bash -c 'echo "${{ github.workspace }}/coredumps/%e.%p.%t" > /proc/sys/kernel/core_pattern'
- run: make setup
- run: make
- name: Run make test
- name: Run tests
run: |
ulimit -c unlimited
make test
Expand Down
35 changes: 10 additions & 25 deletions lib/resty/wasmx/proxy_wasm.lua
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ if subsystem == "http" then
int ngx_http_wasm_ffi_plan_attach(ngx_http_request_t *r,
ngx_wasm_plan_t *plan,
unsigned int isolation);
int ngx_http_wasm_ffi_start(ngx_http_request_t *r);
int ngx_http_wasm_ffi_set_property(ngx_http_request_t *r,
ngx_str_t *key,
ngx_str_t *value,
Expand Down Expand Up @@ -160,6 +159,16 @@ function _M.load(c_plan)
return nil, "failed loading plan"
end

if get_request() then
-- ffi_gc: hold a reference tied to the request lifecycle so users
-- don't have to (like our test suite).
if not ngx.ctx[_M] then
ngx.ctx[_M] = {}
end

ngx.ctx[_M][c_plan] = true
end

return true
end

Expand Down Expand Up @@ -215,30 +224,6 @@ function _M.attach(c_plan, opts)
end


function _M.start()
local phase = ngx.get_phase()
if phase ~= "rewrite" and phase ~= "access" then
error("start must be called from 'rewrite' or 'access' phase", 2)
end

local r = get_request()
if not r then
error("no request found", 2)
end

local rc = C.ngx_http_wasm_ffi_start(r)
if rc == FFI_ERROR then
return nil, "unknown error"
end

if rc == FFI_DECLINED then
return nil, "plan not loaded and attached"
end

return true
end


function _M.set_property(key, value)
if type(key) ~= "string" then
error("key must be a string", 2)
Expand Down
Loading

0 comments on commit 8fd1fcb

Please sign in to comment.