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

[release/6.0] Fix binary serialization of DateTime with DCS and add test. #75650

Conversation

StephenMolloy
Copy link
Member

@StephenMolloy StephenMolloy commented Sep 14, 2022

Fixes: #61528 in 6.0.
This exact change was already included as part of the big DCS/NetFx alignment done in 7.0. (#71752)

Customer Impact

The WCF binary XML format written by XmlDictionaryWriter.CreateBinaryWriter allows for a binary representation of DateTime objects that is shorter and faster to deserialize than the standard string presentation. NetFx used this binary format, but .Net Core has used a string representation. That leads to much larger blobs when the serialized objects contain many DateTimes and, more importantly, significantly longer deserialization times for some customers.

Testing

A test has been added to verify the use of the binary format vs string format for DateTime.

Regression

Yes? This is a regression from 4.8, though it has always been this way in .Net Core.

Risk

Low. This PR restores the NetFx way of serializing a DateTime value... which is to let XmlWriter handle it rather than injecting our own formatting. (I believe the custom formatting was only used because XmlWriter.WriteValue() did not have an overload for DateTime in .Net Core 1.0. It has since been available since 1.1.)

@ghost ghost assigned StephenMolloy Sep 14, 2022
@StephenMolloy StephenMolloy changed the title Fix binary serialization of DateTime with DCS and add test. [release/6.0] Fix binary serialization of DateTime with DCS and add test. Sep 14, 2022
@carlossanlop
Copy link
Member

@StephenMolloy please fill out the full template, add the servicing-consider label, and send email to Tactics to request approval.

@StephenMolloy StephenMolloy added this to the 6.0.x milestone Sep 15, 2022
@StephenMolloy StephenMolloy added the Servicing-consider Issue for next servicing release review label Oct 3, 2022
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Oct 4, 2022
@rbhanda rbhanda modified the milestones: 6.0.x, 6.0.11 Oct 4, 2022
@carlossanlop
Copy link
Member

carlossanlop commented Oct 5, 2022

Feedback addressed, approved by Tactics, signed-off by area owner, CI is green. Ready to merge. :shipit:

Edit: Almost forgot to mention that neither System.Private.DataContractSerialization nor System.Runtime.Serialization.Xml are OOB packages. They do not have <IsPackable>true in the src csproj, and they show up in the file https://github.com/dotnet/runtime/blob/release/6.0/src/libraries/NetCoreAppLibrary.props

@carlossanlop carlossanlop merged commit 3c55f1d into dotnet:release/6.0 Oct 5, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Serialization Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants