Make StreamPipeReader.TryRead more usable

[halter73]

Today the StreamPipeReader.TryRead (which overrides PipeReader.TryRead) either never returns any data (when ReadAsync is never called), or is likely to provide corrupted data (when ReadAsync is sometimes called).

The only way it's even a little bit useful today is if you've read data calling ReadAsync, partially advance, and already know data is available without any call to InnerStream.ReadAsync in the background. This is extremely limited though, and requires the consumer to know a lot about the implementation and limitations of the PipeWriter they are using.

See https://github.com/dotnet/corefx/pull/39228#discussion_r300792703

[waf]

If it's not possible to change the behavior, I'd at least appreciate a mention in the method documentation that ReadAsync must be called first. I expected ReadAsync and TryRead to be asynchronous / synchronous counterparts, when really they're supposed to be used in tandem.

I came to this repo to enter a bug about it; luckily found this issue first!

[RayKoopa]

[...] I'd at least appreciate a mention in the method documentation that ReadAsync must be called first. I expected ReadAsync and TryRead to be asynchronous / synchronous counterparts, when really they're supposed to be used in tandem.

I came to this repo to enter a bug about it; luckily found this issue first!

I am quoting this because it expresses exactly what I thought. It should be made clear that TryRead is - as it currently looks like - not a synchronous alternative to ReadAsync. There has been confusion on StackOverflow aswell and I hope I answered correctly.

[davidfowl]

@waf or @RayKoopa would one of you like to contribute docs here?

[RayKoopa]

Thanks for the offer, but I am afraid I did not understand yet whether this a "bug" or not.

For example, mgravell's Pipelines.Sockets.Unofficial library implements TryRead in a way it "just works" for synchronous reads, as if that was intended (despite him not recommending synchronous Pipeline usage when I noticed there's no synchronous flush).

Since I've rewritten my code to be asynchronous due to this, I did not investigate further. Thus I am not sure what to add to the documentation from my point of view other than a warning like "This is not a synchronous counterpart to ReadAsync, the Pipelines API is meant to be used asynchronously." 🤔

[waf]

I've opened PR #52237 to highlight this in the docs. As @RayKoopa said, it's just a small note that describes this behavior.

[Gavin-Williams]

As someone new to networking, this method still doesn't do what I expect

if (reader.TryRead(out ReadResult result))

never return true. And if the context-docs are meant to explain why it doesn't do what it should do by it's name, they have failed. It says right at the top

Attempts to synchronously read data from the PipeReader

So I take it this method has been updated to be the synchronous version of the asynchronous version?

When I read the terribly formatted extra text in the context-docs, I think that's trying to say that this method is not used for reading the PipeReader, but rather for getting lost messages

Any unconsumed data from a previous asynchronous read will be available.

But it's not doing that either, because the method just returns false all the time.

Edit: Just to expand on how this stuff looks to someone new to networking - I've already detected an incoming message with

clientSocket = await masterSocket.AcceptAsync();

that's where the await happens, so the reader is created to read the data from the socket that has already been accepted. There shouldn't be anything to await at that point. It just needs to be read out. That's why I look to 'reader.TryRead.

Update: As a consquence of reading peoples comments on other people having the issue, my understanding the workings has changed, I can see now that the connection never closes. So ReadAsync is waiting for more data, not AcceptAsync but I still don't know what reader.TryRead() does.