Make DuplexStream work for all duplex streams

[scalablecory]

We approved #43290 with changes, but the changes ended up making DuplexStream incompatible with streams that are technically duplex but can't complete writes. An example is e.g. PipeStream.

Proposed API

-public abstract class DuplexStream : Stream
+public class DuplexStream : Stream
{
+   public virtual bool CanCompleteWrites =>
+       false;

    // when disposed, this write-only stream will call CompleteWrites().
    // this allows compat with e.g. StreamWriter that knows nothing about shutdown.
    public Stream GetWriteOnlyStream() => throw null;
    
-   public abstract void CompleteWrites();
+   public virtual void CompleteWrites() =>
+       Flush();

-    public abstract ValueTask CompleteWritesAsync(CancellationToken cancellationToken = default);
+    public virtual ValueTask CompleteWritesAsync(CancellationToken cancellationToken = default) =>
+        new ValueTask(FlushAsync(cancellationToken));
}

Rationale and discussion

To sum up a discussion happening over on #43290 and its PR, we have two options for what PipeStream would do with the current API we have:

1. Make CompleteWrites() call Flush().
   • This will allow use with protocols that don't strictly require CompleteWrites() to send an EOF, but do have a good place to reasonably call it -- an example being HTTP/1.x with Connection: close, where you know you won't be sending another request -- to succeed.
   • But, it will fail for any protocol that requires EOF: the subsequent Read() would hang, because the peer never saw EOF to know to send its response.
2. Make CompleteWrites() call Dispose().
   • This will prevent Read() from hanging, but it makes those opportunistic calls to CompleteWrites() (e.g. the Connection: close scenario above) bomb the Stream when it would have otherwise succeeded.

I don't like either of those behaviors: in either case you end up having some exception (or worse, a hang) occurring far away from where you were passed the DuplexStream as input, and possibly after a long period of writes.

This proposal would allow code to check that its input DuplexStream supports sending EOF and fail fast, just as we use the existing Can... properties for today.

It would break with existing behavior that has the method affected by the Can... throw. Instead, it will forward to Flush(). This would allow callers to write CompleteWritesAsync(cancellationToken)+Dispose() wherever they would have written FlushAsync(cancellationToken)+DisposeAsync() today.

[ghost]

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

Issue Details

---

We approved #43290 with changes, but the changes ended up making DuplexStream incompatible with streams that are technically duplex but can't complete writes. An example is e.g. PipeStream.

Proposed API

public abstract class DuplexStream : Stream
{
+   public virtual bool CanCompleteWrites =>
+       false;

    // when disposed, this write-only stream will call CompleteWrites().
    // this allows compat with e.g. StreamWriter that knows nothing about shutdown.
    public Stream GetWriteOnlyStream() => throw null;
    
-   public abstract void CompleteWrites();
+   public virtual void CompleteWrites() =>
+       Flush();

-    public abstract ValueTask CompleteWritesAsync(CancellationToken cancellationToken = default);
+    public virtual ValueTask CompleteWritesAsync(CancellationToken cancellationToken = default) =>
+        new ValueTask(FlushAsync(cancellationToken));
}

Rationale and discussion

To sum up a discussion happening over on that issue and its PR, we have a two options for what PipeStream would do with the current API we have:

1. Make CompleteWrites() call Flush().
   • This will allow use with protocols that don't strictly require CompleteWrites() to send an EOF, but do have a good place to reasonably call it -- an example being HTTP/1.x with Connection: close, where you know you won't be sending another request -- to succeed.
   • But, it will fail for any protocol that requires EOF: the subsequent Read() would hang, because the peer never saw EOF to know to send its response.
2. Make CompleteWrites() call Dispose().
   • This will prevent Read() from hanging, but it makes those opportunistic calls to CompleteWrites() (e.g. the Connection: close scenario above) bomb the Stream when it would have otherwise succeeded.

I don't like either of those behaviors: in either case you end up having some exception (or worse, a hang) occurring far away from where you were passed the DuplexStream as input, and possibly after a long period of writes.

This proposal would allow code to check that its input DuplexStream supports sending EOF and fail fast, just as we use the existing Can... properties for today.

It would break with existing behavior that has the method affected by the Can... throw. Instead, it will forward to Flush(). This would allow callers to write CompleteWritesAsync(cancellationToken)+Dispose() wherever they would have written FlushAsync(cancellationToken)+DisposeAsync() today.

Author:	scalablecory
Assignees:	-
Labels:	api-suggestion, area-System.IO
Milestone:	6.0.0

[scalablecory]

CC @stephentoub

[stephentoub]

I'm not a fan of adding CanCompleteWrites, nor making the methods virtual instead of abstract.

Are there other known cases of this besides PipeStream? PipeStream is going to have other issues besides this, in particular because AnonymousPipeClient/ServerStream are unidirectional, with only NamedPipeClient/ServerStream being duplex (and even then only when created with PipeDirection.InOut). We can't insert DuplexStream between NamedPipeClient/ServerStream and PipeStream, and putting DuplexStream below PipeStream would then claim AnonymousPipeClient/ServerStream as duplex, which is erroneous.

So.... the design already doesn't work for NamedPipeClient/ServerStream. If it's the only case of this, I'm tempted to say "oh well", and not complicate the design further for a case that's already imperfect. If there are other known cases, we should look at 'em.

[scalablecory]

If it's the only case of this, I'm tempted to say "oh well", and not complicate the design further for a case that's already imperfect. If there are other known cases, we should look at 'em.

It looks like shutdown behavior doesn't work until TLS 1.3 so we'll probably need some sort of flag if we want to make SslStream support this.

[stephentoub]

so we'll probably need some sort of flag if we want to make SslStream support this

And if we don't, we're basically down to just NetworkStream and QuicStream deriving from this... which begs the question of whether it's actually worthwhile to introduce this DuplexStream, and I'd venture to say "not right now".

[davidfowl]

And if we don't, we're basically down to just NetworkStream and QuicStream deriving from this... which begs the question of whether it's actually worthwhile to introduce this DuplexStream, and I'd venture to say "not right now".

Let me just +1, I don't really see the value in adding this so late. I wish we had it earlier (like wayyyy earlier).

[scalablecory]

Definitely not worthwhile without that flag, and I don't think it's appropriate to introduce a new type where its only feature is optional. I'm closing this out for now.

[geoffkizer]

Is there an issue to track considering this for the future, and tracking all the issues we've hit in considering it for 6.0?

Do we even want to do this in the future? If so what does success here mean?

It seems to me like we are saying
(a) we do want to do something here, but we're not sure exactly what
(b) we don't have runway to figure this out in 6 (which I agree with)

If that's the case, then let's make sure we are setting ourselves up for success in 7 by tracking all the issues, continuing to discuss requirements/scenarios etc, and trying to front-load this work in 7.