Replace Stream.Read with .ReadExactly #345
Conversation
There is a new API that ensures that Stream.Read will read the number of bytes that we're expected. With .NET 9, the SDK raises a warning when ReadExactly should be used. This change updates usage to that, including updating (where possible) to use `Span<byte>` and `MemoryMarshal`.
| // Copy the input file stream to the end of the file | ||
| using (FileStream ifs = File.Open(inputFilename, FileMode.Open, FileAccess.Read, FileShare.Read)) | ||
| using (var ifs = File.Open(inputFilename, FileMode.Open, FileAccess.Read, FileShare.Read)) |
There was a problem hiding this comment.
Nit-pick but it looks like var doesn't match the coding style elsewhere in this file.
| byte[] buf = new byte[header.HashBuckets * 8]; | ||
| dataStream.Read(buf, 0, (int)buf.Length); | ||
| dataStream.ReadExactly(buf, 0, (int)buf.Length); |
There was a problem hiding this comment.
Stream.ReadExactly will throw if there aren't enough bytes to read. Will there certainly be enough bytes in dataStream here? Is throwing if there aren't enough bytes desirable? It might be safer to use Read but check the length of bytes actually read (and take whatever action is appropriate - possibly none - if the buffer isn't entirely filled). Checking the number of bytes read should also address the warning.
There was a problem hiding this comment.
yeah, this is intended. this is a parser for a binary format and expects to read a specified amount. previously, it just assumed to have read the amount without checking; that is the precise reason ReadExact was introduced. It would be better to throw when hitting those cases you mention than assuming things were read.
| @@ -378,53 +370,6 @@ public Stream GetFileStream(int tag, int level, int x, int y) | |||
| return null; | |||
| } | |||
|
|
|||
| public Stream GetFileStreamFullSearch(int tag, int level, int x, int y) | |||
There was a problem hiding this comment.
This is a public method. Have you checked that it doesn't have any callers?
There was a problem hiding this comment.
yup - we don't use reflection to call anything and no callers exist anymore that need it
| f.Close(); | ||
| } | ||
| return ms; | ||
| var fs = File.Open(filename, FileMode.Open, FileAccess.Read, FileShare.ReadWrite); |
There was a problem hiding this comment.
Again, I don't see var used elsewhere.
| public override int ReadByte() | ||
| { | ||
| Span<byte> buffer = stackalloc byte[1]; | ||
| return Read(buffer) == 0 ? -1 : buffer[0]; |
There was a problem hiding this comment.
What is this for? Reading random bytes from the stack frame? I'm not sure if we can depend on the precise location buffer is allocated from not changing depending on the context this is called from.
There was a problem hiding this comment.
The default implementation of ReadByte() will allocate a 1 entry array for each read. Instead, this will allocate it on the stack and use the Read(Span<byte>) overload to fill it. If it didn't read, it would return -1, while if it did, the value is returned.
| fs.Close(); | ||
| await context.Response.OutputStream.WriteAsync(data, 0, demSize, token); | ||
| using var fs = File.Open(filename, FileMode.Open, FileAccess.Read, FileShare.ReadWrite); | ||
| using var slice = StreamSlice.Create(fs, demSize * tileX, demSize); |
| public override int ReadByte() | ||
| { | ||
| Span<byte> buffer = stackalloc byte[1]; | ||
| return Read(buffer) == 0 ? -1 : buffer[0]; |
There is a new API that ensures that Stream.Read will read the number of bytes that we're expected. With .NET 9, the SDK raises a warning when ReadExactly should be used. This change updates usage to that, including updating (where possible) to use
Span<byte>andMemoryMarshal.