-
Notifications
You must be signed in to change notification settings - Fork 770
Fix race condition in concurrent DeploymentState access causing intermittent AzureDeployerTests failures #11974
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
Merged
Merged
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
41c8220
Initial plan
Copilot 51ae33a
Fix race condition in JsonExtensions.Prop causing AzureDeployerTests …
Copilot eeb2ee7
Add thread-safety tests for JsonExtensions.Prop
Copilot 0e9dba7
Use ConditionalWeakTable for lock objects instead of locking on JsonO…
Copilot af312b2
Move thread-safety from JsonExtensions.Prop to ProvisioningContext level
Copilot c4f5c9c
Revert JsonExtensions.cs to original implementation
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
106 changes: 106 additions & 0 deletions
106
tests/Aspire.Hosting.Azure.Tests/JsonExtensionsTests.cs
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Text.Json.Nodes; | ||
|
|
||
| namespace Aspire.Hosting.Azure.Tests; | ||
|
|
||
| public class JsonExtensionsTests | ||
| { | ||
| [Fact] | ||
| public async Task Prop_ConcurrentAccess_DoesNotThrow() | ||
| { | ||
| // Arrange | ||
| var rootJson = new JsonObject(); | ||
| const int threadCount = 10; | ||
| const int iterationsPerThread = 100; | ||
| var tasks = new Task[threadCount]; | ||
|
|
||
| // Act - Multiple threads accessing the same property concurrently | ||
| for (int i = 0; i < threadCount; i++) | ||
| { | ||
| int threadId = i; | ||
| tasks[i] = Task.Run(() => | ||
| { | ||
| for (int j = 0; j < iterationsPerThread; j++) | ||
| { | ||
| // All threads try to get or create the same "Azure" property | ||
| var azureNode = rootJson.Prop("Azure"); | ||
|
|
||
| // Each thread also creates a unique property | ||
| var threadNode = azureNode.Prop($"Thread{threadId}"); | ||
|
|
||
| // And a shared property under Azure | ||
| var deploymentsNode = azureNode.Prop("Deployments"); | ||
|
|
||
| // Access a deeper nested property | ||
| var resourceNode = deploymentsNode.Prop($"Resource{j % 5}"); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| // Assert - Should complete without exceptions | ||
| await Task.WhenAll(tasks).WaitAsync(TimeSpan.FromSeconds(10)); | ||
|
|
||
| // Verify the structure was created correctly | ||
| Assert.NotNull(rootJson["Azure"]); | ||
| var azureObj = rootJson["Azure"]!.AsObject(); | ||
| Assert.NotNull(azureObj["Deployments"]); | ||
|
|
||
| // Check that all thread-specific nodes were created | ||
| for (int i = 0; i < threadCount; i++) | ||
| { | ||
| Assert.NotNull(azureObj[$"Thread{i}"]); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Prop_ReturnsExistingNode_WhenNodeAlreadyExists() | ||
| { | ||
| // Arrange | ||
| var rootJson = new JsonObject(); | ||
| var azureNode = rootJson.Prop("Azure"); | ||
| azureNode.AsObject()["TestProperty"] = "TestValue"; | ||
|
|
||
| // Act | ||
| var retrievedNode = rootJson.Prop("Azure"); | ||
|
|
||
| // Assert | ||
| Assert.Same(azureNode, retrievedNode); | ||
| Assert.Equal("TestValue", retrievedNode["TestProperty"]!.GetValue<string>()); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Prop_CreatesNewNode_WhenNodeDoesNotExist() | ||
| { | ||
| // Arrange | ||
| var rootJson = new JsonObject(); | ||
|
|
||
| // Act | ||
| var newNode = rootJson.Prop("NewProperty"); | ||
|
|
||
| // Assert | ||
| Assert.NotNull(newNode); | ||
| Assert.Same(rootJson["NewProperty"], newNode); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Prop_NestedAccess_CreatesHierarchy() | ||
| { | ||
| // Arrange | ||
| var rootJson = new JsonObject(); | ||
|
|
||
| // Act | ||
| var deeply = rootJson.Prop("Level1") | ||
| .Prop("Level2") | ||
| .Prop("Level3") | ||
| .Prop("Level4"); | ||
|
|
||
| // Assert | ||
| Assert.NotNull(rootJson["Level1"]); | ||
| Assert.NotNull(rootJson["Level1"]!["Level2"]); | ||
| Assert.NotNull(rootJson["Level1"]!["Level2"]!["Level3"]); | ||
| Assert.NotNull(rootJson["Level1"]!["Level2"]!["Level3"]!["Level4"]); | ||
| Assert.Same(deeply, rootJson["Level1"]!["Level2"]!["Level3"]!["Level4"]); | ||
| } | ||
| } |
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.
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.
Locking on the JsonObject instance itself can be problematic as it's a public object that external code might also lock on, potentially causing deadlocks. Consider using a private static readonly object or ConcurrentDictionary for thread-safe operations instead.
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.
@copilot ^
Uh oh!
There was an error while loading. Please reload this page.
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.
I agree with this. I think thread safety would be better added to prevent multiple threads from using the JSON at once, rather than making
Propmethod itself safe.Making
Propsafe against itself just kicks the problem down to the next thing that breaks when multiple threads are reading/writing JSON at the same time.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.
@copilot Address the feedback here.
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.
@copilot consider this feedback. Look at how the json objects are being used and from where. And how we can make it thread safe for writes, and reads.
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.
@copilot This feedback has not been correctly addressed. Look at how the json objects are being used and from where. And how we can make it thread safe for writes, and reads. And then fix that. Add relevant tests too.