Assign PropertyCache after it's configured for source gen in attempt to fix JSON LookupProperty assert failing #68480
Conversation
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsContributes to: #67816 Fixing one potential threading issue related to LookupProperty assert failing. This could possibly happen for source gen when two threads start using same JsonTypeInfo at the same time since. For source gen properties are late assigned meaning if one thread has already assigned and configured them the other might reset that state and first thread might start using it before the second thread finishes configuring. This PR changes so that assignment happens after cache is already configured so that we don't start using semi configured cache in neither thread. I'm not 100% certain this will fix the issue but it's likely. Once this is merged I'll observe if this repros again for another couple of days and if not we can assume it's fixed.
|
| { | ||
| if (PropertyInfoForTypeInfo.ConverterStrategy != ConverterStrategy.Object) | ||
| { | ||
| return; | ||
| return null; |
There was a problem hiding this comment.
Can we also exit early for typeof(T)?
There was a problem hiding this comment.
likely yes but I'd prefer to keep those changes separate
| } | ||
|
|
||
| if (PropertyInfoForTypeInfo.ConverterBase.ElementType != null) | ||
| { | ||
| // Nullable<> or F# optional converter's strategy is set to element's strategy | ||
| return; | ||
| return null; |
There was a problem hiding this comment.
Are there some Debug.Asserts that would be relevant to verify that this is a Nullable or F# optional?
There was a problem hiding this comment.
This specific check is happening because Nulalble<T> converter currently reports Object as its converter strategy but it actually never uses properties - IMO this is incorrect but I didn't want to do the needed refactoring to change - I was preserving existing behavior here. I'll refrain from doing this change in this PR
There was a problem hiding this comment.
This specific check is happening because Nulalble converter currently reports Object as its converter strategy but it actually never uses properties
It only will report Object if T is a struct. In any case it would make sense to an assertion here in case the ElementType assumption gets invalidated by a different converter in the future.
| { | ||
| foreach (var jsonPropertyInfoKv in PropertyCache.List) | ||
| foreach (var jsonPropertyInfoKv in propertyCache.List) |
There was a problem hiding this comment.
nit: should the declaring type's NumberHandling be passed to EnsureConfigured() as an argument?
There was a problem hiding this comment.
more likely this (declaring type JsonTypeInfo) perhaps because we don't really know if other info might be needed here. I'll refrain from changing it in this PR
| { | ||
| JsonPropertyInfo jsonPropertyInfo = jsonPropertyInfoKv.Value!; | ||
| jsonPropertyInfo.DeclaringTypeNumberHandling = NumberHandling; | ||
| jsonPropertyInfo.EnsureConfigured(); | ||
| } | ||
|
|
||
| // This code can be run in multiple threads therefore we assign only after all properties has been configured |
There was a problem hiding this comment.
I think this change makes the code more complicated without addressing the core issue, which is described in the comment here. It doesn't fix all potential race conditions hiding in the initialization code, nor does it prevent any future races from being introduced as the code gets changed.
The only real solution is to prevent multiple threads from attempting to write to an instance. There are a few approaches we could pursue longer term, but the immediate need is to unblock CI for others. As an interim solution I would recommend adding a double-checked lock in the EnsureConfigured() method.
There was a problem hiding this comment.
I was hoping we could achieve same without having to do locks but agreed it will make it harder to prevent from the regression to happen again. Will revert and lock
There was a problem hiding this comment.
actually let me close this PR and create a new one since it's effectively different change
Contributes to: #67816
Fixing one potential threading issue related to LookupProperty assert failing. This could possibly happen for source gen when two threads start using same JsonTypeInfo at the same time since. For source gen properties are late assigned meaning if one thread has already assigned and configured them the other might reset that state and first thread might start using it before the second thread finishes configuring. This PR changes so that assignment happens after cache is already configured so that we don't start using semi configured cache in neither thread.
I'm not 100% certain this will fix the issue (I don't have a local repro) but it's likely. Once this is merged I'll observe if this repros again for another couple of days and if not we can assume it's fixed.