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

Fix issues related to JsonSerializerOptions mutation and caching. #65863

Merged
merged 3 commits into from
Mar 4, 2022

Conversation

eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Feb 24, 2022

Supersedes #65799. After conversation with @layomia in #65799 (comment) I realized that the caching implementation introduced in #64646 contains a bug. More specifically, a locked/immutable JsonSerializerOptions instance can still be mutated by the serialization infrastructure in couple of important ways:

Both of these changes alter the JsonTypeInfo resolution semantics in important ways, and as such they can invalidate the current contents of the metadata cache. This PR makes the following changes:

I ran the benchmark suite and no performance regressions were recorded.

@eiriktsarpalis eiriktsarpalis added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Feb 24, 2022
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Feb 24, 2022
@eiriktsarpalis eiriktsarpalis self-assigned this Feb 24, 2022
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Feb 24, 2022
@ghost
Copy link

ghost commented Feb 24, 2022

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@ghost
Copy link

ghost commented Feb 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

Supersedes #65799. After conversation with @layomia in #65799 (comment) I realized that the caching implementation introduced in #64646 contains a bug. More specifically, a locked/immutable JsonSerializerOptions instance can still be mutated by the serialization infrastructure in couple of important ways:

Both of these changes alter the JsonTypeInfo resolution semantics in important ways, and as such they can invalidate the current contents of the metadata cache. This PR makes the following changes:

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json, breaking-change

Milestone: 7.0.0

Copy link
Contributor

@layomia layomia left a comment

Choose a reason for hiding this comment

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

LGTM.

@jkotas
Copy link
Member

jkotas commented Mar 5, 2022

This PR was merged with failures in the tests introduced by this PR. I am reverting it in #66235. Please fix the test failures and resubmit.
cc @agocke

jkotas added a commit that referenced this pull request Mar 5, 2022
eiriktsarpalis added a commit to eiriktsarpalis/runtime that referenced this pull request Mar 5, 2022
…tnet#65863)

* Fix issues related to JsonSerializerOptions mutation and caching.

* fix test style

* fix linker warning
@eiriktsarpalis
Copy link
Member Author

Apologies for that, I've reposted the changes in #66248 with relevant tests disabled in netfx.

eiriktsarpalis added a commit that referenced this pull request Mar 7, 2022
…6248)

* Fix issues related to JsonSerializerOptions mutation and caching. (#65863)

* Fix issues related to JsonSerializerOptions mutation and caching.

* fix test style

* fix linker warning

* disable failing tests in netfx
@ghost ghost locked as resolved and limited conversation to collaborators Apr 5, 2022
@eiriktsarpalis eiriktsarpalis removed breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Sep 16, 2022
@eiriktsarpalis
Copy link
Member Author

I'm removing the breaking-change label since in hindsight, the removed behavior (i.e. ability to add a JsonSerializerContext to a locked JsonSerializerOptions instance) is fairly pathological (generally resulting in nondeterministic contract resolution in .NET 6) so it probably doesn't warrant mention in the documentation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants