-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Lock EnsureConfigured to prevent initialization issues in JsonTypeInfo #68605
Conversation
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsFixes: #67816 Replaces: #68480 It's theoretically possible to achieve this without having to use locks (which I've tried in the replaced PR) but:
|
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs
Show resolved
Hide resolved
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.
Could you also remove the various debug formatting methods?
@eiriktsarpalis if any of those debug ever fails the information will be very useful - I personally prefer to keep them but can remove if you feel strongly about it |
} | ||
|
||
internal virtual void Configure() | ||
{ | ||
Debug.Assert(Monitor.IsEntered(_configureLock), "Configure called directly, use EnsureConfigured which locks this method"); |
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.
Since this is virtual, might make sense to add the same to any overrides?
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.
they will call base.Configure anyway
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.
OK. That might change in future iterations, but I guess nothing prevents users from overriding the Configure method elsewhere either.
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.
Should the method be changed to protected virtual
instead?
I feel that they were specifically crafted for debugging the issue at hand. If other issues come by in the future the information might not be relevant. I think we should just remove them. |
They were made to debug any JsonTypeInfo/JsonPropertyInfo issues. There are still couple of other flaky issues other than the one mentioned - I'm not yet sure if that will be useful for debugging those or not. Also this is super useful when making changes and something goes wrong or gets regressed (I had to explore these information many times under debugger when doing prototyping for contract resolver and that actually makes it much easier to extract). I'll leave this for now and we can clean it up once we are confident our tests are not blocking CI runs |
There seem to be some infra issue, I couldn't see any timeout in any of the JSON helix items |
Fixes: #67816
Fixes: #68584
Contributes to: #67761
Replaces: #68480
It's theoretically possible to achieve this without having to use locks (which I've tried in the replaced PR) but: