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

Allow reading more than the minimum number when filling buffer in XmlBufferReader #72847

Conversation

Daniel-Svensson
Copy link
Contributor

@Daniel-Svensson Daniel-Svensson commented Jul 26, 2022

Read as much as possible into the buffer without limiting to minimum count
Affects at least XmlDictionaryReader

The current code looked a bit odd with ReadAtLeast while still limiting number of bytes read to the minimum number of bytes to read.

 Read as much as possible into the buffer without limiting to minimum count
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 26, 2022
@Daniel-Svensson Daniel-Svensson changed the title Fix incorrect usage of ReadAtLeast i in XmlDictionaryReader Fix incorrect usage of Stream.ReadAtLeast i in XmlBufferReader Jul 26, 2022
@Daniel-Svensson Daniel-Svensson changed the title Fix incorrect usage of Stream.ReadAtLeast i in XmlBufferReader Allow reading more than the minimum number when filling buffer in XmlBufferReader Jul 26, 2022
@stephentoub
Copy link
Member

Affects at least XmlDictionaryReader

Performance or correctness? If the latter, what test can we add?

@stephentoub
Copy link
Member

cc: @eerhardt

@@ -231,7 +231,7 @@ private bool TryEnsureBytes(int count)
}
int needed = newOffsetMax - _offsetMax;
DiagnosticUtility.DebugAssert(needed > 0, "");
int read = _stream.ReadAtLeast(_buffer.AsSpan(_offsetMax, needed), needed, throwOnEndOfStream: false);
int read = _stream.ReadAtLeast(_buffer.AsSpan(_offsetMax), needed, throwOnEndOfStream: false);
Copy link
Member

Choose a reason for hiding this comment

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

Prior to the change to use ReadAtLeast, this was also limiting how much was read to needed. Is it possible changing it to read more could create bugs? For example, is it possible this might end up reading more from the stream that a consumer intended, such that it would erroneously consume something from the stream after what is being deserialized?

Copy link
Contributor Author

@Daniel-Svensson Daniel-Svensson Jul 26, 2022

Choose a reason for hiding this comment

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

Good catch, I am thinking about closing this for now and maybe add a comment in a separate PR.

Is there any other reader which have any way around that (such as resetting the stream position) and if so how late (dispose or after each read) would it be ok to reset the position?

Copy link
Member

Choose a reason for hiding this comment

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

Resetting position isn't always possible: many (most) streams don't support it.

We should just close this PR for now. Thanks, though.

@eerhardt
Copy link
Member

The current code looked a bit odd with ReadAtLeast while still limiting number of bytes read to the minimum number of bytes to read.

Note, this code needs to use ReadAtLeast in order to handle EOF correctly. This doesn't want to throw an exception when encountering EOF - it just wants to return false;. If we had an overload to ReadExactly with throwOnEndOfStream, this would use ReadExactly and pass in throwOnEndOfStream: false. But there isn't an overload because ReadExactly is meant to be the "easy" API for most consumers.

@Daniel-Svensson Daniel-Svensson deleted the xmlbufferreader_readatleast branch July 26, 2022 20:24
@Daniel-Svensson Daniel-Svensson restored the xmlbufferreader_readatleast branch August 3, 2022 16:29
@ghost ghost locked as resolved and limited conversation to collaborators Sep 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Serialization community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants