Fix low-priority Pipeline review findings (volatile reads, sentinel exception, assertion strengthening)#138
Merged
Chris-Wolfgang merged 1 commit intoApr 25, 2026
Conversation
Addresses findings #6, #7, and #11 from the review. #6 — Use Volatile.Read when reading the Interlocked-incremented CurrentItemCount and CurrentSkippedItemCount fields on ExtractorBase, LoaderBase, and TransformerBase. Writers use Interlocked.Increment; pairing the reads with Volatile.Read closes a theoretical weak-memory-model read hazard (relevant primarily on older ARM NetFx targets) and is free on x86/x64. #7 — Replace the raw InvalidOperationException throws in the CancelOnly* test doubles with a sentinel WrongOverloadCalledException. A future Pipeline refactor that accidentally routes through a bare overload will now produce a unique, named exception in test output instead of a generic "Operation is not valid" message. #11 — Assert Assert.Empty(loader.Loaded) after the pre-cancelled-token cancellation tests on CancelOnlyTests and FullTests. Previously the tests only verified that SOME OperationCanceledException was thrown; a regression where cancellation fires mid-stream after one or two items would have still passed. 208/208 tests pass in Release on net10.0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.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
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.
Stacks on top of #134 (parallel to #137). Addresses the low-priority findings from the code review.
Fixes
CurrentItemCount/CurrentSkippedItemCountgetters read withoutVolatile.Read, while writers useInterlocked.Increment. Theoretical weak-memory-model hazard.Volatile.Read(ref _currentItemCount)inExtractorBase,LoaderBase,TransformerBase. Free on x86/x64, correct on ARM NetFx.InvalidOperationExceptionon unused overloads. A future routing bug surfaces as a confusing generic exception.WrongOverloadCalledExceptionsentinel;CancelOnly*doubles now throw it with the overload signature in the message.OperationCanceledExceptionwas thrown" — a regression where cancellation fires mid-stream after 1-2 items still passes.Assert.Empty(loader.Loaded)after the cancellation throw inCancelOnlyTestsandFullTests.Stats
Not included
Review findings #10 (informational — covariance doc note), #12 (wait for CI signal on
fieldkeyword), and #13 (style nitpick) were judged not worth the churn.🤖 Generated with Claude Code