fix: make WasmBase::doAfterVmCallActions reentry-safe (drain-to-local)#558
Open
johnlanni wants to merge 1 commit into
Open
fix: make WasmBase::doAfterVmCallActions reentry-safe (drain-to-local)#558johnlanni wants to merge 1 commit into
johnlanni wants to merge 1 commit into
Conversation
doAfterVmCallActions() drained after_vm_call_actions_ with an unguarded `while (!empty())` loop, popping and running one action at a time. If an action re-enqueues itself (or otherwise adds a new action) during the drain, the freshly-added action is observed by the same loop and executed again in the same frame. Under synchronous reentry -- e.g. a host call such as sendLocalReply / injectEncodedDataToFilterChain that re-enters the VM and schedules another after-VM-call action -- the loop never sees an empty queue and spins forever, pinning a CPU at 100%. Swap the member queue into a local std::deque before draining, then iterate the snapshot exactly once. Actions re-added 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 loop. This also prevents an action queued in one phase from executing inside a nested VM call of a different phase (see proxy-wasm#326). Adds a regression test (DoAfterVmCallActionsReentrySafe) that hangs forever on the old code and passes with the fix. Signed-off-by: 澄潭 <zty98751@alibaba-inc.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
WasmBase::doAfterVmCallActions()drains itsafter_vm_call_actions_queue with anunguarded
while (!after_vm_call_actions_.empty())loop, popping and running one actionat a time:
If an action re-enqueues itself (or otherwise adds a new action) during the drain,
the freshly-added action is observed by the same loop and executed again in the same
frame. Under synchronous reentry this loop never sees an empty queue and spins forever,
pinning a worker at 100% CPU.
This is reachable from a plugin today. A host call made from within an after-VM-call
action re-enters the VM synchronously, and that nested VM call's
DeferAfterCallActionsguard calls
doAfterVmCallActions()again. For example, in the Envoy host, a deferredonRedisCall/onHttpCallfailure callback that callsinjectEncodedDataToFilterChain(orsendLocalReply) re-enters the VM and schedulesanother after-VM-call action; the outer loop keeps observing a non-empty queue and never
returns.
The same shared-queue reentry is also the mechanism behind #326: an action queued in one
phase (e.g.
continueStream()fromonRequestHeaders) is drained by a nesteddoAfterVmCallActions()belonging to a different phase (e.g.onResponseHeaderstriggered by
sendLocalReply), so the action "escapes" into the wrong phase.Fix
Swap the member queue into a local
std::dequebefore draining, then iterate thesnapshot exactly once:
Actions re-added by a callback during the drain land in the (now-empty) member queue and
are therefore picked up by the next-outer
DeferAfterCallActionsframe, instead of beingre-run in this loop. This:
inside a nested VM call of another phase) — see
addAfterVmCallActionmay cause unexpected problems #326.executed, just deferred to the next-outer frame.
It is a header-only change to a small inline method, zero-ABI and transparent to plugins
(no signature or behavior change visible to Wasm modules).
Test
Adds
TEST_P(TestVm, DoAfterVmCallActionsReentrySafe)intest/wasm_test.cc, wired intothe existing
//test:wasm_testtarget. It registers a self-re-enqueuing action andasserts the count advances by exactly one per
doAfterVmCallActions()call. On the oldcode this test hangs forever (documenting the regression); with the fix each drain runs
the snapshot once and defers the re-added copy.
Refs
addAfterVmCallActionmay cause unexpected problems #326.deferred Redis-failure callback that injects into the filter chain).