Incomplete data returned from StreamByteBuffer

[jovinson-ms]

Describe the bug

Summary: stream reading in StreamByteBuffer is incomplete.
When a DicomFile is created from a Stream, by default large tags will be saved as a StreamByteBuffer, so that the data can be retrieved on demand but not block other operations. When the bytes are requested from the StreamByteBuffer, the Data accessor and GetByteRange both make a single call to Stream.Read. However, the underlying Stream is not guaranteed to have all of the requested bytes available, and so what can be returned is a byte array with only part of the requested data.

This came to light in the context of an Azure Storage LazyLoadingReadOnlyStream, which buffers 4MB by default. The returned byte array for a pixel data element > 4MB will have 4MB of data and the rest of the array will be null values.

As this issue makes clear, this is expected behavior for all Streams, and so the consumer should check the return value of calls to Stream.Read until no more bytes are returned. The same logic is already implemented in the CopyToStream* methods.

To Reproduce
Here's a code sample that uses a toy buffered stream to simulate the buffering behavior, and gets StreamByteBuffer.Data with the existing logic (which results in incomplete data) versus updated logic (which results in complete data).

Expected behavior
I expect StreamByteBuffer.Data to return the entire requested range of bytes. There should be no concerns about blocking since this is an on-demand retrieval.

Environment
Fellow Oak DICOM version: (5.0.2)
OS: (Windows 10 x64)
Platform: (all)

[gofal]

Thats a very good point, Thanks for showing up this issue and also for presenting the solution.
So the solution is simple: the StreamByteBuffer.Data getter needs to have that do-while loop. In case hte underlying stream always has the data, this will only execute the body of this loop once, so it is not worse that until now. But in case the underlying stream does buffering, this solves the issue.

Do you want to make a PR for that?

[jovinson-ms]

Sure, I'll get one up.

[jovinson-ms]

When should I expect this fix to be released? Looking forward to pulling the changes.

[gofal]

usually there is a release every few months.
There are no special actions assigned with a release. We take the code from development branch, create nuget-packag from that and publish it on nuget. There is quite some administrative internal work, like updating some readmes, update api-doc, etc.
So if you want to have the bugfix in your product, then you can take the code from here as it is. Or do you require the nuget-package from nuget.org for some certain reason?

[jovinson-ms]

Thanks, @gofal - we currently consume the nuget package as part of https://github.com/microsoft/dicom-server. That's the main way we prefer to consume the code for a few different reasons, but we'll look into building based on a pinned commit.

I don't intend to pester the maintainers for small fixes / bugs but I thought this was a pretty important fix since it causes data to be missing in a request. We do have a workaround using ReadAll, so we should be ok for now.

[gofal]

Sure, I see. This bugfix will justify a release. I will plan to do an update until next week.