fix: preserve StageActor demand across fused streams - #3461
Conversation
e4de67e to
efa4557
Compare
pjfanning
left a comment
There was a problem hiding this comment.
LGTM as it fixes the issue (or seems likely to).
Claude AI raised some concerns (below).
Two things I'd want addressed
- activeStage is clobbered by the nested execute().
runAsyncInput sets activeStage = logic (GraphInterpreter.scala:577) and calls afterStageHasRun(logic) at the end (line 592). The new execute(2) runs processEvent, which does activeStage = null (line 602) and then reassigns it to whichever connection's owner it processes (lines 616/636/655/666). So when handler(item) returns for message 2, activeStage is no longer this logic.
This is benign for finalization — afterStageHasRun takes the logic explicitly, so it finalizes the right stage. But reportStageError (line 442) uses the mutable activeStage field, not a parameter. If handler(item) throws for message 3, the error is attributed to, and failStage is called on, whatever stage the previous nested execute last touched. That's a misattributed failure — wrong stage failed, wrong stage named in the log.
I have not constructed a failing case for this, so I'd flag it as a question rather than a defect: is it worth saving and restoring activeStage around the execute(2) call? It's a two-line change if so.
- The 2 deserves a firmer justification than "sufficient".
The PR says "1 event slot for the push + chase budget for the pull". Reading execute (line 486), eventsRemaining is decremented per dequeued event and chaseCounter = math.min(ChaseLimit, eventsRemaining) — so with a limit of 2, the chase budget after the first dequeue is 1. That's enough for the simple push→pull chain the test exercises.
What I can't tell from the diff is what happens with a longer downstream chain inside the same shell, where the pull needs more than one chase hop to reach back. The events aren't lost — they stay queued and get drained by the runBatch that follows — but demand may still not be replenished by the time message N+1 is handled, which is exactly the bug. That would make the fix correct for the tested topology and partial for others.
A test with two or three map stages between the source and the async boundary would settle it. Worth asking the author whether that was considered.
|
Let's me check this again. |
…imit Motivation: Review on apache#3461 raised two concerns: (1) the nested interpreter.execute() call clobbers activeStage, so a throwing handler could see failures misattributed to another stage; (2) the hard-coded event limit of 2 only suffices for the simplest topology and lacked justification for longer fused chains. Modification: - Save and restore interpreter.activeStage around the execute() call in LazyDispatch.drain so it keeps denoting the draining logic while a message batch is dispatched. - Replace the magic 2 with LazyDispatch.ReplenishEventLimit = 16, covering push/pull propagation across longer in-shell fused chains; leftover events stay queued for the interpreter's outer run loop. - Extend the regression tests: wait for the initial demand via an onPull signal instead of racing the async subscription, add longer upstream and downstream chain variants, and assert activeStage attribution for every dispatched message. Result: - Demand is replenished per element across fused chains up to the event limit, activeStage stays correctly attributed, and the new tests fail without the fix (upstream chain times out with the previous limit of 2). Tests: - stream-tests/Test/testOnly org.apache.pekko.stream.FusingSpec -- -z replenish -z activeStage: 4/4 passed - stream-tests/Test/testOnly org.apache.pekko.stream.FusingSpec org.apache.pekko.stream.scaladsl.StageActorRefSpec: 34/34 passed - stream-tests/Test/testOnly org.apache.pekko.stream.scaladsl.ActorRefSourceSpec org.apache.pekko.stream.scaladsl.StreamRefsSpec: 45/45 passed - scalafmt --mode diff-ref=origin/main: applied - stream/mimaReportBinaryIssues: passed - git diff --check: clean References: Refs apache#3459, Refs apache#3461
|
@He-Pin this is still ok to merge for me - the new commit does seem to address the highlighted issues |
5678363 to
7529764
Compare
Motivation: Batched StageActor callbacks can observe stale outlet demand before interpreter events propagate across a fused graph. Fixed nested execute limits are topology-dependent and bypass the shell processing budget. Modification: Share the shell event quota with lazy StageActor dispatch. Drain or park callback-generated interpreter work before the next message, resume after the queue becomes idle, preserve active-stage ownership, and stop queued dispatch after completion or failure. Skip empty interpreter executions on callbacks that enqueue no graph work. Add directional coverage for deep fusion, a processing limit of one, completion, and failure. Result: Demand replenishment works for arbitrary fused topology depths without a magic event limit, while interpreter fairness and failure semantics remain effective. The direct StageActor benchmark finds no measurable throughput regression against main. Tests: - FusingSpec and related StageActor/stream specs: 80/80 passed - FusingSpec with fuzzing mode: 24/24 passed - LifecycleInterpreterSpec: 13/13 passed - stream MiMa: passed on Scala 2.13 and Scala 3.3.8 - scalafmt and git diff --check: passed - StageActorRefBenchmark: main 26.286M +/- 6.002M, PR 27.988M +/- 3.124M ops/s References: Fixes apache#3459
7529764 to
3f07c44
Compare
Motivation
StageActorlazy dispatch coalesces a burst into one async callback. A callback can enqueue interpreter work that replenishes downstream demand; dispatching the next message before that work runs makes it observe stale demand and can drop data, as reported in #3459.Fixed nested calls such as
interpreter.execute(2)orexecute(16)are not a general fix: the required work depends on the fused topology depth, and the nested work is not charged to the actor shell's processing quota. Restoring a savedactiveStageis also unsafe when interpreter execution may have completed and released that stage.Modification
StageActordispatch and the graph interpreter share the enclosing shell's event budget.StageActormessage. If the budget is exhausted, park the dispatcher and resume it only after the interpreter queue becomes idle; this has no topology-dependent event limit.GraphInterpreter.executewhen a callback enqueues no interpreter work, preserving the common no-graph-work hot path.activeStagebefore every callback without restoring a potentially finalized stage.sync-processing-limit = 1, stage completion, and callback failure.Result
Demand is replenished before the next
StageActormessage independently of fused topology depth. Actor/interpreter fairness remains bounded by the existing shell quota, lazy dispatch still coalesces mailbox traffic, and completed or failed stages are not dispatched again.The original
pekko-connectorsUDP reproduction was run from connector commit1c15f3b6f; onlyPekkoCoreDependency.currentVersionchanged:sbt "udp/test"2.0.0-M42.0.0-M4+14-3f07c449-SNAPSHOTThis directly verifies the connector regression, not only the reduced Core reproducer.
The direct
StageActorRefBenchmark.lazy_stage_actor_ref_tell_10kJMH benchmark was run on the same machine and OpenJDK 25.0.2 with-wi 3 -i 5 -w 2s -r 2s -f 3:origin/main(6be39c658f)The PR point estimate is 6.5% higher, but the confidence intervals overlap. The supported conclusion is that this run found no measurable throughput regression, not that the change is statistically faster.
Tests
pekko-connectors:sbt "udp/test"— 2.0.0-M4 failed 4/5; PR snapshot passed 5/5.sbt "stream-tests / Test / testOnly org.apache.pekko.stream.FusingSpec org.apache.pekko.stream.scaladsl.StageActorRefSpec org.apache.pekko.stream.scaladsl.ActorRefSourceSpec org.apache.pekko.stream.scaladsl.StreamRefsSpec"— 80/80 passed.sbt -Dpekko.stream.fuzzing-mode=on "stream-tests / Test / testOnly org.apache.pekko.stream.FusingSpec"— 24/24 passed with randomized event ordering and chasing disabled.sbt "stream-tests / Test / testOnly org.apache.pekko.stream.impl.fusing.LifecycleInterpreterSpec"— 13/13 passed.sbt validatePullRequest— Streams suite passed 3,418/3,418. The aggregate command failed outside this change: LevelDB JNI is unavailable on this ARM64 host; two Remote tests failed on local bind/TLS timing;IntegrationDocTestfailed while instantiatingUnboundedMailbox.sbt "stream / mimaReportBinaryIssues" "++3.3.8" "stream / mimaReportBinaryIssues"— passed.sbt headerCreateAll "+headerCheckAll" checkCodeStyle, native scalafmt, andgit diff --check— passed.sbt "bench-jmh / Jmh / run -wi 3 -i 5 -w 2s -r 2s -f 3 .*StageActorRefBenchmark.lazy_stage_actor_ref_tell_10k.*"— results shown above.References
Fixes #3459