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.

Sign up for GitHub

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

Jump to bottom

Fix Options Source Gen RangeAttribute Thread Safety #97045

Merged
merged 4 commits into from
Jan 17, 2024
Merged

Fix Options Source Gen RangeAttribute Thread Safety #97045

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8d2f31
Fix Options Source Gen RangeAttribute Thread Safety
tarekgh Jan 16, 2024
f6a2f6b
Address the feedback
tarekgh Jan 17, 2024
fe7fa3b
More feedback addressing
tarekgh Jan 17, 2024
f9a7f91
Feedback++
tarekgh Jan 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 44 additions & 38 deletions src/libraries/Microsoft.Extensions.Options/gen/Emitter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,26 +382,26 @@ public void EmitRangeAttribute(string modifier, string prefix, string className,

string initializationString = emitTimeSpanSupport ?
"""
if (OperandType == typeof(global::System.TimeSpan))
{
if (!global::System.TimeSpan.TryParse((string)Minimum, culture, out global::System.TimeSpan timeSpanMinimum) ||
!global::System.TimeSpan.TryParse((string)Maximum, culture, out global::System.TimeSpan timeSpanMaximum))
{
throw new global::System.InvalidOperationException(c_minMaxError);
}
Minimum = timeSpanMinimum;
Maximum = timeSpanMaximum;
}
else
{
Minimum = ConvertValue(Minimum, culture) ?? throw new global::System.InvalidOperationException(c_minMaxError);
Maximum = ConvertValue(Maximum, culture) ?? throw new global::System.InvalidOperationException(c_minMaxError);
}
if (OperandType == typeof(global::System.TimeSpan))
{
if (!global::System.TimeSpan.TryParse((string)Minimum, culture, out global::System.TimeSpan timeSpanMinimum) ||
!global::System.TimeSpan.TryParse((string)Maximum, culture, out global::System.TimeSpan timeSpanMaximum))
{
throw new global::System.InvalidOperationException(MinMaxError);
}
Minimum = timeSpanMinimum;
Maximum = timeSpanMaximum;
}
else
{
Minimum = ConvertValue(Minimum, culture) ?? throw new global::System.InvalidOperationException(MinMaxError);
Maximum = ConvertValue(Maximum, culture) ?? throw new global::System.InvalidOperationException(MinMaxError);
}
"""
:
"""
Minimum = ConvertValue(Minimum, culture) ?? throw new global::System.InvalidOperationException(c_minMaxError);
Maximum = ConvertValue(Maximum, culture) ?? throw new global::System.InvalidOperationException(c_minMaxError);
Minimum = ConvertValue(Minimum, culture) ?? throw new global::System.InvalidOperationException(MinMaxError);
Maximum = ConvertValue(Maximum, culture) ?? throw new global::System.InvalidOperationException(MinMaxError);
""";

string convertValue = emitTimeSpanSupport ?
Expand Down Expand Up @@ -470,7 +470,7 @@ public void EmitRangeAttribute(string modifier, string prefix, string className,
public {{qualifiedClassName}}(global::System.Type type, string minimum, string maximum) : base()
{
OperandType = type;
NeedToConvertMinMax = true;
_needToConvertMinMax = true;
Minimum = minimum;
Maximum = maximum;
}
Expand All @@ -483,33 +483,39 @@ public void EmitRangeAttribute(string modifier, string prefix, string className,
public bool ConvertValueInInvariantCulture { get; set; }
public override string FormatErrorMessage(string name) =>
string.Format(global::System.Globalization.CultureInfo.CurrentCulture, GetValidationErrorMessage(), name, Minimum, Maximum);
private bool NeedToConvertMinMax { get; }
private bool Initialized { get; set; }
private const string c_minMaxError = "The minimum and maximum values must be set to valid values.";
private readonly bool _needToConvertMinMax;
private volatile bool _initialized;
private const string MinMaxError = "The minimum and maximum values must be set to valid values.";

public override bool IsValid(object? value)
{
if (!Initialized)
if (!_initialized)
{
if (Minimum is null || Maximum is null)
{
throw new global::System.InvalidOperationException(c_minMaxError);
}
if (NeedToConvertMinMax)
lock (this)
{
System.Globalization.CultureInfo culture = ParseLimitsInInvariantCulture ? global::System.Globalization.CultureInfo.InvariantCulture : global::System.Globalization.CultureInfo.CurrentCulture;
if (!_initialized)
{
if (Minimum is null || Maximum is null)
{
throw new global::System.InvalidOperationException(MinMaxError);
}
if (_needToConvertMinMax)
{
System.Globalization.CultureInfo culture = ParseLimitsInInvariantCulture ? global::System.Globalization.CultureInfo.InvariantCulture : global::System.Globalization.CultureInfo.CurrentCulture;
{{initializationString}}
}
int cmp = ((global::System.IComparable)Minimum).CompareTo((global::System.IComparable)Maximum);
if (cmp > 0)
{
throw new global::System.InvalidOperationException("The maximum value '{Maximum}' must be greater than or equal to the minimum value '{Minimum}'.");
}
else if (cmp == 0 && (MinimumIsExclusive || MaximumIsExclusive))
{
throw new global::System.InvalidOperationException("Cannot use exclusive bounds when the maximum value is equal to the minimum value.");
}
_initialized = true;
}
}
int cmp = ((global::System.IComparable)Minimum).CompareTo((global::System.IComparable)Maximum);
if (cmp > 0)
{
throw new global::System.InvalidOperationException("The maximum value '{Maximum}' must be greater than or equal to the minimum value '{Minimum}'.");
}
else if (cmp == 0 && (MinimumIsExclusive || MaximumIsExclusive))
{
throw new global::System.InvalidOperationException("Cannot use exclusive bounds when the maximum value is equal to the minimum value.");
}
Initialized = true;
}

if (value is null or string { Length: 0 })
Expand Down
52 changes: 29 additions & 23 deletions ...tions/tests/SourceGeneration.Unit.Tests/Baselines/EmitterWithCustomValidator.netcore.g.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public __SourceGen__RangeAttribute(double minimum, double maximum) : base()
public __SourceGen__RangeAttribute(global::System.Type type, string minimum, string maximum) : base()
{
OperandType = type;
NeedToConvertMinMax = true;
_needToConvertMinMax = true;
Minimum = minimum;
Maximum = maximum;
}
Expand All @@ -97,34 +97,40 @@ public __SourceGen__RangeAttribute(global::System.Type type, string minimum, str
public bool ConvertValueInInvariantCulture { get; set; }
public override string FormatErrorMessage(string name) =>
string.Format(global::System.Globalization.CultureInfo.CurrentCulture, GetValidationErrorMessage(), name, Minimum, Maximum);
private bool NeedToConvertMinMax { get; }
private bool Initialized { get; set; }
private const string c_minMaxError = "The minimum and maximum values must be set to valid values.";
private readonly bool _needToConvertMinMax;
private volatile bool _initialized;
private const string MinMaxError = "The minimum and maximum values must be set to valid values.";

public override bool IsValid(object? value)
{
if (!Initialized)
if (!_initialized)
{
if (Minimum is null || Maximum is null)
lock (this)
{
throw new global::System.InvalidOperationException(c_minMaxError);
if (!_initialized)
{
if (Minimum is null || Maximum is null)
{
throw new global::System.InvalidOperationException(MinMaxError);
}
if (_needToConvertMinMax)
{
System.Globalization.CultureInfo culture = ParseLimitsInInvariantCulture ? global::System.Globalization.CultureInfo.InvariantCulture : global::System.Globalization.CultureInfo.CurrentCulture;
Minimum = ConvertValue(Minimum, culture) ?? throw new global::System.InvalidOperationException(MinMaxError);
Maximum = ConvertValue(Maximum, culture) ?? throw new global::System.InvalidOperationException(MinMaxError);
}
int cmp = ((global::System.IComparable)Minimum).CompareTo((global::System.IComparable)Maximum);
if (cmp > 0)
{
throw new global::System.InvalidOperationException("The maximum value '{Maximum}' must be greater than or equal to the minimum value '{Minimum}'.");
}
else if (cmp == 0 && (MinimumIsExclusive || MaximumIsExclusive))
{
throw new global::System.InvalidOperationException("Cannot use exclusive bounds when the maximum value is equal to the minimum value.");
}
_initialized = true;
}
}
if (NeedToConvertMinMax)
{
System.Globalization.CultureInfo culture = ParseLimitsInInvariantCulture ? global::System.Globalization.CultureInfo.InvariantCulture : global::System.Globalization.CultureInfo.CurrentCulture;
Minimum = ConvertValue(Minimum, culture) ?? throw new global::System.InvalidOperationException(c_minMaxError);
Maximum = ConvertValue(Maximum, culture) ?? throw new global::System.InvalidOperationException(c_minMaxError);
}
int cmp = ((global::System.IComparable)Minimum).CompareTo((global::System.IComparable)Maximum);
if (cmp > 0)
{
throw new global::System.InvalidOperationException("The maximum value '{Maximum}' must be greater than or equal to the minimum value '{Minimum}'.");
}
else if (cmp == 0 && (MinimumIsExclusive || MaximumIsExclusive))
{
throw new global::System.InvalidOperationException("Cannot use exclusive bounds when the maximum value is equal to the minimum value.");
}
Initialized = true;
}

if (value is null or string { Length: 0 })
Expand Down
52 changes: 29 additions & 23 deletions ...Options/tests/SourceGeneration.Unit.Tests/Baselines/EmitterWithCustomValidator.netfx.g.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ public __SourceGen__RangeAttribute(double minimum, double maximum) : base()
public __SourceGen__RangeAttribute(global::System.Type type, string minimum, string maximum) : base()
{
OperandType = type;
NeedToConvertMinMax = true;
_needToConvertMinMax = true;
Minimum = minimum;
Maximum = maximum;
}
Expand All @@ -95,34 +95,40 @@ public __SourceGen__RangeAttribute(global::System.Type type, string minimum, str
public bool ConvertValueInInvariantCulture { get; set; }
public override string FormatErrorMessage(string name) =>
string.Format(global::System.Globalization.CultureInfo.CurrentCulture, GetValidationErrorMessage(), name, Minimum, Maximum);
private bool NeedToConvertMinMax { get; }
private bool Initialized { get; set; }
private const string c_minMaxError = "The minimum and maximum values must be set to valid values.";
private readonly bool _needToConvertMinMax;
private volatile bool _initialized;
private const string MinMaxError = "The minimum and maximum values must be set to valid values.";

public override bool IsValid(object? value)
{
if (!Initialized)
if (!_initialized)
{
if (Minimum is null || Maximum is null)
lock (this)
{
throw new global::System.InvalidOperationException(c_minMaxError);
if (!_initialized)
{
if (Minimum is null || Maximum is null)
{
throw new global::System.InvalidOperationException(MinMaxError);
}
if (_needToConvertMinMax)
{
System.Globalization.CultureInfo culture = ParseLimitsInInvariantCulture ? global::System.Globalization.CultureInfo.InvariantCulture : global::System.Globalization.CultureInfo.CurrentCulture;
Minimum = ConvertValue(Minimum, culture) ?? throw new global::System.InvalidOperationException(MinMaxError);
Maximum = ConvertValue(Maximum, culture) ?? throw new global::System.InvalidOperationException(MinMaxError);
}
int cmp = ((global::System.IComparable)Minimum).CompareTo((global::System.IComparable)Maximum);
if (cmp > 0)
{
throw new global::System.InvalidOperationException("The maximum value '{Maximum}' must be greater than or equal to the minimum value '{Minimum}'.");
}
else if (cmp == 0 && (MinimumIsExclusive || MaximumIsExclusive))
{
throw new global::System.InvalidOperationException("Cannot use exclusive bounds when the maximum value is equal to the minimum value.");
}
_initialized = true;
}
}
if (NeedToConvertMinMax)
{
System.Globalization.CultureInfo culture = ParseLimitsInInvariantCulture ? global::System.Globalization.CultureInfo.InvariantCulture : global::System.Globalization.CultureInfo.CurrentCulture;
Minimum = ConvertValue(Minimum, culture) ?? throw new global::System.InvalidOperationException(c_minMaxError);
Maximum = ConvertValue(Maximum, culture) ?? throw new global::System.InvalidOperationException(c_minMaxError);
}
int cmp = ((global::System.IComparable)Minimum).CompareTo((global::System.IComparable)Maximum);
if (cmp > 0)
{
throw new global::System.InvalidOperationException("The maximum value '{Maximum}' must be greater than or equal to the minimum value '{Minimum}'.");
}
else if (cmp == 0 && (MinimumIsExclusive || MaximumIsExclusive))
{
throw new global::System.InvalidOperationException("Cannot use exclusive bounds when the maximum value is equal to the minimum value.");
}
Initialized = true;
}

if (value is null or string { Length: 0 })
Expand Down
Loading
Loading