test: fix flaky FusingSpec activeStage assertion - #3496
Open
pjfanning wants to merge 1 commit into
Open
Conversation
Motivation: FusingSpec "leave activeStage cleared when a stage actor callback completes its stage" (added in apache#3461) intermittently failed observing the downstream IgnoreSink as activeStage. The stage the test guards against leaking - the async source - was cleared correctly; the sink was still being finalized. The completion future the test awaits (Sink.ignore's materialized value) is completed from within the sink's own onUpstreamFinish, while activeStage still references the sink, before the interpreter runs releaseStage to clear it. The test thread then read the non-volatile activeStage in a race with that clear. Modification: Poll the final assertion with awaitAssert so it settles once the interpreter goes idle, instead of reading activeStage once and racing the clear. Rename the local to avoid shadowing GraphStageLogic.interpreter in the inner stage. Result: The test asserts the same invariant without the timing race. Tests: - sbt "stream-tests/testOnly org.apache.pekko.stream.FusingSpec" - 24 passed References: Fixes apache#3494
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
1 participant
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.
Motivation
FusingSpec— "leave activeStage cleared when a stage actor callback completes its stage" (added in #3461) — is intermittently failing (#3494), observing the downstreamIgnoreSinkasactiveStagerather thannull.The invariant #3461 introduced is holding: the async source stage the test guards against leaking was cleared. The reported
activeStageis the downstreamSink.ignore, which the interpreter is still finalizing.done(Sink.ignore's materialized future) is completed from withinIgnoreSink.onUpstreamFinish, at which pointactiveStagestill references the sink, and only afterwards does the interpreter runreleaseStageto clear it (GraphInterpreter.scala:821). The test thread, woken bydone, read the non-volatileactiveStagein a race with that clear — usually sawnull, occasionally the sink.So this is a test-synchronization race, not a product bug.
Modification
Poll the final assertion with
awaitAssert, so it settles once the interpreter reaches idle, instead of readingactiveStageonce and racing the clear. Also rename the local frominterpretertocompletedInterpreterto avoid shadowingGraphStageLogic.interpreterinside the inner stage (a fatal warning under the project's-Wconf).Result
The test asserts the same invariant —
activeStageends upnullafter the callback completes its stage — without the timing race.Tests
sbt "stream-tests/testOnly org.apache.pekko.stream.FusingSpec"— 24 passedReferences
Fixes #3494