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

Cache parameterized ctor delegates in class info rather than converter #34248

Merged
merged 3 commits into from
Mar 31, 2020

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Mar 29, 2020

Fixes #33928.

Caching parameterized ctor delegates in converters is not thread-safe. The delegates should be stored on the JsonClassInfo instance for the type, which is shared across all instances of the converter.

@layomia layomia added this to the 5.0 milestone Mar 29, 2020
@layomia layomia self-assigned this Mar 29, 2020
Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

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

LGTM

CI test failures are not related.

@layomia
Copy link
Contributor Author

layomia commented Mar 31, 2020

CI failure - #28553.

@layomia layomia merged commit 90d1b62 into dotnet:master Mar 31, 2020
@layomia layomia deleted the ctors_null_ref branch April 1, 2020 01:47
layomia added a commit to layomia/dotnet_runtime that referenced this pull request Apr 1, 2020
dotnet#34248)

* Cache parameterized ctor delegates in class info rather than converter

* Address review feedback - move delegate assignment to start of deserialization

* Address review feedback - nullability
Anipik pushed a commit that referenced this pull request Apr 3, 2020
#34248) (#34353)

* Cache parameterized ctor delegates in class info rather than converter

* Address review feedback - move delegate assignment to start of deserialization

* Address review feedback - nullability
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Text.Json.Serialization.Tests.ConstructorTests_StringTValue.OtherPropertiesAreSet fails with nullref
3 participants