Add initial set of conformance tests for Stream - #43834

Merged
stephentoub merged 24 commits into
dotnet:masterfrom
stephentoub:streamconformance
Oct 28, 2020
Merged

Add initial set of conformance tests for Stream#43834
stephentoub merged 24 commits into
dotnet:masterfrom
stephentoub:streamconformance

Conversation

@stephentoub

@stephentoubstephentoub commented Oct 26, 2020

Copy link
Copy Markdown
Member

This also includes fixes to a variety of streams where they're out of conformance, in particular around argument names.

The tests being added are primarily focused on "connected streams", ones where two streams are used to represent two ends of a communication channel. We can subsequently flesh this out for other kinds of streams. Also, while I consolidated a bunch of tests that were spread across different stream-derived type tests, there's still more that can be consolidated over time into these centralized tests.

cc: @geoffkizer, @ericstj, @terrajobst

@Dotnet-GitSync-BotDotnet-GitSync-Bot added the area-Infrastructure-coreclr Only use for closed issues label Oct 26, 2020
@stephentoubstephentoub added area-System.IO breaking-change Issue or PR that represents a breaking API or functional change over a previous release. and removed area-Infrastructure-coreclr Only use for closed issues labels Oct 26, 2020
@ghostghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 26, 2020
@stephentoubstephentoub added this to the 6.0.0 milestone Oct 26, 2020
@stephentoub
stephentoubforce-pushed the streamconformance branch 3 times, most recently from 56ecc67 to 7f45cf0CompareOctober 27, 2020 00:42
@runfoapprunfoappBot mentioned this pull request Oct 27, 2020
Closed
geoffkizer
geoffkizer approved these changes Oct 27, 2020
View reviewed changes

@geoffkizergeoffkizer left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general this looks really good. A few minor comments below.

Are you planning additional work here, or is this basically done?

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs Outdated
Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs Outdated
Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs Outdated
Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs Outdated
Comment thread...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs Outdated
Comment threadsrc/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs Outdated
Comment threadsrc/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs Outdated
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/NetworkStreamTest.cs Outdated
@geoffkizer

geoffkizer commented Oct 27, 2020

Copy link
Copy Markdown
Contributor

I think we should also consider using the WrappedConnectedStreamTests for HttpClient request/response streams. This is a little weird because we don't actually have a Stream implementation on the server side -- but we could hack this together on top of the various loopback server stuff fairly easily. Thoughts?

@stephentoub

stephentoub commented Oct 27, 2020

Copy link
Copy Markdown
MemberAuthor

I think we should also consider using the WrappedConnectedStreamTests for HttpClient request/response streams. This is a little weird because we don't actually have a Stream implementation on the server side -- but we could hack this together on top of the various loopback server stuff fairly easily. Thoughts?

There are more streams in dotnet/runtime than I've included here. My thinking was we'd get this framework in, and then folks can add additional streams to test and additional tests over time. I got many of the heavy hitters here.

@stephentoub
stephentoubforce-pushed the streamconformance branch 2 times, most recently from 7108d31 to d139d8aCompareOctober 28, 2020 02:05
@geoffkizer

geoffkizer commented Oct 28, 2020

Copy link
Copy Markdown
Contributor

@scalablecory Can we go ahead and fix the Dispose behavior in QuicStream? Any reason to hold off on this? There are other issues in #756 that may need additional consideration, but it would be nice to fix the Dispose issues and make these conformance tests work for QuicStream.

@scalablecory

scalablecory commented Oct 28, 2020

Copy link
Copy Markdown
Contributor

@scalablecory Can we go ahead and fix the Dispose behavior in QuicStream? Any reason to hold off on this? There are other issues in #756 that may need additional consideration, but it would be nice to fix the Dispose issues and make these conformance tests work for QuicStream.

No reason to hold off. It has been a thorn in my side for HTTP/3 so I would very much like to fix it :).

@stephentoub

stephentoub commented Oct 28, 2020

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented Oct 28, 2020

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

stephentoub
stephentoub commented Oct 28, 2020
View reviewed changes
Comment thread...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs Outdated

@stephentoubstephentoubOct 28, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@geoffkizer, fyi, my machine isn't set up to run these, and they don't appear to run in CI yet (correct me if I'm wrong). So you may need to tweak them once they're merged.

@stephentoub
Fix invalid assert in StreamBuffer
ef00c0d
This can be hit by calling code and thus shouldn't be an assert. As an assert it prevents testing.
@stephentoub
Add ConnectedStreams.CreateUnidirectional
a590bcf
@stephentoub
Add initial set of Stream conformance tests
f63c227
These primarily focus on "connected streams", ones that can be arranged to communicate with each other in a producer/consumer pattern or as a bidirectional communication mechanism, e.g. NetworkStream, PipeStream, SslStream wrapped around a NetworkStream, FileStream created around pipes, etc. Later we can add more tests focused on standalone streams, e.g. FileStream created for an on-disk file, MemoryStream, etc.
@stephentoub
Add ConnectedStreams tests
59238b7
These are currently helpers used by many other tests. At some point they could become public API as well.
@stephentoub
Fix NetworkStream argument names
0d61950
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Add stream conformance tests for NetworkStream
b20d581
@stephentoub
Add stream conformance tests for QuicStream
80e15eb
@stephentoub
Fix several CryptoStream behaviors
a7f6964
1. Flushing a stream that wraps another stream for writing should always flush that underlying stream, even if no additional data was written as part of the flush.
2. Argument validation should validate buffers are not null rather than null ref'ing on a null buffer.
3. Checks for the CryptoStream mode should come after argument validation.
@stephentoub
Add stream conformance tests for CryptoStream
78bc0ac
@stephentoub
Fix FileStream argument names
3586739
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Fix BufferedStream argument names
4f32db2
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Add stream conformance tests for FileStream
cad5f6a
Specifically when used in a connected fashion, wrapped around an anonymous or named pipe.
@stephentoub
Add stream conformance tests for BufferedStream
ec88166
Specifically when used in a connected fashion, wrapped around some other connected stream.
@stephentoub
Add stream conformance tests for SslStream and NegotiateStream
3f6a0f7
@stephentoub
Fix PipeStream.Flush to not fail on readable streams
fb1de39
Consumers may expect Stream.Flush to be a nop if the stream is readable only, but PipeStream.Flush is throwing in that case. Stop doing that.
@stephentoub
Add stream conformance tests for PipeStream
fb55c4a
@stephentoub
Fix several BrotliStream behaviors
c6fa2f1
1. When passed a null buffer, it's throwing an exception with the argument name "array", even though the parameter's name is "buffer".
2. Even if there's no data written as part of the Flush{Async} call on a writeable stream, it should be calling flush on the wrapped stream.
@stephentoub
Fix several DeflateStream (and friends) issues
af13fb9
1. DeflateStream.Flush{Async} when writing needs to always flush the underlying stream, even if there's no data written as part of the flush call itself.
2. Several byte[] array arguments should be byte[] buffer. Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
3. DeflateStream.EndRead/Write needs to do additional state validation.
4. Not a bug, but simplify ReadAsync to match the sync Read implementation flow.
@stephentoub
Add stream conformance tests for Deflate/ZLib/GZip/BrotliStream
296b99a
@stephentoub
Fix PipeReader/WriterStream argument validation
230c853
@stephentoub
Add stream conformance tests for PipeReader/WriterStream
5e5afba
@stephentoub
Remove erroneous asserts from Stream.ReadWriteTask
6dc26b6
@stephentoub
Address PR feedback
c46cdd1
@stephentoub
Fix a few tests in CI
ac6aff5
@stephentoub
stephentoub merged commit 0acbafe into dotnet:masterOct 28, 2020
@stephentoub
stephentoub deleted the streamconformance branch October 28, 2020 13:36
@runfoapprunfoappBot mentioned this pull request Nov 2, 2020
Closed
@ericstjericstj mentioned this pull request Nov 12, 2020
Merged
@ghostghost locked as resolved and limited conversation to collaborators Dec 6, 2020
@gewarren

gewarren commented Mar 2, 2021

Copy link
Copy Markdown
Contributor

@stephentoub@PriyaPurkayastha Can you create a breaking change docs issue for this here? I believe this went out in Preview 1.

@stephentoub

stephentoub commented Mar 4, 2021

Copy link
Copy Markdown
MemberAuthor

@gewarren, I've opened dotnet/docs#23102. Thanks.

@stephentoub

stephentoub commented Sep 30, 2021

Copy link
Copy Markdown
MemberAuthor

@ericstj, you assigned this to me. What's the action item?

@ericstj

ericstj commented Sep 30, 2021

Copy link
Copy Markdown
Member

Mail is coming shortly. I'm going through all PRs with needs-breaking-change-doc and asking folks to file those issues and remove the label.

@stephentoub

stephentoub commented Sep 30, 2021

Copy link
Copy Markdown
MemberAuthor

Ok. As noted above in the comments, dotnet/docs#23102 was already opened and addressed.

@stephentoubstephentoub removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Sep 30, 2021
@ericstj

ericstj commented Sep 30, 2021

Copy link
Copy Markdown
Member

Thanks for removing the label, that will remove this from my queries.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@geoffkizergeoffkizergeoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stephentoubstephentoub

Labels

area-System.IObreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@stephentoub@geoffkizer@scalablecory@gewarren@ericstj@Dotnet-GitSync-Bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Add initial set of conformance tests for Stream - #43834
Merged
stephentoub merged 24 commits into
dotnet:masterfrom 
stephentoub:streamconformance
Oct 28, 2020
Merged
Add initial set of conformance tests for Stream#43834
stephentoub merged 24 commits into
dotnet:masterfrom 
stephentoub:streamconformance
Conversation
@stephentoub
@stephentoubstephentoub
commented
Oct 26, 2020

Copy link


Copy Markdown

Member
This also includes fixes to a variety of streams where they're out of conformance, in particular around argument names.
The tests being added are primarily focused on "connected streams", ones where two streams are used to represent two ends of a communication channel. We can subsequently flesh this out for other kinds of streams. Also, while I consolidated a bunch of tests that were spread across different stream-derived type tests, there's still more that can be consolidated over time into these centralized tests.
cc: @geoffkizer, @ericstj, @terrajobst
@Dotnet-GitSync-BotDotnet-GitSync-Bot
added
the 
area-Infrastructure-coreclr
Only use for closed issues
label
Oct 26, 2020
@stephentoubstephentoub
added

area-System.IO

breaking-change
Issue or PR that represents a breaking API or functional change over a previous release.
and removed

area-Infrastructure-coreclr
Only use for closed issues
labels
Oct 26, 2020
@ghostghost
added
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Oct 26, 2020
@stephentoubstephentoub
added this to the 6.0.0 milestone
Oct 26, 2020
@stephentoub
stephentoubforce-pushed
the
streamconformance
branch
3 times, most recently
from
56ecc67 to
7f45cf0CompareOctober 27, 2020 00:42
@runfoapprunfoappBot
mentioned this pull request
Oct 27, 2020

Closed

geoffkizer
geoffkizer
approved these changes
Oct 27, 2020
View reviewed changes
@geoffkizergeoffkizer
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

In general this looks really good. A few minor comments below.
Are you planning additional work here, or is this basically done?
Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment thread...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/NetworkStreamTest.cs
Outdated

@geoffkizer
geoffkizer
commented
Oct 27, 2020

Copy link


Copy Markdown

Contributor
I think we should also consider using the WrappedConnectedStreamTests for HttpClient request/response streams. This is a little weird because we don't actually have a Stream implementation on the server side -- but we could hack this together on top of the various loopback server stuff fairly easily. Thoughts?
@stephentoub
stephentoub
commented
Oct 27, 2020

Copy link


Copy Markdown

MemberAuthor
I think we should also consider using the WrappedConnectedStreamTests for HttpClient request/response streams. This is a little weird because we don't actually have a Stream implementation on the server side -- but we could hack this together on top of the various loopback server stuff fairly easily. Thoughts?
There are more streams in dotnet/runtime than I've included here. My thinking was we'd get this framework in, and then folks can add additional streams to test and additional tests over time. I got many of the heavy hitters here.
@stephentoub
stephentoubforce-pushed
the
streamconformance
branch
2 times, most recently
from
7108d31 to
d139d8aCompareOctober 28, 2020 02:05
@geoffkizer
geoffkizer
commented
Oct 28, 2020

Copy link


Copy Markdown

Contributor
@scalablecory Can we go ahead and fix the Dispose behavior in QuicStream? Any reason to hold off on this? There are other issues in #756 that may need additional consideration, but it would be nice to fix the Dispose issues and make these conformance tests work for QuicStream.
@scalablecory
scalablecory
commented
Oct 28, 2020

Copy link


Copy Markdown

Contributor
@scalablecory Can we go ahead and fix the Dispose behavior in QuicStream? Any reason to hold off on this? There are other issues in #756 that may need additional consideration, but it would be nice to fix the Dispose issues and make these conformance tests work for QuicStream.
No reason to hold off. It has been a thorn in my side for HTTP/3 so I would very much like to fix it :).
@stephentoub
stephentoub
commented
Oct 28, 2020

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
stephentoub
stephentoub
commented
Oct 28, 2020
View reviewed changes
Comment thread...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs
Outdated

@stephentoubstephentoubOct 28, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@geoffkizer, fyi, my machine isn't set up to run these, and they don't appear to run in CI yet (correct me if I'm wrong). So you may need to tweak them once they're merged.
@stephentoub
Fix invalid assert in StreamBuffer
ef00c0d
This can be hit by calling code and thus shouldn't be an assert. As an assert it prevents testing.
@stephentoub
Add ConnectedStreams.CreateUnidirectional
a590bcf
@stephentoub
Add initial set of Stream conformance tests
f63c227
These primarily focus on "connected streams", ones that can be arranged to communicate with each other in a producer/consumer pattern or as a bidirectional communication mechanism, e.g. NetworkStream, PipeStream, SslStream wrapped around a NetworkStream, FileStream created around pipes, etc. Later we can add more tests focused on standalone streams, e.g. FileStream created for an on-disk file, MemoryStream, etc.
@stephentoub
Add ConnectedStreams tests
59238b7
These are currently helpers used by many other tests. At some point they could become public API as well.
@stephentoub
Fix NetworkStream argument names
0d61950
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Add stream conformance tests for NetworkStream
b20d581
@stephentoub
Add stream conformance tests for QuicStream
80e15eb
@stephentoub
Fix several CryptoStream behaviors
a7f6964
1. Flushing a stream that wraps another stream for writing should always flush that underlying stream, even if no additional data was written as part of the flush.
2. Argument validation should validate buffers are not null rather than null ref'ing on a null buffer.
3. Checks for the CryptoStream mode should come after argument validation.
@stephentoub
Add stream conformance tests for CryptoStream
78bc0ac
@stephentoub
Fix FileStream argument names
3586739
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Fix BufferedStream argument names
4f32db2
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Add stream conformance tests for FileStream
cad5f6a
Specifically when used in a connected fashion, wrapped around an anonymous or named pipe.
@stephentoub
Add stream conformance tests for BufferedStream
ec88166
Specifically when used in a connected fashion, wrapped around some other connected stream.
@stephentoub
Add stream conformance tests for SslStream and NegotiateStream
3f6a0f7
@stephentoub
Fix PipeStream.Flush to not fail on readable streams
fb1de39
Consumers may expect Stream.Flush to be a nop if the stream is readable only, but PipeStream.Flush is throwing in that case. Stop doing that.
@stephentoub
Add stream conformance tests for PipeStream
fb55c4a
@stephentoub
Fix several BrotliStream behaviors
c6fa2f1
1. When passed a null buffer, it's throwing an exception with the argument name "array", even though the parameter's name is "buffer".
2. Even if there's no data written as part of the Flush{Async} call on a writeable stream, it should be calling flush on the wrapped stream.
@stephentoub
Fix several DeflateStream (and friends) issues
af13fb9
1. DeflateStream.Flush{Async} when writing needs to always flush the underlying stream, even if there's no data written as part of the flush call itself.
2. Several byte[] array arguments should be byte[] buffer. Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
3. DeflateStream.EndRead/Write needs to do additional state validation.
4. Not a bug, but simplify ReadAsync to match the sync Read implementation flow.
@stephentoub
Add stream conformance tests for Deflate/ZLib/GZip/BrotliStream
296b99a
@stephentoub
Fix PipeReader/WriterStream argument validation
230c853
@stephentoub
Add stream conformance tests for PipeReader/WriterStream
5e5afba
@stephentoub
Remove erroneous asserts from Stream.ReadWriteTask
6dc26b6
@stephentoub
Address PR feedback
c46cdd1
@stephentoub
Fix a few tests in CI
ac6aff5
@stephentoub
stephentoub
merged commit 0acbafe
into
dotnet:masterOct 28, 2020
@stephentoub
stephentoub
deleted the

streamconformance

branch
October 28, 2020 13:36
@runfoapprunfoappBot
mentioned this pull request
Nov 2, 2020

Closed

@ericstjericstj
mentioned this pull request
Nov 12, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 6, 2020
@gewarren
gewarren
commented
Mar 2, 2021

Copy link


Copy Markdown

Contributor
@stephentoub@PriyaPurkayastha Can you create a breaking change docs issue for this here? I believe this went out in Preview 1.
@stephentoub
stephentoub
commented
Mar 4, 2021

Copy link


Copy Markdown

MemberAuthor
@gewarren, I've opened dotnet/docs#23102. Thanks.
@stephentoub
stephentoub
commented
Sep 30, 2021

Copy link


Copy Markdown

MemberAuthor
@ericstj, you assigned this to me. What's the action item?
@ericstj
ericstj
commented
Sep 30, 2021

Copy link


Copy Markdown

Member
Mail is coming shortly. I'm going through all PRs with needs-breaking-change-doc and asking folks to file those issues and remove the label.
@stephentoub
stephentoub
commented
Sep 30, 2021

Copy link


Copy Markdown

MemberAuthor
Ok. As noted above in the comments, dotnet/docs#23102 was already opened and addressed.
@stephentoubstephentoub
removed
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Sep 30, 2021
@ericstj
ericstj
commented
Sep 30, 2021

Copy link


Copy Markdown

Member
Thanks for removing the label, that will remove this from my queries.
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@geoffkizergeoffkizergeoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stephentoubstephentoub

Labels

area-System.IObreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@stephentoub@geoffkizer@scalablecory@gewarren@ericstj@Dotnet-GitSync-Bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add initial set of conformance tests for Stream - #43834
Merged
stephentoub merged 24 commits into
dotnet:masterfrom 
stephentoub:streamconformance
Oct 28, 2020
Merged
Add initial set of conformance tests for Stream#43834
stephentoub merged 24 commits into
dotnet:masterfrom 
stephentoub:streamconformance
Conversation
@stephentoub
@stephentoubstephentoub
commented
Oct 26, 2020

Copy link


Copy Markdown

Member
This also includes fixes to a variety of streams where they're out of conformance, in particular around argument names.
The tests being added are primarily focused on "connected streams", ones where two streams are used to represent two ends of a communication channel. We can subsequently flesh this out for other kinds of streams. Also, while I consolidated a bunch of tests that were spread across different stream-derived type tests, there's still more that can be consolidated over time into these centralized tests.
cc: @geoffkizer, @ericstj, @terrajobst
@Dotnet-GitSync-BotDotnet-GitSync-Bot
added
the 
area-Infrastructure-coreclr
Only use for closed issues
label
Oct 26, 2020
@stephentoubstephentoub
added

area-System.IO

breaking-change
Issue or PR that represents a breaking API or functional change over a previous release.
and removed

area-Infrastructure-coreclr
Only use for closed issues
labels
Oct 26, 2020
@ghostghost
added
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Oct 26, 2020
@stephentoubstephentoub
added this to the 6.0.0 milestone
Oct 26, 2020
@stephentoub
stephentoubforce-pushed
the
streamconformance
branch
3 times, most recently
from
56ecc67 to
7f45cf0CompareOctober 27, 2020 00:42
@runfoapprunfoappBot
mentioned this pull request
Oct 27, 2020

Closed

geoffkizer
geoffkizer
approved these changes
Oct 27, 2020
View reviewed changes
@geoffkizergeoffkizer
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

In general this looks really good. A few minor comments below.
Are you planning additional work here, or is this basically done?
Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment thread...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/NetworkStreamTest.cs
Outdated

@geoffkizer
geoffkizer
commented
Oct 27, 2020

Copy link


Copy Markdown

Contributor
I think we should also consider using the WrappedConnectedStreamTests for HttpClient request/response streams. This is a little weird because we don't actually have a Stream implementation on the server side -- but we could hack this together on top of the various loopback server stuff fairly easily. Thoughts?
@stephentoub
stephentoub
commented
Oct 27, 2020

Copy link


Copy Markdown

MemberAuthor
I think we should also consider using the WrappedConnectedStreamTests for HttpClient request/response streams. This is a little weird because we don't actually have a Stream implementation on the server side -- but we could hack this together on top of the various loopback server stuff fairly easily. Thoughts?
There are more streams in dotnet/runtime than I've included here. My thinking was we'd get this framework in, and then folks can add additional streams to test and additional tests over time. I got many of the heavy hitters here.
@stephentoub
stephentoubforce-pushed
the
streamconformance
branch
2 times, most recently
from
7108d31 to
d139d8aCompareOctober 28, 2020 02:05
@geoffkizer
geoffkizer
commented
Oct 28, 2020

Copy link


Copy Markdown

Contributor
@scalablecory Can we go ahead and fix the Dispose behavior in QuicStream? Any reason to hold off on this? There are other issues in #756 that may need additional consideration, but it would be nice to fix the Dispose issues and make these conformance tests work for QuicStream.
@scalablecory
scalablecory
commented
Oct 28, 2020

Copy link


Copy Markdown

Contributor
@scalablecory Can we go ahead and fix the Dispose behavior in QuicStream? Any reason to hold off on this? There are other issues in #756 that may need additional consideration, but it would be nice to fix the Dispose issues and make these conformance tests work for QuicStream.
No reason to hold off. It has been a thorn in my side for HTTP/3 so I would very much like to fix it :).
@stephentoub
stephentoub
commented
Oct 28, 2020

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
stephentoub
stephentoub
commented
Oct 28, 2020
View reviewed changes
Comment thread...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs
Outdated

@stephentoubstephentoubOct 28, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@geoffkizer, fyi, my machine isn't set up to run these, and they don't appear to run in CI yet (correct me if I'm wrong). So you may need to tweak them once they're merged.
@stephentoub
Fix invalid assert in StreamBuffer
ef00c0d
This can be hit by calling code and thus shouldn't be an assert. As an assert it prevents testing.
@stephentoub
Add ConnectedStreams.CreateUnidirectional
a590bcf
@stephentoub
Add initial set of Stream conformance tests
f63c227
These primarily focus on "connected streams", ones that can be arranged to communicate with each other in a producer/consumer pattern or as a bidirectional communication mechanism, e.g. NetworkStream, PipeStream, SslStream wrapped around a NetworkStream, FileStream created around pipes, etc. Later we can add more tests focused on standalone streams, e.g. FileStream created for an on-disk file, MemoryStream, etc.
@stephentoub
Add ConnectedStreams tests
59238b7
These are currently helpers used by many other tests. At some point they could become public API as well.
@stephentoub
Fix NetworkStream argument names
0d61950
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Add stream conformance tests for NetworkStream
b20d581
@stephentoub
Add stream conformance tests for QuicStream
80e15eb
@stephentoub
Fix several CryptoStream behaviors
a7f6964
1. Flushing a stream that wraps another stream for writing should always flush that underlying stream, even if no additional data was written as part of the flush.
2. Argument validation should validate buffers are not null rather than null ref'ing on a null buffer.
3. Checks for the CryptoStream mode should come after argument validation.
@stephentoub
Add stream conformance tests for CryptoStream
78bc0ac
@stephentoub
Fix FileStream argument names
3586739
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Fix BufferedStream argument names
4f32db2
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Add stream conformance tests for FileStream
cad5f6a
Specifically when used in a connected fashion, wrapped around an anonymous or named pipe.
@stephentoub
Add stream conformance tests for BufferedStream
ec88166
Specifically when used in a connected fashion, wrapped around some other connected stream.
@stephentoub
Add stream conformance tests for SslStream and NegotiateStream
3f6a0f7
@stephentoub
Fix PipeStream.Flush to not fail on readable streams
fb1de39
Consumers may expect Stream.Flush to be a nop if the stream is readable only, but PipeStream.Flush is throwing in that case. Stop doing that.
@stephentoub
Add stream conformance tests for PipeStream
fb55c4a
@stephentoub
Fix several BrotliStream behaviors
c6fa2f1
1. When passed a null buffer, it's throwing an exception with the argument name "array", even though the parameter's name is "buffer".
2. Even if there's no data written as part of the Flush{Async} call on a writeable stream, it should be calling flush on the wrapped stream.
@stephentoub
Fix several DeflateStream (and friends) issues
af13fb9
1. DeflateStream.Flush{Async} when writing needs to always flush the underlying stream, even if there's no data written as part of the flush call itself.
2. Several byte[] array arguments should be byte[] buffer. Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
3. DeflateStream.EndRead/Write needs to do additional state validation.
4. Not a bug, but simplify ReadAsync to match the sync Read implementation flow.
@stephentoub
Add stream conformance tests for Deflate/ZLib/GZip/BrotliStream
296b99a
@stephentoub
Fix PipeReader/WriterStream argument validation
230c853
@stephentoub
Add stream conformance tests for PipeReader/WriterStream
5e5afba
@stephentoub
Remove erroneous asserts from Stream.ReadWriteTask
6dc26b6
@stephentoub
Address PR feedback
c46cdd1
@stephentoub
Fix a few tests in CI
ac6aff5
@stephentoub
stephentoub
merged commit 0acbafe
into
dotnet:masterOct 28, 2020
@stephentoub
stephentoub
deleted the

streamconformance

branch
October 28, 2020 13:36
@runfoapprunfoappBot
mentioned this pull request
Nov 2, 2020

Closed

@ericstjericstj
mentioned this pull request
Nov 12, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 6, 2020
@gewarren
gewarren
commented
Mar 2, 2021

Copy link


Copy Markdown

Contributor
@stephentoub@PriyaPurkayastha Can you create a breaking change docs issue for this here? I believe this went out in Preview 1.
@stephentoub
stephentoub
commented
Mar 4, 2021

Copy link


Copy Markdown

MemberAuthor
@gewarren, I've opened dotnet/docs#23102. Thanks.
@stephentoub
stephentoub
commented
Sep 30, 2021

Copy link


Copy Markdown

MemberAuthor
@ericstj, you assigned this to me. What's the action item?
@ericstj
ericstj
commented
Sep 30, 2021

Copy link


Copy Markdown

Member
Mail is coming shortly. I'm going through all PRs with needs-breaking-change-doc and asking folks to file those issues and remove the label.
@stephentoub
stephentoub
commented
Sep 30, 2021

Copy link


Copy Markdown

MemberAuthor
Ok. As noted above in the comments, dotnet/docs#23102 was already opened and addressed.
@stephentoubstephentoub
removed
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Sep 30, 2021
@ericstj
ericstj
commented
Sep 30, 2021

Copy link


Copy Markdown

Member
Thanks for removing the label, that will remove this from my queries.
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@geoffkizergeoffkizergeoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stephentoubstephentoub

Labels

area-System.IObreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@stephentoub@geoffkizer@scalablecory@gewarren@ericstj@Dotnet-GitSync-Bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add initial set of conformance tests for Stream - #43834
Merged
stephentoub merged 24 commits into
dotnet:masterfrom 
stephentoub:streamconformance
Oct 28, 2020
Merged
Add initial set of conformance tests for Stream#43834
stephentoub merged 24 commits into
dotnet:masterfrom 
stephentoub:streamconformance
Conversation
@stephentoub
@stephentoubstephentoub
commented
Oct 26, 2020

Copy link


Copy Markdown

Member
This also includes fixes to a variety of streams where they're out of conformance, in particular around argument names.
The tests being added are primarily focused on "connected streams", ones where two streams are used to represent two ends of a communication channel. We can subsequently flesh this out for other kinds of streams. Also, while I consolidated a bunch of tests that were spread across different stream-derived type tests, there's still more that can be consolidated over time into these centralized tests.
cc: @geoffkizer, @ericstj, @terrajobst
@Dotnet-GitSync-BotDotnet-GitSync-Bot
added
the 
area-Infrastructure-coreclr
Only use for closed issues
label
Oct 26, 2020
@stephentoubstephentoub
added

area-System.IO

breaking-change
Issue or PR that represents a breaking API or functional change over a previous release.
and removed

area-Infrastructure-coreclr
Only use for closed issues
labels
Oct 26, 2020
@ghostghost
added
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Oct 26, 2020
@stephentoubstephentoub
added this to the 6.0.0 milestone
Oct 26, 2020
@stephentoub
stephentoubforce-pushed
the
streamconformance
branch
3 times, most recently
from
56ecc67 to
7f45cf0CompareOctober 27, 2020 00:42
@runfoapprunfoappBot
mentioned this pull request
Oct 27, 2020

Closed

geoffkizer
geoffkizer
approved these changes
Oct 27, 2020
View reviewed changes
@geoffkizergeoffkizer
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

In general this looks really good. A few minor comments below.
Are you planning additional work here, or is this basically done?
Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment thread...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/NetworkStreamTest.cs
Outdated

@geoffkizer
geoffkizer
commented
Oct 27, 2020

Copy link


Copy Markdown

Contributor
I think we should also consider using the WrappedConnectedStreamTests for HttpClient request/response streams. This is a little weird because we don't actually have a Stream implementation on the server side -- but we could hack this together on top of the various loopback server stuff fairly easily. Thoughts?
@stephentoub
stephentoub
commented
Oct 27, 2020

Copy link


Copy Markdown

MemberAuthor
I think we should also consider using the WrappedConnectedStreamTests for HttpClient request/response streams. This is a little weird because we don't actually have a Stream implementation on the server side -- but we could hack this together on top of the various loopback server stuff fairly easily. Thoughts?
There are more streams in dotnet/runtime than I've included here. My thinking was we'd get this framework in, and then folks can add additional streams to test and additional tests over time. I got many of the heavy hitters here.
@stephentoub
stephentoubforce-pushed
the
streamconformance
branch
2 times, most recently
from
7108d31 to
d139d8aCompareOctober 28, 2020 02:05
@geoffkizer
geoffkizer
commented
Oct 28, 2020

Copy link


Copy Markdown

Contributor
@scalablecory Can we go ahead and fix the Dispose behavior in QuicStream? Any reason to hold off on this? There are other issues in #756 that may need additional consideration, but it would be nice to fix the Dispose issues and make these conformance tests work for QuicStream.
@scalablecory
scalablecory
commented
Oct 28, 2020

Copy link


Copy Markdown

Contributor
@scalablecory Can we go ahead and fix the Dispose behavior in QuicStream? Any reason to hold off on this? There are other issues in #756 that may need additional consideration, but it would be nice to fix the Dispose issues and make these conformance tests work for QuicStream.
No reason to hold off. It has been a thorn in my side for HTTP/3 so I would very much like to fix it :).
@stephentoub
stephentoub
commented
Oct 28, 2020

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
stephentoub
stephentoub
commented
Oct 28, 2020
View reviewed changes
Comment thread...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs
Outdated

@stephentoubstephentoubOct 28, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@geoffkizer, fyi, my machine isn't set up to run these, and they don't appear to run in CI yet (correct me if I'm wrong). So you may need to tweak them once they're merged.
@stephentoub
Fix invalid assert in StreamBuffer
ef00c0d
This can be hit by calling code and thus shouldn't be an assert. As an assert it prevents testing.
@stephentoub
Add ConnectedStreams.CreateUnidirectional
a590bcf
@stephentoub
Add initial set of Stream conformance tests
f63c227
These primarily focus on "connected streams", ones that can be arranged to communicate with each other in a producer/consumer pattern or as a bidirectional communication mechanism, e.g. NetworkStream, PipeStream, SslStream wrapped around a NetworkStream, FileStream created around pipes, etc. Later we can add more tests focused on standalone streams, e.g. FileStream created for an on-disk file, MemoryStream, etc.
@stephentoub
Add ConnectedStreams tests
59238b7
These are currently helpers used by many other tests. At some point they could become public API as well.
@stephentoub
Fix NetworkStream argument names
0d61950
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Add stream conformance tests for NetworkStream
b20d581
@stephentoub
Add stream conformance tests for QuicStream
80e15eb
@stephentoub
Fix several CryptoStream behaviors
a7f6964
1. Flushing a stream that wraps another stream for writing should always flush that underlying stream, even if no additional data was written as part of the flush.
2. Argument validation should validate buffers are not null rather than null ref'ing on a null buffer.
3. Checks for the CryptoStream mode should come after argument validation.
@stephentoub
Add stream conformance tests for CryptoStream
78bc0ac
@stephentoub
Fix FileStream argument names
3586739
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Fix BufferedStream argument names
4f32db2
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Add stream conformance tests for FileStream
cad5f6a
Specifically when used in a connected fashion, wrapped around an anonymous or named pipe.
@stephentoub
Add stream conformance tests for BufferedStream
ec88166
Specifically when used in a connected fashion, wrapped around some other connected stream.
@stephentoub
Add stream conformance tests for SslStream and NegotiateStream
3f6a0f7
@stephentoub
Fix PipeStream.Flush to not fail on readable streams
fb1de39
Consumers may expect Stream.Flush to be a nop if the stream is readable only, but PipeStream.Flush is throwing in that case. Stop doing that.
@stephentoub
Add stream conformance tests for PipeStream
fb55c4a
@stephentoub
Fix several BrotliStream behaviors
c6fa2f1
1. When passed a null buffer, it's throwing an exception with the argument name "array", even though the parameter's name is "buffer".
2. Even if there's no data written as part of the Flush{Async} call on a writeable stream, it should be calling flush on the wrapped stream.
@stephentoub
Fix several DeflateStream (and friends) issues
af13fb9
1. DeflateStream.Flush{Async} when writing needs to always flush the underlying stream, even if there's no data written as part of the flush call itself.
2. Several byte[] array arguments should be byte[] buffer. Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
3. DeflateStream.EndRead/Write needs to do additional state validation.
4. Not a bug, but simplify ReadAsync to match the sync Read implementation flow.
@stephentoub
Add stream conformance tests for Deflate/ZLib/GZip/BrotliStream
296b99a
@stephentoub
Fix PipeReader/WriterStream argument validation
230c853
@stephentoub
Add stream conformance tests for PipeReader/WriterStream
5e5afba
@stephentoub
Remove erroneous asserts from Stream.ReadWriteTask
6dc26b6
@stephentoub
Address PR feedback
c46cdd1
@stephentoub
Fix a few tests in CI
ac6aff5
@stephentoub
stephentoub
merged commit 0acbafe
into
dotnet:masterOct 28, 2020
@stephentoub
stephentoub
deleted the

streamconformance

branch
October 28, 2020 13:36
@runfoapprunfoappBot
mentioned this pull request
Nov 2, 2020

Closed

@ericstjericstj
mentioned this pull request
Nov 12, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 6, 2020
@gewarren
gewarren
commented
Mar 2, 2021

Copy link


Copy Markdown

Contributor
@stephentoub@PriyaPurkayastha Can you create a breaking change docs issue for this here? I believe this went out in Preview 1.
@stephentoub
stephentoub
commented
Mar 4, 2021

Copy link


Copy Markdown

MemberAuthor
@gewarren, I've opened dotnet/docs#23102. Thanks.
@stephentoub
stephentoub
commented
Sep 30, 2021

Copy link


Copy Markdown

MemberAuthor
@ericstj, you assigned this to me. What's the action item?
@ericstj
ericstj
commented
Sep 30, 2021

Copy link


Copy Markdown

Member
Mail is coming shortly. I'm going through all PRs with needs-breaking-change-doc and asking folks to file those issues and remove the label.
@stephentoub
stephentoub
commented
Sep 30, 2021

Copy link


Copy Markdown

MemberAuthor
Ok. As noted above in the comments, dotnet/docs#23102 was already opened and addressed.
@stephentoubstephentoub
removed
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Sep 30, 2021
@ericstj
ericstj
commented
Sep 30, 2021

Copy link


Copy Markdown

Member
Thanks for removing the label, that will remove this from my queries.
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@geoffkizergeoffkizergeoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stephentoubstephentoub

Labels

area-System.IObreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@stephentoub@geoffkizer@scalablecory@gewarren@ericstj@Dotnet-GitSync-Bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Add initial set of conformance tests for Stream - #43834
Merged
stephentoub merged 24 commits into
dotnet:masterfrom 
stephentoub:streamconformance
Oct 28, 2020
Merged
Add initial set of conformance tests for Stream#43834
stephentoub merged 24 commits into
dotnet:masterfrom 
stephentoub:streamconformance
Conversation
@stephentoub
@stephentoubstephentoub
commented
Oct 26, 2020

Copy link


Copy Markdown

Member
This also includes fixes to a variety of streams where they're out of conformance, in particular around argument names.
The tests being added are primarily focused on "connected streams", ones where two streams are used to represent two ends of a communication channel. We can subsequently flesh this out for other kinds of streams. Also, while I consolidated a bunch of tests that were spread across different stream-derived type tests, there's still more that can be consolidated over time into these centralized tests.
cc: @geoffkizer, @ericstj, @terrajobst
@Dotnet-GitSync-BotDotnet-GitSync-Bot
added
the 
area-Infrastructure-coreclr
Only use for closed issues
label
Oct 26, 2020
@stephentoubstephentoub
added

area-System.IO

breaking-change
Issue or PR that represents a breaking API or functional change over a previous release.
and removed

area-Infrastructure-coreclr
Only use for closed issues
labels
Oct 26, 2020
@ghostghost
added
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Oct 26, 2020
@stephentoubstephentoub
added this to the 6.0.0 milestone
Oct 26, 2020
@stephentoub
stephentoubforce-pushed
the
streamconformance
branch
3 times, most recently
from
56ecc67 to
7f45cf0CompareOctober 27, 2020 00:42
@runfoapprunfoappBot
mentioned this pull request
Oct 27, 2020

Closed

geoffkizer
geoffkizer
approved these changes
Oct 27, 2020
View reviewed changes
@geoffkizergeoffkizer
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

In general this looks really good. A few minor comments below.
Are you planning additional work here, or is this basically done?
Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment thread...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/NetworkStreamTest.cs
Outdated

@geoffkizer
geoffkizer
commented
Oct 27, 2020

Copy link


Copy Markdown

Contributor
I think we should also consider using the WrappedConnectedStreamTests for HttpClient request/response streams. This is a little weird because we don't actually have a Stream implementation on the server side -- but we could hack this together on top of the various loopback server stuff fairly easily. Thoughts?
@stephentoub
stephentoub
commented
Oct 27, 2020

Copy link


Copy Markdown

MemberAuthor
I think we should also consider using the WrappedConnectedStreamTests for HttpClient request/response streams. This is a little weird because we don't actually have a Stream implementation on the server side -- but we could hack this together on top of the various loopback server stuff fairly easily. Thoughts?
There are more streams in dotnet/runtime than I've included here. My thinking was we'd get this framework in, and then folks can add additional streams to test and additional tests over time. I got many of the heavy hitters here.
@stephentoub
stephentoubforce-pushed
the
streamconformance
branch
2 times, most recently
from
7108d31 to
d139d8aCompareOctober 28, 2020 02:05
@geoffkizer
geoffkizer
commented
Oct 28, 2020

Copy link


Copy Markdown

Contributor
@scalablecory Can we go ahead and fix the Dispose behavior in QuicStream? Any reason to hold off on this? There are other issues in #756 that may need additional consideration, but it would be nice to fix the Dispose issues and make these conformance tests work for QuicStream.
@scalablecory
scalablecory
commented
Oct 28, 2020

Copy link


Copy Markdown

Contributor
@scalablecory Can we go ahead and fix the Dispose behavior in QuicStream? Any reason to hold off on this? There are other issues in #756 that may need additional consideration, but it would be nice to fix the Dispose issues and make these conformance tests work for QuicStream.
No reason to hold off. It has been a thorn in my side for HTTP/3 so I would very much like to fix it :).
@stephentoub
stephentoub
commented
Oct 28, 2020

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
stephentoub
stephentoub
commented
Oct 28, 2020
View reviewed changes
Comment thread...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs
Outdated

@stephentoubstephentoubOct 28, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@geoffkizer, fyi, my machine isn't set up to run these, and they don't appear to run in CI yet (correct me if I'm wrong). So you may need to tweak them once they're merged.
@stephentoub
Fix invalid assert in StreamBuffer
ef00c0d
This can be hit by calling code and thus shouldn't be an assert. As an assert it prevents testing.
@stephentoub
Add ConnectedStreams.CreateUnidirectional
a590bcf
@stephentoub
Add initial set of Stream conformance tests
f63c227
These primarily focus on "connected streams", ones that can be arranged to communicate with each other in a producer/consumer pattern or as a bidirectional communication mechanism, e.g. NetworkStream, PipeStream, SslStream wrapped around a NetworkStream, FileStream created around pipes, etc. Later we can add more tests focused on standalone streams, e.g. FileStream created for an on-disk file, MemoryStream, etc.
@stephentoub
Add ConnectedStreams tests
59238b7
These are currently helpers used by many other tests. At some point they could become public API as well.
@stephentoub
Fix NetworkStream argument names
0d61950
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Add stream conformance tests for NetworkStream
b20d581
@stephentoub
Add stream conformance tests for QuicStream
80e15eb
@stephentoub
Fix several CryptoStream behaviors
a7f6964
1. Flushing a stream that wraps another stream for writing should always flush that underlying stream, even if no additional data was written as part of the flush.
2. Argument validation should validate buffers are not null rather than null ref'ing on a null buffer.
3. Checks for the CryptoStream mode should come after argument validation.
@stephentoub
Add stream conformance tests for CryptoStream
78bc0ac
@stephentoub
Fix FileStream argument names
3586739
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Fix BufferedStream argument names
4f32db2
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Add stream conformance tests for FileStream
cad5f6a
Specifically when used in a connected fashion, wrapped around an anonymous or named pipe.
@stephentoub
Add stream conformance tests for BufferedStream
ec88166
Specifically when used in a connected fashion, wrapped around some other connected stream.
@stephentoub
Add stream conformance tests for SslStream and NegotiateStream
3f6a0f7
@stephentoub
Fix PipeStream.Flush to not fail on readable streams
fb1de39
Consumers may expect Stream.Flush to be a nop if the stream is readable only, but PipeStream.Flush is throwing in that case. Stop doing that.
@stephentoub
Add stream conformance tests for PipeStream
fb55c4a
@stephentoub
Fix several BrotliStream behaviors
c6fa2f1
1. When passed a null buffer, it's throwing an exception with the argument name "array", even though the parameter's name is "buffer".
2. Even if there's no data written as part of the Flush{Async} call on a writeable stream, it should be calling flush on the wrapped stream.
@stephentoub
Fix several DeflateStream (and friends) issues
af13fb9
1. DeflateStream.Flush{Async} when writing needs to always flush the underlying stream, even if there's no data written as part of the flush call itself.
2. Several byte[] array arguments should be byte[] buffer. Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
3. DeflateStream.EndRead/Write needs to do additional state validation.
4. Not a bug, but simplify ReadAsync to match the sync Read implementation flow.
@stephentoub
Add stream conformance tests for Deflate/ZLib/GZip/BrotliStream
296b99a
@stephentoub
Fix PipeReader/WriterStream argument validation
230c853
@stephentoub
Add stream conformance tests for PipeReader/WriterStream
5e5afba
@stephentoub
Remove erroneous asserts from Stream.ReadWriteTask
6dc26b6
@stephentoub
Address PR feedback
c46cdd1
@stephentoub
Fix a few tests in CI
ac6aff5
@stephentoub
stephentoub
merged commit 0acbafe
into
dotnet:masterOct 28, 2020
@stephentoub
stephentoub
deleted the

streamconformance

branch
October 28, 2020 13:36
@runfoapprunfoappBot
mentioned this pull request
Nov 2, 2020

Closed

@ericstjericstj
mentioned this pull request
Nov 12, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 6, 2020
@gewarren
gewarren
commented
Mar 2, 2021

Copy link


Copy Markdown

Contributor
@stephentoub@PriyaPurkayastha Can you create a breaking change docs issue for this here? I believe this went out in Preview 1.
@stephentoub
stephentoub
commented
Mar 4, 2021

Copy link


Copy Markdown

MemberAuthor
@gewarren, I've opened dotnet/docs#23102. Thanks.
@stephentoub
stephentoub
commented
Sep 30, 2021

Copy link


Copy Markdown

MemberAuthor
@ericstj, you assigned this to me. What's the action item?
@ericstj
ericstj
commented
Sep 30, 2021

Copy link


Copy Markdown

Member
Mail is coming shortly. I'm going through all PRs with needs-breaking-change-doc and asking folks to file those issues and remove the label.
@stephentoub
stephentoub
commented
Sep 30, 2021

Copy link


Copy Markdown

MemberAuthor
Ok. As noted above in the comments, dotnet/docs#23102 was already opened and addressed.
@stephentoubstephentoub
removed
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Sep 30, 2021
@ericstj
ericstj
commented
Sep 30, 2021

Copy link


Copy Markdown

Member
Thanks for removing the label, that will remove this from my queries.
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@geoffkizergeoffkizergeoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stephentoubstephentoub

Labels

area-System.IObreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@stephentoub@geoffkizer@scalablecory@gewarren@ericstj@Dotnet-GitSync-Bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add initial set of conformance tests for Stream - #43834
Merged
stephentoub merged 24 commits into
dotnet:masterfrom 
stephentoub:streamconformance
Oct 28, 2020
Merged
Add initial set of conformance tests for Stream#43834
stephentoub merged 24 commits into
dotnet:masterfrom 
stephentoub:streamconformance
Conversation
@stephentoub
@stephentoubstephentoub
commented
Oct 26, 2020

Copy link


Copy Markdown

Member
This also includes fixes to a variety of streams where they're out of conformance, in particular around argument names.
The tests being added are primarily focused on "connected streams", ones where two streams are used to represent two ends of a communication channel. We can subsequently flesh this out for other kinds of streams. Also, while I consolidated a bunch of tests that were spread across different stream-derived type tests, there's still more that can be consolidated over time into these centralized tests.
cc: @geoffkizer, @ericstj, @terrajobst
@Dotnet-GitSync-BotDotnet-GitSync-Bot
added
the 
area-Infrastructure-coreclr
Only use for closed issues
label
Oct 26, 2020
@stephentoubstephentoub
added

area-System.IO

breaking-change
Issue or PR that represents a breaking API or functional change over a previous release.
and removed

area-Infrastructure-coreclr
Only use for closed issues
labels
Oct 26, 2020
@ghostghost
added
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Oct 26, 2020
@stephentoubstephentoub
added this to the 6.0.0 milestone
Oct 26, 2020
@stephentoub
stephentoubforce-pushed
the
streamconformance
branch
3 times, most recently
from
56ecc67 to
7f45cf0CompareOctober 27, 2020 00:42
@runfoapprunfoappBot
mentioned this pull request
Oct 27, 2020

Closed

geoffkizer
geoffkizer
approved these changes
Oct 27, 2020
View reviewed changes
@geoffkizergeoffkizer
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

In general this looks really good. A few minor comments below.
Are you planning additional work here, or is this basically done?
Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment thread...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/NetworkStreamTest.cs
Outdated

@geoffkizer
geoffkizer
commented
Oct 27, 2020

Copy link


Copy Markdown

Contributor
I think we should also consider using the WrappedConnectedStreamTests for HttpClient request/response streams. This is a little weird because we don't actually have a Stream implementation on the server side -- but we could hack this together on top of the various loopback server stuff fairly easily. Thoughts?
@stephentoub
stephentoub
commented
Oct 27, 2020

Copy link


Copy Markdown

MemberAuthor
I think we should also consider using the WrappedConnectedStreamTests for HttpClient request/response streams. This is a little weird because we don't actually have a Stream implementation on the server side -- but we could hack this together on top of the various loopback server stuff fairly easily. Thoughts?
There are more streams in dotnet/runtime than I've included here. My thinking was we'd get this framework in, and then folks can add additional streams to test and additional tests over time. I got many of the heavy hitters here.
@stephentoub
stephentoubforce-pushed
the
streamconformance
branch
2 times, most recently
from
7108d31 to
d139d8aCompareOctober 28, 2020 02:05
@geoffkizer
geoffkizer
commented
Oct 28, 2020

Copy link


Copy Markdown

Contributor
@scalablecory Can we go ahead and fix the Dispose behavior in QuicStream? Any reason to hold off on this? There are other issues in #756 that may need additional consideration, but it would be nice to fix the Dispose issues and make these conformance tests work for QuicStream.
@scalablecory
scalablecory
commented
Oct 28, 2020

Copy link


Copy Markdown

Contributor
@scalablecory Can we go ahead and fix the Dispose behavior in QuicStream? Any reason to hold off on this? There are other issues in #756 that may need additional consideration, but it would be nice to fix the Dispose issues and make these conformance tests work for QuicStream.
No reason to hold off. It has been a thorn in my side for HTTP/3 so I would very much like to fix it :).
@stephentoub
stephentoub
commented
Oct 28, 2020

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
stephentoub
stephentoub
commented
Oct 28, 2020
View reviewed changes
Comment thread...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs
Outdated

@stephentoubstephentoubOct 28, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@geoffkizer, fyi, my machine isn't set up to run these, and they don't appear to run in CI yet (correct me if I'm wrong). So you may need to tweak them once they're merged.
@stephentoub
Fix invalid assert in StreamBuffer
ef00c0d
This can be hit by calling code and thus shouldn't be an assert. As an assert it prevents testing.
@stephentoub
Add ConnectedStreams.CreateUnidirectional
a590bcf
@stephentoub
Add initial set of Stream conformance tests
f63c227
These primarily focus on "connected streams", ones that can be arranged to communicate with each other in a producer/consumer pattern or as a bidirectional communication mechanism, e.g. NetworkStream, PipeStream, SslStream wrapped around a NetworkStream, FileStream created around pipes, etc. Later we can add more tests focused on standalone streams, e.g. FileStream created for an on-disk file, MemoryStream, etc.
@stephentoub
Add ConnectedStreams tests
59238b7
These are currently helpers used by many other tests. At some point they could become public API as well.
@stephentoub
Fix NetworkStream argument names
0d61950
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Add stream conformance tests for NetworkStream
b20d581
@stephentoub
Add stream conformance tests for QuicStream
80e15eb
@stephentoub
Fix several CryptoStream behaviors
a7f6964
1. Flushing a stream that wraps another stream for writing should always flush that underlying stream, even if no additional data was written as part of the flush.
2. Argument validation should validate buffers are not null rather than null ref'ing on a null buffer.
3. Checks for the CryptoStream mode should come after argument validation.
@stephentoub
Add stream conformance tests for CryptoStream
78bc0ac
@stephentoub
Fix FileStream argument names
3586739
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Fix BufferedStream argument names
4f32db2
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Add stream conformance tests for FileStream
cad5f6a
Specifically when used in a connected fashion, wrapped around an anonymous or named pipe.
@stephentoub
Add stream conformance tests for BufferedStream
ec88166
Specifically when used in a connected fashion, wrapped around some other connected stream.
@stephentoub
Add stream conformance tests for SslStream and NegotiateStream
3f6a0f7
@stephentoub
Fix PipeStream.Flush to not fail on readable streams
fb1de39
Consumers may expect Stream.Flush to be a nop if the stream is readable only, but PipeStream.Flush is throwing in that case. Stop doing that.
@stephentoub
Add stream conformance tests for PipeStream
fb55c4a
@stephentoub
Fix several BrotliStream behaviors
c6fa2f1
1. When passed a null buffer, it's throwing an exception with the argument name "array", even though the parameter's name is "buffer".
2. Even if there's no data written as part of the Flush{Async} call on a writeable stream, it should be calling flush on the wrapped stream.
@stephentoub
Fix several DeflateStream (and friends) issues
af13fb9
1. DeflateStream.Flush{Async} when writing needs to always flush the underlying stream, even if there's no data written as part of the flush call itself.
2. Several byte[] array arguments should be byte[] buffer. Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
3. DeflateStream.EndRead/Write needs to do additional state validation.
4. Not a bug, but simplify ReadAsync to match the sync Read implementation flow.
@stephentoub
Add stream conformance tests for Deflate/ZLib/GZip/BrotliStream
296b99a
@stephentoub
Fix PipeReader/WriterStream argument validation
230c853
@stephentoub
Add stream conformance tests for PipeReader/WriterStream
5e5afba
@stephentoub
Remove erroneous asserts from Stream.ReadWriteTask
6dc26b6
@stephentoub
Address PR feedback
c46cdd1
@stephentoub
Fix a few tests in CI
ac6aff5
@stephentoub
stephentoub
merged commit 0acbafe
into
dotnet:masterOct 28, 2020
@stephentoub
stephentoub
deleted the

streamconformance

branch
October 28, 2020 13:36
@runfoapprunfoappBot
mentioned this pull request
Nov 2, 2020

Closed

@ericstjericstj
mentioned this pull request
Nov 12, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 6, 2020
@gewarren
gewarren
commented
Mar 2, 2021

Copy link


Copy Markdown

Contributor
@stephentoub@PriyaPurkayastha Can you create a breaking change docs issue for this here? I believe this went out in Preview 1.
@stephentoub
stephentoub
commented
Mar 4, 2021

Copy link


Copy Markdown

MemberAuthor
@gewarren, I've opened dotnet/docs#23102. Thanks.
@stephentoub
stephentoub
commented
Sep 30, 2021

Copy link


Copy Markdown

MemberAuthor
@ericstj, you assigned this to me. What's the action item?
@ericstj
ericstj
commented
Sep 30, 2021

Copy link


Copy Markdown

Member
Mail is coming shortly. I'm going through all PRs with needs-breaking-change-doc and asking folks to file those issues and remove the label.
@stephentoub
stephentoub
commented
Sep 30, 2021

Copy link


Copy Markdown

MemberAuthor
Ok. As noted above in the comments, dotnet/docs#23102 was already opened and addressed.
@stephentoubstephentoub
removed
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Sep 30, 2021
@ericstj
ericstj
commented
Sep 30, 2021

Copy link


Copy Markdown

Member
Thanks for removing the label, that will remove this from my queries.
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@geoffkizergeoffkizergeoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stephentoubstephentoub

Labels

area-System.IObreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@stephentoub@geoffkizer@scalablecory@gewarren@ericstj@Dotnet-GitSync-Bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add initial set of conformance tests for Stream - #43834
Merged
stephentoub merged 24 commits into
dotnet:masterfrom 
stephentoub:streamconformance
Oct 28, 2020
Merged
Add initial set of conformance tests for Stream#43834
stephentoub merged 24 commits into
dotnet:masterfrom 
stephentoub:streamconformance
Conversation
@stephentoub
@stephentoubstephentoub
commented
Oct 26, 2020

Copy link


Copy Markdown

Member
This also includes fixes to a variety of streams where they're out of conformance, in particular around argument names.
The tests being added are primarily focused on "connected streams", ones where two streams are used to represent two ends of a communication channel. We can subsequently flesh this out for other kinds of streams. Also, while I consolidated a bunch of tests that were spread across different stream-derived type tests, there's still more that can be consolidated over time into these centralized tests.
cc: @geoffkizer, @ericstj, @terrajobst
@Dotnet-GitSync-BotDotnet-GitSync-Bot
added
the 
area-Infrastructure-coreclr
Only use for closed issues
label
Oct 26, 2020
@stephentoubstephentoub
added

area-System.IO

breaking-change
Issue or PR that represents a breaking API or functional change over a previous release.
and removed

area-Infrastructure-coreclr
Only use for closed issues
labels
Oct 26, 2020
@ghostghost
added
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Oct 26, 2020
@stephentoubstephentoub
added this to the 6.0.0 milestone
Oct 26, 2020
@stephentoub
stephentoubforce-pushed
the
streamconformance
branch
3 times, most recently
from
56ecc67 to
7f45cf0CompareOctober 27, 2020 00:42
@runfoapprunfoappBot
mentioned this pull request
Oct 27, 2020

Closed

geoffkizer
geoffkizer
approved these changes
Oct 27, 2020
View reviewed changes
@geoffkizergeoffkizer
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

In general this looks really good. A few minor comments below.
Are you planning additional work here, or is this basically done?
Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment thread...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/NetworkStreamTest.cs
Outdated

@geoffkizer
geoffkizer
commented
Oct 27, 2020

Copy link


Copy Markdown

Contributor
I think we should also consider using the WrappedConnectedStreamTests for HttpClient request/response streams. This is a little weird because we don't actually have a Stream implementation on the server side -- but we could hack this together on top of the various loopback server stuff fairly easily. Thoughts?
@stephentoub
stephentoub
commented
Oct 27, 2020

Copy link


Copy Markdown

MemberAuthor
I think we should also consider using the WrappedConnectedStreamTests for HttpClient request/response streams. This is a little weird because we don't actually have a Stream implementation on the server side -- but we could hack this together on top of the various loopback server stuff fairly easily. Thoughts?
There are more streams in dotnet/runtime than I've included here. My thinking was we'd get this framework in, and then folks can add additional streams to test and additional tests over time. I got many of the heavy hitters here.
@stephentoub
stephentoubforce-pushed
the
streamconformance
branch
2 times, most recently
from
7108d31 to
d139d8aCompareOctober 28, 2020 02:05
@geoffkizer
geoffkizer
commented
Oct 28, 2020

Copy link


Copy Markdown

Contributor
@scalablecory Can we go ahead and fix the Dispose behavior in QuicStream? Any reason to hold off on this? There are other issues in #756 that may need additional consideration, but it would be nice to fix the Dispose issues and make these conformance tests work for QuicStream.
@scalablecory
scalablecory
commented
Oct 28, 2020

Copy link


Copy Markdown

Contributor
@scalablecory Can we go ahead and fix the Dispose behavior in QuicStream? Any reason to hold off on this? There are other issues in #756 that may need additional consideration, but it would be nice to fix the Dispose issues and make these conformance tests work for QuicStream.
No reason to hold off. It has been a thorn in my side for HTTP/3 so I would very much like to fix it :).
@stephentoub
stephentoub
commented
Oct 28, 2020

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
stephentoub
stephentoub
commented
Oct 28, 2020
View reviewed changes
Comment thread...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs
Outdated

@stephentoubstephentoubOct 28, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@geoffkizer, fyi, my machine isn't set up to run these, and they don't appear to run in CI yet (correct me if I'm wrong). So you may need to tweak them once they're merged.
@stephentoub
Fix invalid assert in StreamBuffer
ef00c0d
This can be hit by calling code and thus shouldn't be an assert. As an assert it prevents testing.
@stephentoub
Add ConnectedStreams.CreateUnidirectional
a590bcf
@stephentoub
Add initial set of Stream conformance tests
f63c227
These primarily focus on "connected streams", ones that can be arranged to communicate with each other in a producer/consumer pattern or as a bidirectional communication mechanism, e.g. NetworkStream, PipeStream, SslStream wrapped around a NetworkStream, FileStream created around pipes, etc. Later we can add more tests focused on standalone streams, e.g. FileStream created for an on-disk file, MemoryStream, etc.
@stephentoub
Add ConnectedStreams tests
59238b7
These are currently helpers used by many other tests. At some point they could become public API as well.
@stephentoub
Fix NetworkStream argument names
0d61950
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Add stream conformance tests for NetworkStream
b20d581
@stephentoub
Add stream conformance tests for QuicStream
80e15eb
@stephentoub
Fix several CryptoStream behaviors
a7f6964
1. Flushing a stream that wraps another stream for writing should always flush that underlying stream, even if no additional data was written as part of the flush.
2. Argument validation should validate buffers are not null rather than null ref'ing on a null buffer.
3. Checks for the CryptoStream mode should come after argument validation.
@stephentoub
Add stream conformance tests for CryptoStream
78bc0ac
@stephentoub
Fix FileStream argument names
3586739
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Fix BufferedStream argument names
4f32db2
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Add stream conformance tests for FileStream
cad5f6a
Specifically when used in a connected fashion, wrapped around an anonymous or named pipe.
@stephentoub
Add stream conformance tests for BufferedStream
ec88166
Specifically when used in a connected fashion, wrapped around some other connected stream.
@stephentoub
Add stream conformance tests for SslStream and NegotiateStream
3f6a0f7
@stephentoub
Fix PipeStream.Flush to not fail on readable streams
fb1de39
Consumers may expect Stream.Flush to be a nop if the stream is readable only, but PipeStream.Flush is throwing in that case. Stop doing that.
@stephentoub
Add stream conformance tests for PipeStream
fb55c4a
@stephentoub
Fix several BrotliStream behaviors
c6fa2f1
1. When passed a null buffer, it's throwing an exception with the argument name "array", even though the parameter's name is "buffer".
2. Even if there's no data written as part of the Flush{Async} call on a writeable stream, it should be calling flush on the wrapped stream.
@stephentoub
Fix several DeflateStream (and friends) issues
af13fb9
1. DeflateStream.Flush{Async} when writing needs to always flush the underlying stream, even if there's no data written as part of the flush call itself.
2. Several byte[] array arguments should be byte[] buffer. Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
3. DeflateStream.EndRead/Write needs to do additional state validation.
4. Not a bug, but simplify ReadAsync to match the sync Read implementation flow.
@stephentoub
Add stream conformance tests for Deflate/ZLib/GZip/BrotliStream
296b99a
@stephentoub
Fix PipeReader/WriterStream argument validation
230c853
@stephentoub
Add stream conformance tests for PipeReader/WriterStream
5e5afba
@stephentoub
Remove erroneous asserts from Stream.ReadWriteTask
6dc26b6
@stephentoub
Address PR feedback
c46cdd1
@stephentoub
Fix a few tests in CI
ac6aff5
@stephentoub
stephentoub
merged commit 0acbafe
into
dotnet:masterOct 28, 2020
@stephentoub
stephentoub
deleted the

streamconformance

branch
October 28, 2020 13:36
@runfoapprunfoappBot
mentioned this pull request
Nov 2, 2020

Closed

@ericstjericstj
mentioned this pull request
Nov 12, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 6, 2020
@gewarren
gewarren
commented
Mar 2, 2021

Copy link


Copy Markdown

Contributor
@stephentoub@PriyaPurkayastha Can you create a breaking change docs issue for this here? I believe this went out in Preview 1.
@stephentoub
stephentoub
commented
Mar 4, 2021

Copy link


Copy Markdown

MemberAuthor
@gewarren, I've opened dotnet/docs#23102. Thanks.
@stephentoub
stephentoub
commented
Sep 30, 2021

Copy link


Copy Markdown

MemberAuthor
@ericstj, you assigned this to me. What's the action item?
@ericstj
ericstj
commented
Sep 30, 2021

Copy link


Copy Markdown

Member
Mail is coming shortly. I'm going through all PRs with needs-breaking-change-doc and asking folks to file those issues and remove the label.
@stephentoub
stephentoub
commented
Sep 30, 2021

Copy link


Copy Markdown

MemberAuthor
Ok. As noted above in the comments, dotnet/docs#23102 was already opened and addressed.
@stephentoubstephentoub
removed
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Sep 30, 2021
@ericstj
ericstj
commented
Sep 30, 2021

Copy link


Copy Markdown

Member
Thanks for removing the label, that will remove this from my queries.
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@geoffkizergeoffkizergeoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stephentoubstephentoub

Labels

area-System.IObreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@stephentoub@geoffkizer@scalablecory@gewarren@ericstj@Dotnet-GitSync-Bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Add initial set of conformance tests for Stream - #43834
Merged
stephentoub merged 24 commits into
dotnet:masterfrom 
stephentoub:streamconformance
Oct 28, 2020
Merged
Add initial set of conformance tests for Stream#43834
stephentoub merged 24 commits into
dotnet:masterfrom 
stephentoub:streamconformance
Conversation
@stephentoub
@stephentoubstephentoub
commented
Oct 26, 2020

Copy link


Copy Markdown

Member
This also includes fixes to a variety of streams where they're out of conformance, in particular around argument names.
The tests being added are primarily focused on "connected streams", ones where two streams are used to represent two ends of a communication channel. We can subsequently flesh this out for other kinds of streams. Also, while I consolidated a bunch of tests that were spread across different stream-derived type tests, there's still more that can be consolidated over time into these centralized tests.
cc: @geoffkizer, @ericstj, @terrajobst
@Dotnet-GitSync-BotDotnet-GitSync-Bot
added
the 
area-Infrastructure-coreclr
Only use for closed issues
label
Oct 26, 2020
@stephentoubstephentoub
added

area-System.IO

breaking-change
Issue or PR that represents a breaking API or functional change over a previous release.
and removed

area-Infrastructure-coreclr
Only use for closed issues
labels
Oct 26, 2020
@ghostghost
added
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Oct 26, 2020
@stephentoubstephentoub
added this to the 6.0.0 milestone
Oct 26, 2020
@stephentoub
stephentoubforce-pushed
the
streamconformance
branch
3 times, most recently
from
56ecc67 to
7f45cf0CompareOctober 27, 2020 00:42
@runfoapprunfoappBot
mentioned this pull request
Oct 27, 2020

Closed

geoffkizer
geoffkizer
approved these changes
Oct 27, 2020
View reviewed changes
@geoffkizergeoffkizer
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

In general this looks really good. A few minor comments below.
Are you planning additional work here, or is this basically done?
Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/Common/tests/Tests/System/IO/StreamConformanceTests.cs
Outdated

Comment thread...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Security/tests/FunctionalTests/SslStreamConformanceTests.cs
Outdated

Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/NetworkStreamTest.cs
Outdated

@geoffkizer
geoffkizer
commented
Oct 27, 2020

Copy link


Copy Markdown

Contributor
I think we should also consider using the WrappedConnectedStreamTests for HttpClient request/response streams. This is a little weird because we don't actually have a Stream implementation on the server side -- but we could hack this together on top of the various loopback server stuff fairly easily. Thoughts?
@stephentoub
stephentoub
commented
Oct 27, 2020

Copy link


Copy Markdown

MemberAuthor
I think we should also consider using the WrappedConnectedStreamTests for HttpClient request/response streams. This is a little weird because we don't actually have a Stream implementation on the server side -- but we could hack this together on top of the various loopback server stuff fairly easily. Thoughts?
There are more streams in dotnet/runtime than I've included here. My thinking was we'd get this framework in, and then folks can add additional streams to test and additional tests over time. I got many of the heavy hitters here.
@stephentoub
stephentoubforce-pushed
the
streamconformance
branch
2 times, most recently
from
7108d31 to
d139d8aCompareOctober 28, 2020 02:05
@geoffkizer
geoffkizer
commented
Oct 28, 2020

Copy link


Copy Markdown

Contributor
@scalablecory Can we go ahead and fix the Dispose behavior in QuicStream? Any reason to hold off on this? There are other issues in #756 that may need additional consideration, but it would be nice to fix the Dispose issues and make these conformance tests work for QuicStream.
@scalablecory
scalablecory
commented
Oct 28, 2020

Copy link


Copy Markdown

Contributor
@scalablecory Can we go ahead and fix the Dispose behavior in QuicStream? Any reason to hold off on this? There are other issues in #756 that may need additional consideration, but it would be nice to fix the Dispose issues and make these conformance tests work for QuicStream.
No reason to hold off. It has been a thorn in my side for HTTP/3 so I would very much like to fix it :).
@stephentoub
stephentoub
commented
Oct 28, 2020

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
stephentoub
stephentoub
commented
Oct 28, 2020
View reviewed changes
Comment thread...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs
Outdated

@stephentoubstephentoubOct 28, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@geoffkizer, fyi, my machine isn't set up to run these, and they don't appear to run in CI yet (correct me if I'm wrong). So you may need to tweak them once they're merged.
@stephentoub
Fix invalid assert in StreamBuffer
ef00c0d
This can be hit by calling code and thus shouldn't be an assert. As an assert it prevents testing.
@stephentoub
Add ConnectedStreams.CreateUnidirectional
a590bcf
@stephentoub
Add initial set of Stream conformance tests
f63c227
These primarily focus on "connected streams", ones that can be arranged to communicate with each other in a producer/consumer pattern or as a bidirectional communication mechanism, e.g. NetworkStream, PipeStream, SslStream wrapped around a NetworkStream, FileStream created around pipes, etc. Later we can add more tests focused on standalone streams, e.g. FileStream created for an on-disk file, MemoryStream, etc.
@stephentoub
Add ConnectedStreams tests
59238b7
These are currently helpers used by many other tests. At some point they could become public API as well.
@stephentoub
Fix NetworkStream argument names
0d61950
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Add stream conformance tests for NetworkStream
b20d581
@stephentoub
Add stream conformance tests for QuicStream
80e15eb
@stephentoub
Fix several CryptoStream behaviors
a7f6964
1. Flushing a stream that wraps another stream for writing should always flush that underlying stream, even if no additional data was written as part of the flush.
2. Argument validation should validate buffers are not null rather than null ref'ing on a null buffer.
3. Checks for the CryptoStream mode should come after argument validation.
@stephentoub
Add stream conformance tests for CryptoStream
78bc0ac
@stephentoub
Fix FileStream argument names
3586739
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Fix BufferedStream argument names
4f32db2
Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
@stephentoub
Add stream conformance tests for FileStream
cad5f6a
Specifically when used in a connected fashion, wrapped around an anonymous or named pipe.
@stephentoub
Add stream conformance tests for BufferedStream
ec88166
Specifically when used in a connected fashion, wrapped around some other connected stream.
@stephentoub
Add stream conformance tests for SslStream and NegotiateStream
3f6a0f7
@stephentoub
Fix PipeStream.Flush to not fail on readable streams
fb1de39
Consumers may expect Stream.Flush to be a nop if the stream is readable only, but PipeStream.Flush is throwing in that case. Stop doing that.
@stephentoub
Add stream conformance tests for PipeStream
fb55c4a
@stephentoub
Fix several BrotliStream behaviors
c6fa2f1
1. When passed a null buffer, it's throwing an exception with the argument name "array", even though the parameter's name is "buffer".
2. Even if there's no data written as part of the Flush{Async} call on a writeable stream, it should be calling flush on the wrapped stream.
@stephentoub
Fix several DeflateStream (and friends) issues
af13fb9
1. DeflateStream.Flush{Async} when writing needs to always flush the underlying stream, even if there's no data written as part of the flush call itself.
2. Several byte[] array arguments should be byte[] buffer. Technically a breaking change, but the current divergence from the base Stream class is a bug, and bringing them into sync means argument exceptions that emerge from the derived type make sense when used via the base type, as well as then being able to use shared validation logic across all streams (subsequent to these changes).
3. DeflateStream.EndRead/Write needs to do additional state validation.
4. Not a bug, but simplify ReadAsync to match the sync Read implementation flow.
@stephentoub
Add stream conformance tests for Deflate/ZLib/GZip/BrotliStream
296b99a
@stephentoub
Fix PipeReader/WriterStream argument validation
230c853
@stephentoub
Add stream conformance tests for PipeReader/WriterStream
5e5afba
@stephentoub
Remove erroneous asserts from Stream.ReadWriteTask
6dc26b6
@stephentoub
Address PR feedback
c46cdd1
@stephentoub
Fix a few tests in CI
ac6aff5
@stephentoub
stephentoub
merged commit 0acbafe
into
dotnet:masterOct 28, 2020
@stephentoub
stephentoub
deleted the

streamconformance

branch
October 28, 2020 13:36
@runfoapprunfoappBot
mentioned this pull request
Nov 2, 2020

Closed

@ericstjericstj
mentioned this pull request
Nov 12, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 6, 2020
@gewarren
gewarren
commented
Mar 2, 2021

Copy link


Copy Markdown

Contributor
@stephentoub@PriyaPurkayastha Can you create a breaking change docs issue for this here? I believe this went out in Preview 1.
@stephentoub
stephentoub
commented
Mar 4, 2021

Copy link


Copy Markdown

MemberAuthor
@gewarren, I've opened dotnet/docs#23102. Thanks.
@stephentoub
stephentoub
commented
Sep 30, 2021

Copy link


Copy Markdown

MemberAuthor
@ericstj, you assigned this to me. What's the action item?
@ericstj
ericstj
commented
Sep 30, 2021

Copy link


Copy Markdown

Member
Mail is coming shortly. I'm going through all PRs with needs-breaking-change-doc and asking folks to file those issues and remove the label.
@stephentoub
stephentoub
commented
Sep 30, 2021

Copy link


Copy Markdown

MemberAuthor
Ok. As noted above in the comments, dotnet/docs#23102 was already opened and addressed.
@stephentoubstephentoub
removed
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Sep 30, 2021
@ericstj
ericstj
commented
Sep 30, 2021

Copy link


Copy Markdown

Member
Thanks for removing the label, that will remove this from my queries.
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@geoffkizergeoffkizergeoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stephentoubstephentoub

Labels

area-System.IObreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@stephentoub@geoffkizer@scalablecory@gewarren@ericstj@Dotnet-GitSync-Bot