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

[C] avoid concurrent access to _values #24579

Merged
merged 1 commit into from
Sep 27, 2024
Merged

[C] avoid concurrent access to _values #24579

merged 1 commit into from
Sep 27, 2024

Conversation

StephaneDelcroix
Copy link
Contributor

@samhouts samhouts added the area-xaml XAML, CSS, Triggers, Behaviors label Sep 4, 2024
Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Just so the machines can stop nagging me about this PR, I think locking around the setting of the field is also worth doing just to make sure that there is no code path that also allows double setting.

Unless that is somehow impossible because of something else.

@@ -14,6 +14,8 @@ internal class SetterSpecificityList
KeyValuePair<SetterSpecificity, object>? _second;
SortedList<SetterSpecificity, object>? _values;

object _lock = new();
Copy link
Contributor

Choose a reason for hiding this comment

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

Locks are typically readonly.

@MartyIX
Copy link
Contributor

MartyIX commented Sep 27, 2024

I wonder why the lock is necessary in the first place. Is SetterSpecificityList.cs to be accessed from multiple threads?

If access from multiple threads is expected, then why _values is locked but not

KeyValuePair<SetterSpecificity, object>? _first;
KeyValuePair<SetterSpecificity, object>? _second;
?

Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

Seems this could help with the issue. Without a way to reproduce with a test is not easy to make sure it will actually work. But lets try

@MartyIX
Copy link
Contributor

MartyIX commented Sep 27, 2024

The original issue mentions #23597

*CurrentNode is extended form StackLayout which has some hierarchy childs.

My understanding is that StackLayout should not be used in the first place but rather {Horizontal|Vertical}StackLayout and it seems suspicious that it happens on a customized control. There is a good chance that there is a bug in that custom control.

Anyway, just my 50c.

@rmarinho rmarinho merged commit f7d4914 into main Sep 27, 2024
97 checks passed
@rmarinho rmarinho deleted the fix_23597 branch September 27, 2024 12:56
@StephaneDelcroix
Copy link
Contributor Author

@MartyIX we agree that this shouldn't happen, if used correctly. but if this fix prevents a crash while the user abuses the API, it's worth it

rmarinho pushed a commit that referenced this pull request Sep 30, 2024
@samhouts samhouts added fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! labels Oct 1, 2024
@PureWeen PureWeen added this to the .NET 9 SR1 milestone Oct 20, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Nov 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-xaml XAML, CSS, Triggers, Behaviors fixed-in-net8.0-nightly This may be available in a nightly release! fixed-in-net9.0-nightly This may be available in a nightly release!
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Collection was modified after the enumerator was instantiated.
6 participants