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

ReadAsync length #17

Closed
alexis- opened this issue Apr 20, 2022 · 4 comments
Closed

ReadAsync length #17

alexis- opened this issue Apr 20, 2022 · 4 comments

Comments

@alexis-
Copy link

alexis- commented Apr 20, 2022

Hello,

First of all, this is a really nice project. I'm enjoying reading your code a lot, and this will definitely save me time so thanks. :)

While familiarizing myself with H.Pipes, I came across this bit in PipeStreamReader.cs

    private async Task<byte[]> ReadAsync(int length, CancellationToken cancellationToken = default)
    {
        var buffer = new byte[length];
#if NETSTANDARD2_1 || NETCOREAPP3_1_OR_GREATER
        await BaseStream.ReadAsync(buffer, cancellationToken).ConfigureAwait(false);
#elif NET461_OR_GREATER || NETSTANDARD2_0
        await BaseStream.ReadAsync(buffer, 0, buffer.Length, cancellationToken).ConfigureAwait(false);
#else
#error Target Framework is not supported
#endif

        return buffer;
    }

Shouldn't the return value (= number of bytes read) of ReadAsync be assigned and compared with the intended message length?

var readLength = await BaseStream.ReadAsync(buffer, cancellationToken).ConfigureAwait(false);

if (readLength != length)
  throw new IOException($"Expected {length} bytes but read {readLength}");
@HavenDV
Copy link
Owner

HavenDV commented Apr 21, 2022

Thanks for the code review. Yes, it really is worth it.
In fact, I'm surprised I don't have warnings about not using explicitly ignoring return values via the discard pattern _ = Method();
I'm trying to figure out how to enable this permanently for all my projects.
It seems this line in .editorconfig does not work in VS 2022 latest preview:

csharp_style_unused_value_assignment_preference = discard_variable:warning

@HavenDV
Copy link
Owner

HavenDV commented Apr 21, 2022

Had to tinker to find why this diagnostic didn't work:
dotnet/roslyn#60875

@HavenDV
Copy link
Owner

HavenDV commented Apr 21, 2022

Fixed 030e9ba

@alexis-
Copy link
Author

alexis- commented Apr 21, 2022

Already fixed! Nice 👍

It's surprising that the unused_value_assignment bug made is this far into the release cycle of VS2022.

@alexis- alexis- closed this as completed Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants