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

Ensure serializer reflection-dependency sentinel is accurate #67104

Merged

Conversation

eiriktsarpalis
Copy link
Member

Backport of #66522 addressing possible race condition introduced by #65898.

Customer Impact

This is an internal find prompted by test failures in our CI matrix. A race condition in static initialization can result in a NullReferenceException when multiple threads attempt to serialize for the first time. In principle we believe that this issue could also impact 6.0, however differences in the static initialization implementation makes it less likely to occur. I have not been able to reproduce the bug in 6.0 when stress testing for the scenario in question.

Testing

Merging #66522 in main eliminated any further occurrences of the issue in CI testing.

Risk

Low.

@ghost
Copy link

ghost commented Mar 24, 2022

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

Issue Details

Backport of #66522 addressing possible race condition introduced by #65898.

Customer Impact

This is an internal find prompted by test failures in our CI matrix. A race condition in static initialization can result in a NullReferenceException when multiple threads attempt to serialize for the first time. In principle we believe that this issue could also impact 6.0, however differences in the static initialization implementation makes it less likely to occur. I have not been able to reproduce the bug in 6.0 when stress testing for the scenario in question.

Testing

Merging #66522 in main eliminated any further occurrences of the issue in CI testing.

Risk

Low.

Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 6.0.x milestone Mar 24, 2022
@eiriktsarpalis eiriktsarpalis added the Servicing-consider Issue for next servicing release review label Mar 24, 2022
@@ -30,10 +31,10 @@ public sealed partial class JsonSerializerOptions
[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
private void RootBuiltInConverters()
{
if (s_defaultSimpleConverters is null)
if (Volatile.Read(ref s_defaultFactoryConverters) is null)
Copy link
Member

Choose a reason for hiding this comment

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

I know this could not be a direct backport. So I assume the comments in the main change do not apply here? https://github.com/dotnet/runtime/pull/66522/files#diff-143351693c2cb653ad82e34086cfd8863b0bda66c8b498ed8a400924c4f5224e

Copy link
Member Author

Choose a reason for hiding this comment

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

They do, but this is fairly standard use of Volatile so I don't believe it warrants explanation.

@ericstj
Copy link
Member

ericstj commented Mar 24, 2022

This will need our servicing changes added after we do branding for 6.0.5, alternatively we can just do that as part of the branding by leaving GeneratePackageOnBuild and incrementing ServicingVersion @carlossanlop

@leecow leecow modified the milestones: 6.0.x, 6.0.5 Mar 24, 2022
@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 24, 2022
@carlossanlop
Copy link
Member

carlossanlop commented Mar 24, 2022

This will need our servicing changes added after we do branding for 6.0.5, alternatively we can just do that as part of the branding by leaving GeneratePackageOnBuild and incrementing ServicingVersion @carlossanlop

I think it's better to do it here, for clarity.

@eiriktsarpalis you'll just need to bump the <ServicingVersion> number to 4. The <GeneratePackageOnBuild> boolean needs to stay true. This change will be merged after branding, and in branding one of the steps is to reset all these booleans to false. I'll make sure to double check this value is set correctly before I merge it when the time comes.

<GeneratePackageOnBuild>true</GeneratePackageOnBuild>
<ServicingVersion>3</ServicingVersion>

@jkotas

This comment was marked as off-topic.

@jkotas

This comment was marked as off-topic.

@leecow
Copy link
Member

leecow commented Apr 13, 2022

This should be merged asap to make the May releases.

@carlossanlop carlossanlop merged commit 49c4a15 into dotnet:release/6.0 Apr 13, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants