Add DuplexStream and implement for NetworkStream and QuicStream #51434

scalablecory · 2021-04-17T10:19:48Z

Resolves #43290.

Adds DuplexStream for NetworkStream and QuicStream, and new conformance tests.

Adding issues #51431, #51432, and #51433 for filling out other Stream implementations.

@karelz please adjust labels if OCD kicks in :D

dotnet-issue-labeler · 2021-04-17T10:19:53Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-04-17T10:19:53Z

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

Resolves #43290.

Adds DuplexStream for NetworkStream and QuicStream, and new conformance tests.

Adding issues #51431, #51432, and #51433 for filling out other Stream implementations.

@karelz please adjust labels if OCD kicks in :D

Author:	scalablecory
Assignees:	-
Labels:	`area-System.IO`, `area-System.Net.Quic`, `area-System.Net.Sockets`
Milestone:	6.0.0

stephentoub · 2021-04-17T11:45:01Z

Adding issues #51431, #51432, and #51433 for filling out other Stream implementations.

PipeStream is also duplex, yes? What do we want to do for such duplex streams that lack the notion of closing one direction only... just have CloseWrites Dispose? (We can actually implement named pipes on Unix to close writes if that's desired, as it wraps socket, but that would lead to another visible behavioral difference between platforms. )

In addition to SslStream, we also need to fix NegiotiateStream presumably.

And some of the streams in HttpClient, even for 1.1, probably need to be addressed, e.g. the "raw" stream used for switching protocols.

It's fine to have issues for these rather than doing them all in this first PR, but I'd like to see those issues addressed soon after.

stephentoub · 2021-04-17T11:49:01Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs

+            }
+            catch (Exception ex) when (ex is not OutOfMemoryException)
+            {
+                throw new IOException("Unable to complete writes: " + ex.Message, ex);


We'll do a pass to move all resources into a resx subsequently?

Yes, there's a QUIC issue for it.

stephentoub · 2021-04-17T11:55:59Z

src/libraries/System.Private.CoreLib/src/System/IO/DuplexStream.cs

+                throw CreateNotSupportedException();
+
+            public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) =>
+                TaskToApm.Begin(ReadAsync(buffer, offset, count), callback, state);


Should this just throw like the other read operations?

stephentoub · 2021-04-17T11:57:24Z

src/libraries/System.Private.CoreLib/src/System/IO/DuplexStream.cs

+            {
+                if (_disposed != 0) ThrowException();
+                static void ThrowException() => throw new ObjectDisposedException(nameof(WriteOnlyProxyStream));
+            }


Can we use the ODE ThrowHelper instead?

stephentoub · 2021-04-17T11:57:47Z

src/libraries/System.Private.CoreLib/src/System/IO/DuplexStream.cs

+            }
+
+            private Exception CreateNotSupportedException() =>
+                new NotSupportedException(SR.DuplexStream_InvalidReadOnWriteOnlyStream);


Can we use the NSE ThrowHelper instead?

stephentoub · 2021-04-17T12:02:33Z

Adding issues

What about places in dotnet/runtime where we should be checking for and using DuplexStream?

Can you also file appropriate issues in dotnet/aspnetcore? Presumably there are multiple streams there that should be updated.

scalablecory · 2021-04-17T12:22:22Z

PipeStream is also duplex, yes? What do we want to do for such duplex streams that lack the notion of closing one direction only... just have CloseWrites Dispose?

If we want to use DuplexStream for any duplex stream regardless of them supporting shutdown, I go back to thinking a bool CanCompleteWrites { get; } is the only way to get success in all scenarios.

We might have it dispose. At least this will avoid having Read() hang when you need shutdown. But, this has some problems.

If your code requires shutdown to work, then the experience will be that your (maybe very long) operation will only fail near the end, potentially far from where you passed in a DuplexStream.

If your code doesn't require shutdown, but has points where it could reasonably use it, then the dispose will break things. Consider this example:

void SendSingleHttpRequest(...)
{
   using DuplexStream stream = ...;
   
   WriteConnectionCloseRequest();

   // it's the last request on this stream; HTTP doesn't require this to
   // work but it's not incorrect or even unreasonable for someone to do it.
   stream.CompleteWrites();

   // you still want to read though, so Dispose doesn't work.
   ReadRequest();
}

An alternative that fixes that is to have it forward to Flush(), but that breaks you when you do need shutdown to work.

scalablecory · 2021-04-17T12:24:50Z

Adding issues

What about places in dotnet/runtime where we should be checking for and using DuplexStream?

Can you also file appropriate issues in dotnet/aspnetcore? Presumably there are multiple streams there that should be updated.

Yeah, I'll do a quick search to find these.

stephentoub · 2021-04-17T12:57:40Z

It occurs to me as well that introducing this new base type that someone has to opt into using gives us a rare opportunity to rethink the surface area in general. For example, we can change the delegation strategy between overloads, overriding Read/WriteAsync that work with Memory to be abstract and overriding the array based overloads to call them, similarly inverting the relationship between the array and span based sync methods, overriding the APM methods to use the Async overloads with TaskToApm rather than queuing sync calls, etc. It doesn't need to be in this PR, but we should take a hard look at that soon or we'll lose our chance.

cc: @geoffkizer, @bartonjs

geoffkizer · 2021-04-18T02:07:21Z

For example, we can change the delegation strategy between overloads, overriding Read/WriteAsync that work with Memory to be abstract and overriding the array based overloads to call them, similarly inverting the relationship between the array and span based sync methods, overriding the APM methods to use the Async overloads with TaskToApm rather than queuing sync calls, etc.

Yep, totally agree. Let's take advantage of this unique opportunity.

scalablecory · 2021-04-18T09:25:59Z

How will that play out for someone overriding NetworkStream today?

stephentoub · 2021-04-18T10:40:12Z

How will that play out for someone overriding NetworkStream today?

NetworkStream overrides everything. The difference shouldn't be perceptible to a type derived from it.

ManickaP · 2021-04-19T14:22:22Z

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs

@@ -101,7 +101,9 @@ public override Task WriteAsync(byte[] buffer, int offset, int count, Cancellati

        public ValueTask ShutdownWriteCompleted(CancellationToken cancellationToken = default) => _provider.ShutdownWriteCompleted(cancellationToken);

-        public void Shutdown() => _provider.Shutdown();
+        public override void CompleteWrites() => _provider.CompleteWrites();


Why are we replacing Shutdown? I thought this is a replacement for ShutdownWriteCompleted? Or did I completely miss the whole point of this change somehow?

ShutdownWriteCompleted will become something like Task Completed { get; } to denote when both sides have finished using the stream (i.e. full graceful shutdown), which is what its current behavior is.

I thought that original ShutdownWriteCompleted means "I'm finished writing you can start sending response", which I thought is the meaning of the new CompleteWrites?
I also thought that original Shutdown is the final step of the communication, i.e.: after the response has been sent.

I don't know anything about Task Completed { get; }, I don't think we got that far with the QUIC API reviews, but if I understand the meaning that should correspond to Shutdown.

Shutdown is "shutdown writes, send EOF to peer".

ShutdownWriteCompleted was "wait for peer to acknowledge that shutdown" which we found to be mostly useless so I believe we changed it to mean "wait for peer to acknowledge that shutdown AND to shutdown their side too". It has a terrible name. This is what I'd like to see changed to a Task Completed { get; }.

ShutdownWriteCompleted actually remained for now, I've added new ShutdownCompleted alongside... But I agree ShutdownWriteCompleted doesn't have any use for us anymore, so it can just be removed anytime

I was completely bamboozled by the naming and made assumption based on that.

bartonjs · 2021-04-19T16:07:58Z

for example, we can change the delegation strategy between overloads

I think that's what I meant by "override all the things", but I'll admit we didn't talk about that line in the meeting 😄.

overriding Read/WriteAsync that work with Memory to be abstract

That's further than I had thought, but I like it. I don't see a lot of pushback on it, but it's probably worth throwing back through the big room once you know where all you want to write abstract.

geoffkizer · 2021-04-20T19:32:25Z

src/libraries/Common/tests/StreamConformanceTests/System/IO/StreamConformanceTests.cs

@@ -2938,6 +2938,72 @@ public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken
        }
    }

+    /// <summary>Base class for a connected stream that can have writes completed.</summary>
+    public abstract class DuplexConnectedStreamConformanceTests : ConnectedStreamConformanceTests


What about a test for calling Write after CompleteWrites?

Also, a test that CompleteWrites implicitly does a Flush? (i.e. call Write without Flush, then CompleteWrites and verify the unflushed data was sent)

geoffkizer · 2021-04-29T15:22:33Z

Are we closing this for now, per discussion on #51566?

Add DuplexStream and implement for NetworkStream and QuicStream. Reso…

913269c

…lves dotnet#43290

scalablecory added area-System.IO area-System.Net.Sockets area-System.Net.Quic labels Apr 17, 2021

scalablecory added this to the 6.0.0 milestone Apr 17, 2021

scalablecory requested review from stephentoub, CarnaViire, ManickaP, geoffkizer and a team April 17, 2021 10:19

scalablecory added this to To Do (Low Priority) in HTTP/3 via automation Apr 17, 2021

dotnet-issue-labeler bot added the new-api-needs-documentation label Apr 17, 2021

Fix comments in NetworkStream.

97e3c44

stephentoub reviewed Apr 17, 2021

View reviewed changes

ManickaP reviewed Apr 19, 2021

View reviewed changes

geoffkizer reviewed Apr 20, 2021

View reviewed changes

scalablecory closed this May 3, 2021

HTTP/3 automation moved this from To Do (Low Priority) to Done May 3, 2021

dotnet locked as resolved and limited conversation to collaborators Jun 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DuplexStream and implement for NetworkStream and QuicStream #51434

Add DuplexStream and implement for NetworkStream and QuicStream #51434

scalablecory commented Apr 17, 2021 •

edited

dotnet-issue-labeler bot commented Apr 17, 2021

ghost commented Apr 17, 2021

stephentoub commented Apr 17, 2021 •

edited

stephentoub Apr 17, 2021

scalablecory Apr 17, 2021

stephentoub Apr 17, 2021

stephentoub Apr 17, 2021

stephentoub Apr 17, 2021

stephentoub commented Apr 17, 2021

scalablecory commented Apr 17, 2021

scalablecory commented Apr 17, 2021

stephentoub commented Apr 17, 2021 •

edited

geoffkizer commented Apr 18, 2021

scalablecory commented Apr 18, 2021

stephentoub commented Apr 18, 2021

ManickaP Apr 19, 2021

scalablecory Apr 20, 2021 •

edited

ManickaP Apr 20, 2021

scalablecory Apr 20, 2021

CarnaViire Apr 20, 2021

ManickaP Apr 20, 2021

bartonjs commented Apr 19, 2021

geoffkizer Apr 20, 2021

geoffkizer Apr 20, 2021

geoffkizer commented Apr 29, 2021

Add DuplexStream and implement for NetworkStream and QuicStream #51434

Add DuplexStream and implement for NetworkStream and QuicStream #51434

Conversation

scalablecory commented Apr 17, 2021 • edited

dotnet-issue-labeler bot commented Apr 17, 2021

ghost commented Apr 17, 2021

stephentoub commented Apr 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Apr 17, 2021

scalablecory commented Apr 17, 2021

scalablecory commented Apr 17, 2021

stephentoub commented Apr 17, 2021 • edited

geoffkizer commented Apr 18, 2021

scalablecory commented Apr 18, 2021

stephentoub commented Apr 18, 2021

Choose a reason for hiding this comment

scalablecory Apr 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartonjs commented Apr 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffkizer commented Apr 29, 2021

scalablecory commented Apr 17, 2021 •

edited

stephentoub commented Apr 17, 2021 •

edited

stephentoub commented Apr 17, 2021 •

edited

scalablecory Apr 20, 2021 •

edited