fix: make BsdiffDiffer thread-safe for concurrent pipeline use - #450
Merged
Conversation
Remove per-call mutable instance fields (_oldfilePath, _newfilePath, _patchPath) that caused race conditions when a single BsdiffDiffer instance was shared across parallel DiffPipeline tasks. The Task.Run lambdas now capture parameters directly via closure, making each invocation independently thread-safe. Also add missing GeneralUpdate.Core project reference to DifferentialTest, and add comprehensive single/multi-file differential integration tests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes BsdiffDiffer safe to reuse concurrently (e.g., when DiffPipeline is configured with parallelism > 1) by removing per-call mutable instance state and passing file paths through the call stack. It also adds a comprehensive test suite that exercises single-file round-trips and multi-file parallel pipeline scenarios.
Changes:
- Refactor
BsdiffDifferto remove per-invocation instance fields and make parameter validation purely argument-based. - Add a new comprehensive differential test suite covering bsdiff/hdiff round-trips, pipeline parallelism, delete handling, progress, and cancellation.
- Add
GeneralUpdate.Coreproject reference to thesrc/c#/DifferentialTesttest project.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/DifferentialTest/ComprehensiveDifferentialTests.cs | Adds broad single-/multi-file and parallelism test coverage for differ + pipeline behavior. |
| src/c#/GeneralUpdate.Differential/Differ/BsdiffDiffer.cs | Removes shared mutable state and updates docs/validation to enable safe concurrent use. |
| src/c#/DifferentialTest/DifferentialTest.csproj | Adds a direct reference to GeneralUpdate.Core for tests that use Core pipeline/models. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Also add some new files in target (no old version) | ||
| File.WriteAllBytes(Path.Combine(tgt, "new_features.dll"), [1, 2, 3, 4, 5]); | ||
|
|
||
| var completedFiles = new ConcurrentBag<string>(); |
Comment on lines
+399
to
+402
| var progress = new SyncProgress<DiffProgress>(); | ||
| var pipeline = new DiffPipelineBuilder() | ||
| .WithParallelism(Environment.ProcessorCount) | ||
| .Build(); |
Comment on lines
+409
to
+412
| // Assert | ||
| Assert.True(progress.LastValue.IsComplete); | ||
| _output.WriteLine($"Progress: {progress.LastValue.Completed}/{progress.LastValue.Total}"); | ||
|
|
Comment on lines
+810
to
+812
| var pipeline = new DiffPipelineBuilder() | ||
| .WithParallelism(Environment.ProcessorCount) | ||
| .Build(); |
Comment on lines
+18
to
+19
| /// Thread-safety: this class is stateless beyond the compression provider. | ||
| /// A single instance is safe for concurrent calls. |
- Remove unused ConcurrentBag variable - Cap Environment.ProcessorCount in tests for stable CI - Strengthen DiffProgress assertions (check Total > 0 and Completed == Total) - Clarify BsdiffDiffer thread-safety doc to mention compression provider contract Co-Authored-By: Claude Opus 4.7 <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
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.
Summary
_oldfilePath,_newfilePath,_patchPathinstance fields fromBsdiffDifferthat caused race conditions under concurrent callsValidationParameters()now accepts parameters directly instead of reading shared mutable stateClean()andDirty()lambdas capture file paths via closure — each invocation gets its own copyBsdiffDifferis now safe to use withDiffPipelineBuilder.WithParallelism(>1)Root Cause
When
DiffPipelineruns with parallelism > 1, concurrent tasks share the sameBsdiffDifferinstance. The old code wrote per-call state (_oldfilePath,_newfilePath,_patchPath) to instance fields insideTask.Run, making them subject to race conditions. Thread A could overwrite thread B's fields between assignment and use.Test Plan
BsdiffDifferwithWithParallelism(4)inDiffPipelineproduces correct resultsDiffPipeline_CustomDiffer_BsdiffDiffer_Worksfrom parallelism=1 to parallelism=4, confirms fix🤖 Generated with Claude Code