Skip to content

Fix async XsdValidatingReader to not use synchronious IO #84888

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 19, 2023
Merged

Fix async XsdValidatingReader to not use synchronious IO #84888

merged 3 commits into from
May 19, 2023

Conversation

mla-alm
Copy link

@mla-alm mla-alm commented Apr 15, 2023

Fixes #78802

@ghost ghost added area-System.Xml community-contribution Indicates that the PR has been added by a community member labels Apr 15, 2023
@ghost
Copy link

ghost commented Apr 15, 2023

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

Issue Details

Fixes #78802

Author: mla-alm
Assignees: -
Labels:

area-System.Xml
Milestone: -

@Wraith2
Copy link
Contributor

Wraith2 commented Apr 16, 2023

I don't think you need to change the ProcessReaderEventAsync method to async. It looks like you only need to change the way that the GetString sync methods are called, replacing them with GetValueAsync as you have done and then adding c continuation to pass them to the validator and returning the task to the caller. This adds layer of compiler generated async machinery which doesn't seem needed.

@mla-alm
Copy link
Author

mla-alm commented Apr 17, 2023

Thanks, @Wraith2, (hopefully) changed as you suggested.

@mla-alm
Copy link
Author

mla-alm commented May 16, 2023

@stephentoub Any more input?

@layomia layomia requested a review from krwq May 16, 2023 15:28
@adamsitnik adamsitnik requested a review from stephentoub May 18, 2023 18:19
stephentoub
stephentoub approved these changes May 19, 2023
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@stephentoub stephentoub merged commit 335f2ef into dotnet:main May 19, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

@krwq krwq Awaiting requested review from krwq

+1 more reviewer

@Wraith2 Wraith2 Wraith2 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
area-System.Xml community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Synchronous calls in XsdValidatingReader
3 participants
@mla-alm @Wraith2 @stephentoub