-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Add Stream argument validation helpers and use throughout dotnet/runtime #43968
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
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
src/libraries/System.Net.Connections/src/System/Net/Connections/DuplexPipeStream.cs
Outdated
Show resolved
Hide resolved
geoffkizer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1500 fewer lines of code, nice :)
5937aca to
90f5952
Compare
90f5952 to
4fac81c
Compare
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/DeflateStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO/tests/BinaryWriter/BinaryWriter.WriteByteCharTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO/tests/BinaryWriter/BinaryWriter.WriteByteCharTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.HttpListener/src/System/Net/HttpRequestStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.HttpListener/src/System/Net/HttpRequestStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/MemoryStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/MemoryStream.cs
Outdated
Show resolved
Hide resolved
Stream.Null was using BlockingBegin/EndRead/Write, but the operations are nops (they don't even do argument validation), so the functionality was unnecessary complication. We can just use TaskToApm instead, with already completed tasks, and then also delete the SynchronousAsyncResult that was used by these operations. Also, the Begin/EndRead/Write methods were checking CanRead/Write and throwing if they return false, but this is a private sealed type with CanRead/Write hardcoded to return true! Delete. Finally added consistency across Stream.Null to checking for cancellation.
Also consolidate a few resources and use a few existing ThrowHelpers where appropriate.
4fac81c to
ea62688
Compare
carlossanlop
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good, thank you for the PR.
I left some questions asking for clarification of some of the changes.
Fixes #43176
cc: @geoffkizer, @carlossanlop, @jozkee, @ericstj