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/7.0] Restore Advance when reading float/double with XmlBufferReader. #80321

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jan 7, 2023

Backport of #80274 to release/7.0

Fixes #78913, #79056, and #79092

/cc @StephenMolloy

Customer Impact

A combination of performance and big-endian changes towards the end of 7.0 broke binary-deserialization of float/doubles in DataContractSerializer. Values are still serialized correctly, but 7.0 DCS clients fail when reading those primative types. The description on #80274 gets into more root-cause details.

Testing

Testing already existed for the correctness of the bytes being written and the order they get read back in... but that testing was targeted and shortcutted the buffered reader that DCS uses in end-to-end scenarios. This PR adds a quick round-trip check to our existing end-to-end DCS tests for most types.

Risk

Low. The change is small and represents an understood restoration of behavior from previous versions.

@StephenMolloy StephenMolloy self-assigned this Jan 7, 2023
@StephenMolloy StephenMolloy added this to the 7.0.x milestone Jan 7, 2023
@StephenMolloy StephenMolloy added the Servicing-consider Issue for next servicing release review label Jan 7, 2023
@StephenMolloy
Copy link
Member

runtime-dev-innerloop failure is #80284

@HongGit HongGit added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 10, 2023
@mmitche
Copy link
Member

mmitche commented Jan 11, 2023

@carlossanlop Is there a fix in progress for the source build issue

/cc @MichaelSimons @crummel

@carlossanlop
Copy link
Member

@carlossanlop Is there a fix in progress for the source build issue

Not yet. It's being actively investigated. Here is the issue tracking it: #80284

@carlossanlop
Copy link
Member

carlossanlop commented Jan 11, 2023

@mmitche @StephenMolloy is that issue a blocker for merging this PR?

Edit: I'll assume it's not a blocker, since Stephen mentioned it in a comment.

@carlossanlop carlossanlop modified the milestones: 7.0.x, 7.0.3 Jan 11, 2023
@carlossanlop
Copy link
Member

Approved by Tactics (7.0.3).
Signed off by area owner.
CI failure unrelated: #80284
No OOB changes needed for this PR because System.Private.DataContractSerialization.csproj does not have <IsPackable>true</IsPackable> set.
Ready to merge. :shipit:

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