Skip to content

Commit 54f7d26

Browse files
thibaultchathibaultcha
committed
refactor(proxy-wasm) improve pwexec resurrection and instance lifecycle
The main goal of this overhaul is to simplify `on_context_create`, make it fully re-entrant *and* properly handle instance recycling at the same time. The way to do so, in my opinion, was to move `pwexec` creation where `rexec` already was. In other words, always lookup the context id in the instance rbtree, and if not found, create it. This means that surrounding code also needed big overhauls. It also removes the reference counting poor man's GC of the older implementation. The code became really ugly by then so I took the time to also review this module's code structure instead of making a *very* ugly commit. This new ngx_proxy_wasm.c file should be much easier to read and follow now. One change I do not fully like is moving the `next_id` to a global counter, but we do not have a "global proxy-wasm conf" object yet. I also started thinking about pre-allocating a number of `pwexecs` (like `worker_connections`) and use free/busy queue that all filter chains can dip into to get a context id + context memory zone. Perhaps for a later time.
1 parent 378c370 commit 54f7d26

17 files changed

+1081
-948
lines changed

‎src/common/proxy_wasm/ngx_proxy_wasm.c

Lines changed: 1010 additions & 815 deletions
Large diffs are not rendered by default.

‎src/common/proxy_wasm/ngx_proxy_wasm.h

Lines changed: 13 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,6 @@
88

99
#define NGX_PROXY_WASM_ROOT_CTX_ID 0
1010

11-
#define ngx_proxy_wasm_store_init(s, p) \
12-
(s)->pool = (p); \
13-
ngx_queue_init(&(s)->sweep); \
14-
ngx_queue_init(&(s)->free); \
15-
ngx_queue_init(&(s)->busy)
16-
1711

1812
typedef enum {
1913
NGX_PROXY_WASM_0_1_0 = 0,
@@ -195,6 +189,7 @@ struct ngx_proxy_wasm_exec_s {
195189
ngx_proxy_wasm_ctx_t *parent;
196190
ngx_proxy_wasm_filter_t *filter;
197191
ngx_proxy_wasm_instance_t *ictx;
192+
ngx_proxy_wasm_store_t *store;
198193
ngx_event_t *ev;
199194
#ifdef NGX_WASM_HTTP
200195
ngx_http_proxy_wasm_dispatch_t *call;
@@ -203,8 +198,8 @@ struct ngx_proxy_wasm_exec_s {
203198
/* flags */
204199

205200
unsigned started:1;
206-
unsigned ecode_logged:1;
207201
unsigned in_tick:1;
202+
unsigned ecode_logged:1;
208203
};
209204

210205

@@ -259,9 +254,7 @@ struct ngx_proxy_wasm_ctx_s {
259254

260255

261256
struct ngx_proxy_wasm_instance_s {
262-
ngx_uint_t next_id;
263-
ngx_uint_t nrefs;
264-
ngx_queue_t q;
257+
ngx_queue_t q; /* store busy/free/sweep */
265258
ngx_rbtree_t tree_ctxs;
266259
ngx_rbtree_t root_ctxs;
267260
ngx_rbtree_node_t sentinel_ctxs;
@@ -274,7 +267,7 @@ struct ngx_proxy_wasm_instance_s {
274267

275268
/* swap */
276269

277-
ngx_proxy_wasm_exec_t *pwexec;
270+
ngx_proxy_wasm_exec_t *pwexec; /* current pwexec */
278271
};
279272

280273

@@ -379,30 +372,27 @@ struct ngx_proxy_wasm_filter_s {
379372
};
380373

381374

382-
/* root */
383-
void ngx_proxy_wasm_init(ngx_conf_t *cf);
384-
void ngx_proxy_wasm_exit();
385-
ngx_uint_t ngx_proxy_wasm_id(ngx_str_t *name, ngx_str_t *config,
386-
uintptr_t data);
375+
/* root context */
376+
void ngx_proxy_wasm_init(ngx_conf_t *cf, ngx_proxy_wasm_store_t *gstore);
377+
void ngx_proxy_wasm_exit(ngx_proxy_wasm_store_t *gstore);
387378
ngx_int_t ngx_proxy_wasm_load(ngx_proxy_wasm_filter_t *filter, ngx_log_t *log);
388379
ngx_int_t ngx_proxy_wasm_start(ngx_cycle_t *cycle);
389380

390381

391-
/* ctx/store */
382+
/* stream context */
392383
ngx_proxy_wasm_ctx_t *ngx_proxy_wasm_ctx_alloc(ngx_pool_t *pool);
393384
ngx_proxy_wasm_ctx_t *ngx_proxy_wasm_ctx(ngx_uint_t *filter_ids,
394385
size_t nfilters, ngx_uint_t isolation, ngx_proxy_wasm_subsystem_t *subsys,
395386
void *data);
396387
void ngx_proxy_wasm_ctx_destroy(ngx_proxy_wasm_ctx_t *pwctx);
397388
ngx_int_t ngx_proxy_wasm_resume(ngx_proxy_wasm_ctx_t *pwctx,
398389
ngx_wasm_phase_t *phase, ngx_proxy_wasm_step_e step);
390+
ngx_proxy_wasm_err_e ngx_proxy_wasm_run_step(ngx_proxy_wasm_exec_t *pwexec,
391+
ngx_proxy_wasm_step_e step);
392+
393+
394+
/* host handlers */
399395
ngx_wavm_ptr_t ngx_proxy_wasm_alloc(ngx_proxy_wasm_exec_t *pwexec, size_t size);
400-
void ngx_proxy_wasm_store_destroy(ngx_proxy_wasm_store_t *store);
401-
ngx_proxy_wasm_instance_t *ngx_proxy_wasm_get_instance(
402-
ngx_proxy_wasm_filter_t *filter, ngx_proxy_wasm_store_t *store,
403-
ngx_proxy_wasm_exec_t *pwexec, ngx_log_t *log);
404-
void ngx_proxy_wasm_release_instance(ngx_proxy_wasm_instance_t *ictx,
405-
unsigned sweep);
406396

407397

408398
/* utils */
@@ -418,8 +408,6 @@ ngx_int_t ngx_proxy_wasm_pairs_unmarshal(ngx_proxy_wasm_exec_t *pwexec,
418408
unsigned ngx_proxy_wasm_marshal(ngx_proxy_wasm_exec_t *pwexec,
419409
ngx_list_t *list, ngx_array_t *extras, ngx_wavm_ptr_t *out,
420410
uint32_t *out_size, ngx_uint_t *truncated);
421-
ngx_proxy_wasm_err_e ngx_proxy_wasm_run_step(ngx_proxy_wasm_exec_t *pwexec,
422-
ngx_proxy_wasm_instance_t *ictx, ngx_proxy_wasm_step_e step);
423411

424412

425413
static ngx_inline void

‎src/common/proxy_wasm/ngx_proxy_wasm_host.c

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -205,44 +205,43 @@ ngx_proxy_wasm_hfuncs_set_tick_period(ngx_wavm_instance_t *instance,
205205
{
206206
uint32_t period = args[0].of.i32;
207207
ngx_event_t *ev;
208-
ngx_proxy_wasm_exec_t *pwexec = ngx_proxy_wasm_instance2pwexec(instance);
208+
ngx_proxy_wasm_exec_t *rexec = ngx_proxy_wasm_instance2pwexec(instance);
209209

210-
ngx_wasm_assert(pwexec->root_id == NGX_PROXY_WASM_ROOT_CTX_ID);
210+
ngx_wasm_assert(rexec->root_id == NGX_PROXY_WASM_ROOT_CTX_ID);
211211

212-
if (pwexec->root_id != NGX_PROXY_WASM_ROOT_CTX_ID) {
212+
if (rexec->root_id != NGX_PROXY_WASM_ROOT_CTX_ID) {
213213
/* ignore */
214214
return ngx_proxy_wasm_result_ok(rets);
215215
}
216216

217217
if (ngx_exiting) {
218-
return ngx_proxy_wasm_result_trap(pwexec, "process exiting", rets,
219-
NGX_WAVM_OK);
218+
return ngx_proxy_wasm_result_trap(rexec, "process exiting",
219+
rets, NGX_WAVM_OK);
220220
}
221221

222-
if (pwexec->tick_period) {
223-
return ngx_proxy_wasm_result_trap(pwexec, "tick_period already set",
222+
if (rexec->tick_period) {
223+
return ngx_proxy_wasm_result_trap(rexec, "tick_period already set",
224224
rets, NGX_WAVM_OK);
225225
}
226226

227-
pwexec->tick_period = period;
227+
rexec->tick_period = period;
228228

229229
ev = ngx_calloc(sizeof(ngx_event_t), instance->log);
230230
if (ev == NULL) {
231231
goto nomem;
232232
}
233233

234234
ev->handler = ngx_proxy_wasm_filter_tick_handler;
235-
ev->data = pwexec;
236-
ev->log = pwexec->log;
235+
ev->data = rexec;
236+
ev->log = rexec->log;
237237

238-
ngx_add_timer(ev, pwexec->tick_period);
238+
ngx_add_timer(ev, rexec->tick_period);
239239

240240
return ngx_proxy_wasm_result_ok(rets);
241241

242242
nomem:
243243

244-
return ngx_proxy_wasm_result_trap(pwexec, "no memory",
245-
rets, NGX_WAVM_ERROR);
244+
return ngx_proxy_wasm_result_trap(rexec, "no memory", rets, NGX_WAVM_ERROR);
246245
}
247246

248247

‎src/common/proxy_wasm/ngx_proxy_wasm_util.c

Lines changed: 15 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -193,65 +193,49 @@ ngx_proxy_wasm_log_error(ngx_uint_t level, ngx_log_t *log,
193193
void
194194
ngx_proxy_wasm_filter_tick_handler(ngx_event_t *ev)
195195
{
196-
ngx_log_t *log = ev->log;
197-
ngx_proxy_wasm_exec_t *pwexec = ev->data;
196+
ngx_proxy_wasm_err_e ecode;
197+
ngx_log_t *log = ev->log;
198+
ngx_proxy_wasm_exec_t *rexec = ev->data;
199+
ngx_proxy_wasm_filter_t *filter = rexec->filter;
198200
#ifdef NGX_WASM_HTTP
199-
ngx_proxy_wasm_ctx_t *pwctx = pwexec->parent;
201+
ngx_proxy_wasm_ctx_t *pwctx = rexec->parent;
200202
#endif
201-
ngx_proxy_wasm_filter_t *filter = pwexec->filter;
202-
ngx_proxy_wasm_instance_t *ictx;
203-
ngx_proxy_wasm_err_e ecode;
204203

205204
dd("enter");
206205

207-
ngx_wasm_assert(pwexec->root_id == NGX_PROXY_WASM_ROOT_CTX_ID);
206+
ngx_wasm_assert(rexec->root_id == NGX_PROXY_WASM_ROOT_CTX_ID);
208207

209208
ngx_free(ev);
210209

211-
pwexec->ev = NULL;
210+
rexec->ev = NULL;
212211

213212
if (ngx_exiting || !filter->proxy_on_timer_ready) {
214213
return;
215214
}
216215

217-
ictx = ngx_proxy_wasm_get_instance(filter, filter->store, pwexec,
218-
filter->log);
219-
if (ictx == NULL) {
220-
ngx_wasm_log_error(NGX_LOG_ERR, log, 0,
221-
"tick_handler: no instance");
222-
return;
223-
}
224-
225216
#ifdef NGX_WASM_HTTP
226217
pwctx->phase = ngx_wasm_phase_lookup(&ngx_http_wasm_subsystem,
227218
NGX_WASM_BACKGROUND_PHASE);
228219
#endif
229-
pwexec->in_tick = 1;
230-
231-
ecode = ngx_proxy_wasm_run_step(pwexec, ictx, NGX_PROXY_WASM_STEP_TICK);
232-
233-
ngx_proxy_wasm_release_instance(ictx, 0);
234220

221+
ecode = ngx_proxy_wasm_run_step(rexec, NGX_PROXY_WASM_STEP_TICK);
235222
if (ecode != NGX_PROXY_WASM_ERR_NONE) {
236-
pwexec->ecode = ecode;
237223
return;
238224
}
239225

240-
pwexec->in_tick = 0;
241-
242226
if (!ngx_exiting) {
243-
pwexec->ev = ngx_calloc(sizeof(ngx_event_t), log);
244-
if (pwexec->ev == NULL) {
227+
rexec->ev = ngx_calloc(sizeof(ngx_event_t), log);
228+
if (rexec->ev == NULL) {
245229
goto nomem;
246230
}
247231

248-
pwexec->ev->handler = ngx_proxy_wasm_filter_tick_handler;
249-
pwexec->ev->data = pwexec;
250-
pwexec->ev->log = log;
232+
rexec->ev->handler = ngx_proxy_wasm_filter_tick_handler;
233+
rexec->ev->data = rexec;
234+
rexec->ev->log = log;
251235

252-
dd("scheduling next tick in %ld", pwexec->tick_period);
236+
dd("scheduling next tick in %ld", rexec->tick_period);
253237

254-
ngx_add_timer(pwexec->ev, pwexec->tick_period);
238+
ngx_add_timer(rexec->ev, rexec->tick_period);
255239
}
256240

257241
return;

‎src/http/ngx_http_wasm_module.c

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,7 @@ ngx_http_wasm_create_main_conf(ngx_conf_t *cf)
265265
mcf->vm = ngx_wasm_main_vm(cf->cycle);
266266

267267
ngx_queue_init(&mcf->plans);
268+
ngx_proxy_wasm_init(cf, &mcf->store);
268269

269270
return mcf;
270271
}
@@ -281,9 +282,6 @@ ngx_http_wasm_init_main_conf(ngx_conf_t *cf, void *conf)
281282
return NGX_CONF_ERROR;
282283
}
283284

284-
ngx_proxy_wasm_init(cf);
285-
ngx_proxy_wasm_store_init(&mcf->store, cf->pool);
286-
287285
return NGX_CONF_OK;
288286
}
289287

@@ -445,8 +443,7 @@ ngx_http_wasm_exit_process(ngx_cycle_t *cycle)
445443

446444
mcf = ngx_http_cycle_get_module_main_conf(cycle, ngx_http_wasm_module);
447445
if (mcf) {
448-
ngx_proxy_wasm_exit();
449-
ngx_proxy_wasm_store_destroy(&mcf->store);
446+
ngx_proxy_wasm_exit(&mcf->store);
450447

451448
ngx_wasm_ops_destroy(mcf->ops);
452449
}

‎src/http/ngx_http_wasm_util.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -411,8 +411,8 @@ ngx_http_wasm_ops_add_filter(ngx_wasm_ops_plan_t *plan,
411411
goto error;
412412
}
413413

414-
filter->pool = plan->pool;
415414
filter->log = vm->log;
415+
filter->pool = store->pool;
416416
filter->store = store;
417417

418418
if (config) {

‎src/http/proxy_wasm/ngx_http_proxy_wasm.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ ngx_http_proxy_wasm_get_rctx(ngx_wavm_instance_t *instance)
1919

2020
pwexec = ngx_proxy_wasm_instance2pwexec(instance);
2121
pwctx = pwexec->parent;
22-
if (!pwctx) {
22+
if (pwctx == NULL) {
2323
return NULL;
2424
}
2525

‎src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -825,7 +825,7 @@ ngx_http_proxy_wasm_dispatch_resume_handler(ngx_wasm_socket_tcp_t *sock)
825825
/* save step */
826826
step = pwexec->parent->step;
827827

828-
ecode = ngx_proxy_wasm_run_step(pwexec, pwexec->ictx,
828+
ecode = ngx_proxy_wasm_run_step(pwexec,
829829
NGX_PROXY_WASM_STEP_DISPATCH_RESPONSE);
830830
if (ecode != NGX_PROXY_WASM_ERR_NONE) {
831831
goto error;

‎src/wasm/vm/ngx_wavm.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1345,12 +1345,12 @@ ngx_wavm_instance_destroy(ngx_wavm_instance_t *instance)
13451345
ngx_wavm_func_t *func;
13461346
ngx_wavm_module_t *module = instance->module;
13471347

1348-
ngx_log_debug5(NGX_LOG_DEBUG_WASM,
1348+
ngx_log_debug6(NGX_LOG_DEBUG_WASM,
13491349
instance->log ? instance->log : ngx_cycle->log, 0,
13501350
"wasm freeing \"%V\" instance in \"%V\" vm"
1351-
" (vm: %p, module: %p, instance: %p)",
1351+
" (vm: %p, module: %p, instance: %p, trapped: %d)",
13521352
&module->name, module->vm->name,
1353-
module->vm, module, instance);
1353+
module->vm, module, instance, instance->trapped);
13541354

13551355
if (instance->funcs.nelts) {
13561356
for (i = 0; i < instance->funcs.nelts; i++) {

‎t/03-proxy_wasm/003-on_tick.t

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -166,14 +166,14 @@ Should not prevent http context/instance from starting
166166
return 200;
167167
}
168168
--- response_body
169-
--- grep_error_log eval: qr/\[(info|crit)].*?on_tick.*/
169+
--- grep_error_log eval: qr/(\[crit\]|\[info].*?on_tick).*/
170170
--- grep_error_log_out eval
171171
qr/.*?
172172
\[info\] .*? on_tick 200.*
173-
\[crit\] .*? panicked at 'on_tick trap'.*
173+
\[crit\] .*? panicked at.*
174174
.*?
175175
\[info\] .*? on_tick 200.*
176-
\[crit\] .*? panicked at 'on_tick trap'.*/
176+
\[crit\] .*? panicked at.*/
177177
--- error_log
178178
filter 1/1 resuming "on_request_headers" step
179179
filter 1/1 resuming "on_response_headers" step

0 commit comments

Comments
 (0)