Skip to content
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

Fix Options Source Gen RangeAttribute Thread Safety #97045

Merged

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented Jan 16, 2024

#97037

The source generator for the RangeAttribute creates code optimizing the generation of multiple instances of the RangeAttribute with identical parameters when used in various locations. However, the initialization code within the generated RangeAttribute was not thread safe. This presents an issue when the initialization code is executed concurrently from multiple threads. The solution is straightforward: implement thread safety measures in the initialization code.

@ghost ghost assigned tarekgh Jan 16, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 16, 2024
@tarekgh tarekgh added this to the 9.0.0 milestone Jan 16, 2024
@tarekgh tarekgh added area-Extensions-Options source-generator Indicates an issue with a source generator feature and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jan 16, 2024
@ghost
Copy link

ghost commented Jan 16, 2024

Tagging subscribers to this area: @dotnet/area-extensions-options
See info in area-owners.md if you want to be subscribed.

Issue Details

#97037

The source generator for the RangeAttribute creates code optimizing the generation of multiple instances of the RangeAttribute with identical parameters when used in various locations. However, the initialization code within the generated RangeAttribute was not thread safe. This presents an issue when the initialization code is executed concurrently from multiple threads. The solution is straightforward: implement thread safety measures in the initialization code.

Author: tarekgh
Assignees: tarekgh
Labels:

area-Extensions-Options, source-generator

Milestone: 9.0.0

@tarekgh tarekgh force-pushed the FixOptionsSourceGenRangeAttributeThreadSafety branch from 3a02e31 to c8d2f31 Compare January 16, 2024 21:11
@tarekgh
Copy link
Member Author

tarekgh commented Jan 16, 2024

reviewers, please hide the white space diffs when reviewing the change for simplicity.

@@ -338,47 +338,53 @@ public __SourceGen__RangeAttribute(global::System.Type type, string minimum, str
public bool ConvertValueInInvariantCulture { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these properties that have a public setter, who is calling set on them, and when? They're used as part of IsValid, so I'm wondering if it's possible they might be changed concurrently with its execution?

Copy link
Member Author

@tarekgh tarekgh Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is helper property usually called only when declaring the RangeAttribute. Something like [RangeAttribute(typeof(TimeSpan), "01:00:00", "23:59:59", ConvertValueInInvariantCulture = true)].

@tarekgh
Copy link
Member Author

tarekgh commented Jan 17, 2024

/backport to release/8.0

Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/7560572213

Copy link
Contributor

@tarekgh backporting to release/8.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix Options Source Gen RangeAttribute Thread Safety
Using index info to reconstruct a base tree...
M	src/libraries/Microsoft.Extensions.Options/gen/Emitter.cs
A	src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Baselines/OptionsExtendingSystemClassTest.netcore.g.cs
A	src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Baselines/OptionsExtendingSystemClassTest.netfx.g.cs
M	src/libraries/Microsoft.Extensions.Options/tests/SourceGenerationTests/Baselines/NetCoreApp/Validators.g.cs
M	src/libraries/Microsoft.Extensions.Options/tests/SourceGenerationTests/Baselines/NetFX/Validators.g.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/Microsoft.Extensions.Options/tests/SourceGenerationTests/Baselines/NetFX/Validators.g.cs
Auto-merging src/libraries/Microsoft.Extensions.Options/tests/SourceGenerationTests/Baselines/NetCoreApp/Validators.g.cs
CONFLICT (modify/delete): src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Baselines/OptionsExtendingSystemClassTest.netfx.g.cs deleted in HEAD and modified in Fix Options Source Gen RangeAttribute Thread Safety. Version Fix Options Source Gen RangeAttribute Thread Safety of src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Baselines/OptionsExtendingSystemClassTest.netfx.g.cs left in tree.
CONFLICT (modify/delete): src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Baselines/OptionsExtendingSystemClassTest.netcore.g.cs deleted in HEAD and modified in Fix Options Source Gen RangeAttribute Thread Safety. Version Fix Options Source Gen RangeAttribute Thread Safety of src/libraries/Microsoft.Extensions.Options/tests/SourceGeneration.Unit.Tests/Baselines/OptionsExtendingSystemClassTest.netcore.g.cs left in tree.
Auto-merging src/libraries/Microsoft.Extensions.Options/gen/Emitter.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix Options Source Gen RangeAttribute Thread Safety
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Copy link
Contributor

@tarekgh an error occurred while backporting to release/8.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

@tarekgh
Copy link
Member Author

tarekgh commented Jan 17, 2024

I opened the issue #97112 for tracking the unrelated failure

@tarekgh tarekgh merged commit f5a97b2 into dotnet:main Jan 17, 2024
107 of 111 checks passed
@tarekgh tarekgh deleted the FixOptionsSourceGenRangeAttributeThreadSafety branch January 17, 2024 21:57
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
* Fix Options Source Gen RangeAttribute Thread Safety

* Address the feedback

* More feedback addressing

* Feedback++
@github-actions github-actions bot locked and limited conversation to collaborators Feb 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Options source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants