FullDuplexStream requires Flushing, breaking scenarios
Closed this issue · 5 comments
I'm (attempting to) use FullDuplexStream to write a unit test for a SSL stream, where one side acts as the server and the other side as the client.
As part of this scenario, I'm calling SslStream.AuthenticateAsClientAsync and AuthenticateAsServerAsync, more or less like this:
await Task.WhenAll(
client.AuthenticateAsClientAsync(...),
client.AuthenticateAsServerAsync(...))The await never returns. Based on #37, it seems that FullDuplexStream will require the calling code to flush. It also looks like the SslStream methods do not flush.
When I wrap the streams in a custom stream which calls .FlushAsync after WriteAsync, the above snippet starts working.
Calling Flush explicitly appears to be sufficiently uncommon, so it'd be nice if there were a way to have FullDuplexStream work without having to explicitly call Flush .
Yes, as you found on the linked issue:
is consistent with other streams and actually makes this stream more useful in testing environments since it better emulates most other types of streams (e.g. if product code is failing to flush, that bug will now show up with the new FullDuplexStream behavior).
If the API you're calling does not flush the stream it writes to but assumes the bytes propagate to the remote party, that code is buggy.
Any reason you didn't file a bug against the code you're using?
It seems the bug is here:
This struct is used, assuming that WriteAsync will send the bytes, I guess. It should call FlushAsync as well in order to ensure that contract, independently of the underlying Stream's buffering characteristics.
If the API you're calling does not flush the stream it writes to but assumes the bytes propagate to the remote party, that code is buggy.
That's a very strong statement, when contrasted with the documentation for NetworkStream.Flush:
The Flush method implements the Stream.Flush method; however, because NetworkStream is not buffered, it has no effect on network streams. Calling the Flush method does not throw an exception.
Testing network streams was mentioned as one of the use cases for FullDuplexStream, hence I expected it to have similar semantics as NetworkStream.
I worked around the difference in behavior by wrapping both ends of the full duplex stream in a stream which will call Flush after each Write, so this issue is not blocking for me.
That's a very strong statement,
Some stream implementations perform local buffering of the underlying data to improve performance. For such streams, you can use the Flush or FlushAsync method to clear any internal buffers and ensure that all data has been written to the underlying data source or repository.
So you see we have documented license to implement a stream with buffers, which is why the Flush method is on the abstract Stream class to begin with. So anyone that takes Stream and writes to it and requires that the bytes be transmitted must flush or else they're taking a dependency on a detail (implementation-detail or documented) of a derived class and thus cannot work correctly with every Stream.
The fact that NetworkStream as one particular class does not buffer is fine -- an implementation can choose to not buffer. But that's not the only stream used to communicate to other machines or processes and some of those do buffer.
Testing network streams was mentioned as one of the use cases for FullDuplexStream, hence I expected it to have similar semantics as NetworkStream.
Yes it is a good test stream to use, and per the description in #37 which you linked to:
This introduces a requirement that FullDuplexStream paired streams must be flushed after writing before the reader can read it. This is a behavioral breaking change, but is consistent with other streams and actually makes this stream more useful in testing environments since it better emulates most other types of streams (e.g. if product code is failing to flush, that bug will now show up with the new FullDuplexStream behavior).
I can tell you, I've personally found bugs in a couple of my own libraries where we failed to flush by using FullDuplexStream to test. Shipping libraries that reliably flush whenever they need to know they've been transmitted guarantees my libraries work with the broadest set of Stream implementations.
Since you're not blocked and we have an active issue tracking the bug in .NET, I'll close the issue. Thanks for reporting.