diff --git a/include/proxy-wasm/wasm.h b/include/proxy-wasm/wasm.h index 10c16a79..74c69a6b 100644 --- a/include/proxy-wasm/wasm.h +++ b/include/proxy-wasm/wasm.h @@ -147,9 +147,21 @@ class WasmBase : public std::enable_shared_from_this { // NB: this may be deleted by a delayed function unless prevented. if (!after_vm_call_actions_.empty()) { auto self = shared_from_this(); - while (!self->after_vm_call_actions_.empty()) { - auto f = std::move(self->after_vm_call_actions_.front()); - self->after_vm_call_actions_.pop_front(); + // Swap the queue into a local before draining, then iterate the snapshot exactly + // once. Actions re-added by a callback during the drain land in the (now-empty) + // member queue and are picked up by the next-outer DeferAfterCallActions frame, + // instead of being re-run in this same loop. With the old unguarded + // `while (!empty())`, an action that (directly or transitively) re-enqueues during + // the drain -- e.g. a host call such as sendLocalReply/injectEncodedDataToFilterChain + // that synchronously re-enters the VM and schedules another after-VM-call action -- + // means the loop never observes an empty queue and spins forever, pinning a CPU at + // 100%. Deferring re-added actions to the next-outer frame also prevents an action + // queued in one phase (e.g. continueStream from onRequestHeaders) from executing + // inside a nested VM call of a different phase (e.g. onResponseHeaders triggered by + // sendLocalReply); see proxy-wasm/proxy-wasm-cpp-host#326. + std::deque> local; + local.swap(self->after_vm_call_actions_); + for (auto &f : local) { f(); } } diff --git a/test/wasm_test.cc b/test/wasm_test.cc index 2a3e60cc..7aed8330 100644 --- a/test/wasm_test.cc +++ b/test/wasm_test.cc @@ -324,4 +324,32 @@ TEST_P(TestVm, CleanupThreadLocalCacheKeys) { EXPECT_TRUE(stale_wasms_keys.empty()); } +// doAfterVmCallActions() must not spin forever when a queued action re-enqueues itself +// during the drain. With the old unguarded `while (!empty())` loop this loops forever; +// with the drain-to-local fix each call runs the snapshot exactly once and the re-added +// copy is deferred to the next drain. +TEST_P(TestVm, DoAfterVmCallActionsReentrySafe) { + // WasmBase::doAfterVmCallActions() calls shared_from_this(), so the instance must be + // owned by a std::shared_ptr (a stack WasmBase would crash). + auto wasm = std::make_shared(makeVm(engine_)); + + int count = 0; + std::function f; + f = [&]() { + ++count; + // Re-enqueue self during the drain. Under the old code this would be picked up by the + // same loop and never terminate. + wasm->addAfterVmCallAction(f); + }; + + wasm->addAfterVmCallAction(f); + // Snapshot {f} runs exactly once; the re-added copy lands in the now-empty member queue. + wasm->doAfterVmCallActions(); + EXPECT_EQ(count, 1); // Returns after one pass (the old while-loop would never return). + + // The re-added copy runs on the next frame (and re-enqueues once more). + wasm->doAfterVmCallActions(); + EXPECT_EQ(count, 2); +} + } // namespace proxy_wasm