Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QuicStream.ReadsClosed completes before EOS is consumed #79818

Closed
bentoi opened this issue Dec 19, 2022 · 4 comments · Fixed by #90253
Closed

QuicStream.ReadsClosed completes before EOS is consumed #79818

bentoi opened this issue Dec 19, 2022 · 4 comments · Fixed by #90253
Assignees
Labels
area-System.Net.Quic question Answer questions and provide assistance, not an issue with source code or documentation.
Milestone

Comments

@bentoi
Copy link

bentoi commented Dec 19, 2022

This is related to the discussion on #77216 .

It looks like today QuicStream.ReadsClosed gets completed as soon as the last piece of data is returned by QuicStream.ReadAsync and before QuicStream.ReadAsync returns 0 to indicate the end of the stream.

This is demonstrated by the test case attached. What are the excepted semantics for ReadsClosed?

Should it complete:

  1. as soon as EOS is received and even if there's buffered data
  2. when EOS is received and after the buffered data is consumed
  3. when EOS is received and after the buffered data and EOS are consumed.

quicstreameos.zip

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 19, 2022
@ghost
Copy link

ghost commented Dec 19, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

This is related to the discussion on #77216 .

It looks like today QuicStream.ReadsClosed gets completed as soon as the last piece of data is returned by QuicStream.ReadAsync and before QuicStream.ReadAsync returns 0 to indicate the end of the stream.

This is demonstrated by the test case attached. What are the excepted semantics for ReadsClosed?

Should it complete:

  1. as soon as EOS is received and even if there's buffered data
  2. when EOS is received and after the buffered data is consumed
  3. when EOS is received and after the buffered data and EOS are consumed.

quicstreameos.zip

Author: bentoi
Assignees: -
Labels:

area-System.IO, untriaged

Milestone: -

@ghost
Copy link

ghost commented Dec 19, 2022

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

Issue Details

This is related to the discussion on #77216 .

It looks like today QuicStream.ReadsClosed gets completed as soon as the last piece of data is returned by QuicStream.ReadAsync and before QuicStream.ReadAsync returns 0 to indicate the end of the stream.

This is demonstrated by the test case attached. What are the excepted semantics for ReadsClosed?

Should it complete:

  1. as soon as EOS is received and even if there's buffered data
  2. when EOS is received and after the buffered data is consumed
  3. when EOS is received and after the buffered data and EOS are consumed.

quicstreameos.zip

Author: bentoi
Assignees: -
Labels:

untriaged, area-System.Net.Quic

Milestone: -

@ManickaP
Copy link
Member

ManickaP commented Jan 2, 2023

  1. when EOS is received and after the buffered data is consumed

is the answer here.

It shouldn't be case 3. as the goal is to get the info about reading side state without issuing ReadAsync. And for case 1., it would be confusing to report reading side closed while still having data in buffer. Note that the peer might close the writing side separately from the last data. Also the task gets finished not only with FIN flag, but also with AbortRead (send STOP_SENDING) or the peers RESET_STREAM as a corresponding exception.

If you have different opinions on the behavior, please do share them. We can discuss them and re-evaluate the current behavior.

Final note, I looked at your example and since you're using await using var localStream ... and await using var remoteStream ... you don't need DisposeWhenClosed, await using will take care of that. And I'm not sure that hooking up DisposeAsync as a follow up on Reads/WritesClosed is the best way to do this. They weren't designed with something like this in mind, but I do concur that this might be valid use-case and we could make it behave somewhat better at least.

@ManickaP ManickaP added question Answer questions and provide assistance, not an issue with source code or documentation. and removed untriaged New issue has not been triaged by the area owner labels Jan 2, 2023
@ManickaP ManickaP added this to the 8.0.0 milestone Jan 2, 2023
@ManickaP
Copy link
Member

ManickaP commented Jan 2, 2023

Triage: marking as a question/discussion for now. There might not be anything actionable from this and if so, it should be fixed in 8.0.

@ManickaP ManickaP self-assigned this Jan 26, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 9, 2023
@karelz karelz modified the milestones: 8.0.0, 9.0.0 Aug 14, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 13, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Quic question Answer questions and provide assistance, not an issue with source code or documentation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants